Zylann / godot_voxel

Voxel module for Godot Engine
MIT License
2.68k stars 251 forks source link

Ability to save changes without using LOD? #146

Closed wacyym closed 4 years ago

wacyym commented 4 years ago

Can i save terrain changes using VoxelTerrain (not VoxelTerrainLod)? I try to do this with this settings using VoxelStreamRegionFiles: image

But changes not saved after restart level.

Edit: using VoxelTerrainLOD + VoxelStreamRegionFiles works...

Zylann commented 4 years ago

Do you get any errors in the console? Are any files getting saved or the directory remains empty?

menip commented 4 years ago

Are we supposed to be able to save/load blocky terrain (VoxelTerrain) with, for example, VoxelStreamRegionFiles? I've tried, but it seems to not work. As shown in OPs screenshot, VoxelTerrain has warning saying "VoxelTerrain supports only stream channels 'Type' or 'SDF'."

There are no errors being printed to console. What I noticed is that when option Save Fallback Output is enabled, some files do get saved. When it is not, nothing gets saved. Additionally, terrain modifications don't get saved: try to add/remove voxels, restart level, then you see that only terrain from fallback stream is there, modifications are not.

I was originally thinking I have to make my own generator to have persistent terrain modifications, but would much prefer for a built in solution, such as VoxelStreamRegionFiles.

Zylann commented 4 years ago

Are we supposed to be able to save/load blocky terrain (VoxelTerrain) with, for example, VoxelStreamRegionFiles?

Yes.

As shown in OPs screenshot, VoxelTerrain has warning saying "VoxelTerrain supports only stream channels 'Type' or 'SDF'."

It can mean three possible things:

What I noticed is that when option Save Fallback Output is enabled, some files do get saved

That option saves blocks as they are generated. When turned off, it only saves modified blocks and generates the others again in the next session.

If you run the game in verbose mode (-v) you should see a console log when a block is detected as being modified. They are normally saved when they get unloaded. I'm not sure if they will if you use the Stop button from the editor (as it kills the game ungracefully?). If not then there is probably a bug in VoxelTerrain.

menip commented 4 years ago

They are normally saved when they get unloaded. I'm not sure if they will if you use the Stop button from the editor (as it kills the game ungracefully?).

Ah, this was the important bit. I tried modifying blocks, flying out of view distance so my modified block gets unloaded, then restarting game. Modifications were loaded as expected. However, changes do not get saved by just exiting the game, gracefully or not. I expected that by default, if you exit in any way, changes would be saved (autosaving, but I guess this can just be triggered via script).

Note I'm only testing inside Godot, maybe with full on export, terrain will get saved on window close? Short clip of what is happening: https://vimeo.com/441141222

Zylann commented 4 years ago

Note I'm only testing inside Godot, maybe with full on export, terrain will get saved on window close?

I doubt exporting will change it.

There is this in the destructor of VoxelTerrain: https://github.com/Zylann/godot_voxel/blob/071c5ee5c0862eef18a1faeda1c7ba4902d8fcf4/terrain/voxel_terrain.cpp#L42 I wonder why that doesnt catch modifications.

menip commented 4 years ago

Exposing VoxelTerrain.save_all_modified_blocks(with_copy) and having below GDScript code, saves modifications on window close, as is expected. Note when stopping via editor (not graceful exit), modifications not saved.

func _notification(what):
    if what == MainLoop.NOTIFICATION_WM_QUIT_REQUEST:
        $VoxelTerrain.save_all_modified_blocks(false)
        get_tree().quit() # default behavior
Zylann commented 4 years ago

My guess is that something makes the _stream_thread stop when the game closes, before the terrain gets destroyed. That would explain why it doesn't get to the conditional and fails to save automatically.

menip commented 4 years ago

@Zylann Can you specify which conditional you are talking about?

Zylann commented 4 years ago

This one https://github.com/Zylann/godot_voxel/blob/071c5ee5c0862eef18a1faeda1c7ba4902d8fcf4/terrain/voxel_terrain.cpp#L39

menip commented 4 years ago

No, it gets there. I have both lines printed from inside conditional, but upon restarting, I don't see modifications.

if (_stream_thread != nullptr) {
        // Schedule saving of all modified blocks,
        // without copy because we are destroying the map anyways
        PRINT_VERBOSE("About to do the save");
        save_all_modified_blocks(false);
        PRINT_VERBOSE("Save should be done");

        memdelete(_stream_thread);
    }
Zylann commented 4 years ago

Must need more debugging then. The problem shouldn't be far from here. There is a print further down, which looks like this:

Requesting save of block {0}

If that's not printed, it means no modified blocks were found (you may want to use a debugger to step through the code though)

menip commented 4 years ago

Yea, that Requesting... line gets printed. From what I see, the call goes into BlockThreadManager::push(...) and the push function does run, as I'm getting some debugg prints from start and end of push function. I'll try to use a debugger, so far have been debugging with prints. Trying to get experience with C++, so hopefully can figure out whats wrong.

Zylann commented 4 years ago

Then the problem might be in the thread then. Perhaps it stops without having processed the remaining input: when long inactive, the streaming thread waits for a semaphore. When destroyed, the semaphore gets raised, and the thread exits right away without actually checking the remaining input queue, and it should be fixed there because forcing save manually from outside doesnt guarantee that (timing luck).

menip commented 4 years ago

Then the problem might be in the thread then. Perhaps it stops without having processed the remaining input: when long inactive, the streaming thread waits for a semaphore. When destroyed, the semaphore gets raised, and the thread exits right away without actually checking the remaining input queue, and it should be fixed there because forcing save manually from outside doesnt guarantee that (timing luck).

After more testing, it's seems that whether save happens is "random", which would indicate that the issue is indeed due to thread things. Gonna keep playing with debugger, see if I can find out whats up.

menip commented 4 years ago

The issue seems to be that in VoxelBlockThreadManager destructor, when we have a job whose semaphore value is not 0 (eg it is 1). My understanding is that we need to have semaphore value be 0, before we can act on the job, so I think we need to wait until semaphore value is 0.

Edit: I tried the following code in VoxelBlockThreadManager destructor, but it doesn't seem to solve it. The problem does seem to happen every time semaphore value isn't 0 upon entry into VoxelBlockThreadManager destructor however.

while(job.semaphore->get() > 0) {
    job.semaphore->wait();
}

Edit: I now think the issue is in VoxelBlockThreadManager::thread_func(...), probably under the condition that data.thread_exit is true.

Edit: there seems to be two issues, one with semaphore values, as already mention in this comment. Another is thread ending before call Thread::wait_to_finish(job.thread), which would seem to be the timing issue mentioned by Zylann. The thread ending may also not be issue, but just printing order (as I'm not sure how that works from threads).

Edit: I think I have a fix that works, not sure if correct way to do it though. Will submit PR shortly.

Zylann commented 4 years ago

thread ending before call Thread::wait_to_finish(job.thread)

That's not an issue, it's actually very good (usually) because that means no blocking occurs.

Meanwhile while reading the code I'm more confident about how to fix it, just need to find the time to do it properly.

menip commented 4 years ago

@wacyym eef3818 seems to have fixed the issue. Can you confirm that your issue is resolved?