Skip to content

Fix save_generator_output not working in VoxelLodTerrain.#728

Open
SiriusRP wants to merge 1 commit intoZylann:masterfrom
SiriusRP:fix_caching_in_voxel_lod_terrain
Open

Fix save_generator_output not working in VoxelLodTerrain.#728
SiriusRP wants to merge 1 commit intoZylann:masterfrom
SiriusRP:fix_caching_in_voxel_lod_terrain

Conversation

@SiriusRP
Copy link

@SiriusRP SiriusRP commented Feb 6, 2025

cache_generated_blocks is passed to LoadBlockDataTask where it is used to check if a block should be generated and saved if not found by the stream. However, it default constructs to false and is never written to. Meaning this check will always fail.

} else if (voxel_query_data.result == VoxelStream::RESULT_BLOCK_NOT_FOUND) {
if (_generate_cache_data) {
Ref<VoxelGenerator> generator = _stream_dependency->generator;
if (generator.is_valid()) {
VoxelGenerator::BlockTaskParams params;
params.voxels = _voxels;
params.volume_id = _volume_id;
params.block_position = _position;
params.lod_index = _lod_index;
params.block_size = _block_size;
params.stream_dependency = _stream_dependency;
params.priority_dependency = _priority_dependency;
params.use_gpu = _generator_use_gpu;
params.data = _voxel_data;
IThreadedTask *task = generator->create_block_task(params);
VoxelEngine::get_singleton().push_async_task(task);
_requested_generator_task = true;
} else {
// If there is no generator... what do we do? What defines the format of that empty block?
// If the user leaves the defaults it's fine, but otherwise blocks of inconsistent format can
// end up in the volume and that can cause errors.
// TODO Define format on volume?
}
} else {
_voxels.reset();
}
}

Setting this to construct as true fixes the issue and better aligns with how VoxelTerrain is written. But if there is a better option for fixing this I'm happy to change it.

@Zylann
Copy link
Owner

Zylann commented Feb 6, 2025

It is not written to because I wanted to investigate and test before introducing it in this terrain type, meaning that feature is indeed not officially supported here at the moment. I don't remember right now all the details that this implies, but I know it might have some edge cases.
Another reason is that for this terrain type, I haven't needed that caching yet. Also, it consumes a LOT more memory, and saving to stream can reduce the effectiveness of detail rendering.
There might be considerations to take with full loading mode, which, while not using that flag currently, could also use caching somehow.

Note that this boolean is not directly about saving, it is about caching. Saving indeed only occurs if you also enable that option on the stream. There could be ways to save without caching, which would have to happen in the meshing task, but that seems weird.

If you make it default to true, you are also not fixing the fact that nothing sets it. You'll just make the terrain cache everything all the time and use more memory (assuming it works), which some projects might not want.

Basically, this option isn't really ready to be exposed or changed yet.

@SiriusRP
Copy link
Author

SiriusRP commented Feb 6, 2025

Do you have any suggestions for this should be implemented?

I have a custom generator that reads GIS data into VoxelLodTerrain. Currently it is very slow and I was hoping to speed up this process by writing the generated blocks to disk and then streaming them on subsequent loads.

@Zylann
Copy link
Owner

Zylann commented Feb 6, 2025

Do you have any suggestions for this should be implemented?

Not currently. My free time is currently blocked by something else so I can't work on my projects much.

I have a custom generator that reads GIS data into VoxelLodTerrain. Currently it is very slow and I was hoping to speed up this process by writing the generated blocks to disk and then streaming them on subsequent loads.

If you want to save this into a stream, you could consider doing this process ahead of time maybe. You could even consider implementing a custom stream directly, or figuring how to optimize what is slow. If this is slow because you're doing web requests though, you might be taking the wrong approach IMO, since you would end up with all task threads blocked waiting for requests, not doing actual work, the system isn't optimized for that.
You're also welcome to test your current change with your project more in depth, even if I don't expose that yet.
Unfortunately for the same reasons stated above I can't offer deeper help on subjects like this at the moment,

@Zylann Zylann force-pushed the master branch 4 times, most recently from b12cadf to dbe0bcb Compare March 6, 2025 17:58
@Zylann Zylann force-pushed the master branch 6 times, most recently from 7b15242 to e94948f Compare March 16, 2025 01:31
@Zylann
Copy link
Owner

Zylann commented Mar 23, 2025

I pushed an experimental property that exposes the variable in bfc72a9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants