Rework viewport capture in preview generation

[RandomShaper]

An additional patch to #87229 + #88391.

Bear in mind this is brittle, as it's tailored to the current usage of the semaphore by the importers, namely, wait until rendering has happened so the preview can be captured from some viewport. Anyway, there's also the notion that all this set of PRs may be not needed anymore at some point (see #87229, where that was originally stated). EDIT: Let me add that, in case this had to stay for longer, it could be abstracted out a bit more in a way the preview generators have an API to wait for rendering to happen, leaving the sync/async details to the helpers.

Fixes #88576.
Bugsquad edit: fixes #88728

Worked well in my tests,. Please give it a try.

[akien-mga]

I don't seem to reproduce the deadlock on Linux with my test project https://github.com/team-godog/NGJ2023_Godog (running with --rendering-driver opengl3).

With this PR, I get a bunch of errors when generating the thumbnails (after rm -rf ~/.cache/godot).

ERROR: Expected Image data size of 161x130x1 (DXT5 RGBA8 with 7 mipmaps) = 28848 bytes, got 29424 bytes instead.
   at: initialize_data (./core/io/image.cpp:2220)
ERROR: Condition "image->is_empty()" is true. Returning: Ref<Image>()
   at: texture_2d_get (drivers/gles3/storage/texture_storage.cpp:1045)
ERROR: Expected Image data size of 69x47x1 (DXT5 RGBA8 with 6 mipmaps) = 4672 bytes, got 4704 bytes instead.
   at: initialize_data (./core/io/image.cpp:2220)
ERROR: Condition "image->is_empty()" is true. Returning: Ref<Image>()
   at: texture_2d_get (drivers/gles3/storage/texture_storage.cpp:1045)
ERROR: Expected Image data size of 126x70x1 (DXT5 RGBA8 with 6 mipmaps) = 12352 bytes, got 12432 bytes instead.
   at: initialize_data (./core/io/image.cpp:2220)
ERROR: Condition "image->is_empty()" is true. Returning: Ref<Image>()
   at: texture_2d_get (drivers/gles3/storage/texture_storage.cpp:1045)
ERROR: Expected Image data size of 61x46x1 (DXT5 RGBA8 with 5 mipmaps) = 4128 bytes, got 4144 bytes instead.
   at: initialize_data (./core/io/image.cpp:2220)
ERROR: Condition "image->is_empty()" is true. Returning: Ref<Image>()
   at: texture_2d_get (drivers/gles3/storage/texture_storage.cpp:1045)

These errors don't happen in current master.

This is on Linux with an AMD GPU.

[RandomShaper]

I've been able to repro. This is puzzling. I can't get why that happens because of this change. However, I can understand why there's a mismatch between the expected size and the passed size. The function at

godot/drivers/gles3/storage/texture_storage.cpp

Line 999 in b714563

Ref<Image> TextureStorage::texture_2d_get(RID p_texture) const {

computes the size needed to hold the obtained texture data based on alloc_width-height. Later, Image::create_from_data() is called with the real width-height, which in turns invokes the same algorithm to compute the size of the data. It's worth mentioning that the sizes of the mips that those two calls obtain are different, which leads to the mismatch.

Using alloc_width-height seems to be done to account for the size of the compressed block size. However, the function that computes the data size already takes that into account, but per mipmap. I think that's where the difference lies. Pre-compensating leads to different mipmap sizes being considered.

So, it seems there's a bug there. However, again, I don't get what all that has to do with the changes in this PR and, moreover, I don't get why such mismatch hadn't been an issue until now. I need someone lucid to help me.

[Malcolmnixon]

This PR solves my deadlock problem on the godot_xr_template project.

While the PreviewSemaphore/wait/post works, it seems a little weird. Usually you want some result, and you internally use a semaphore to implement it. Here you're overriding a semaphore to trigger some result you want.

Would it be possible to hide the redraw capture/wait inside a function such as _generate_frame_draw() which easily describes what it's trying to do:

// This method returns when the preview frame has been drawn.
void EditorMaterialPreviewPlugin::_generate_frame_draw() const {
    // One-shot subscribe to RenderServer frame_pre_draw to detect start of next frame.
    RS::get_singleton()->connect(SNAME("frame_pre_draw"), callable_mp(const_cast<EditorMaterialPreviewPlugin *>(this), &EditorMaterialPreviewPlugin::_generate_frame_started), Object::CONNECT_ONE_SHOT);

    if (EditorResourcePreview::get_singleton()->is_threaded()) {
        // Preview is multi-threaded - wait for the preview_done notification.
        preview_done.wait();
    } else {
        // Preview is single-threaded - directly call draw.
        RS::get_singleton()->draw(false);
    }
}

void EditorMaterialPreviewPlugin::_generate_frame_started() {
    RS::get_singleton()->viewport_set_update_mode(viewport, RS::VIEWPORT_UPDATE_ONCE); //once used for capture

    // If threaded then request a callback when the frame-draw completes.
    if (EditorResourcePreview::get_singleton()->is_threaded()) {
        RS::get_singleton()->request_frame_drawn_callback(callable_mp(const_cast<EditorMaterialPreviewPlugin *>(this), &EditorMaterialPreviewPlugin::_preview_done));
    }
}

void EditorMaterialPreviewPlugin::_preview_done() {
    preview_done.post();
}

void EditorMaterialPreviewPlugin::abort() {
    if (EditorResourcePreview::get_singleton()->is_threaded()) {
        preview_done.post();
    }
}

Something like this would use the semaphore only when necessary, rather than overriding the semaphore to achieve non-semaphore side-effects.

[RandomShaper]

@Malcolmnixon, I was considering doing a full abstraction in a future iteration if this turned out to have to stay for long. However, you've given me enough boost to do it now. I've gone even a bit further than what you suggested.

The texture data size mismatch issue persits, though.

[dsnopek]

For me, on a fresh project, adding a MeshInstance3D and creating a new BoxMesh is enough to completely deadlock the editor.

I haven't evaluated the code, but the changes in this PR fix the issue for me!

[Malcolmnixon]

This is working fine for me on the test project. Many thanks!

[Malcolmnixon]

This looks good to me - the optional semaphore/wait is hidden behind the DrawRequester class/API. It also appears to fix the different gl_compatibility deadlocks being reported.

[akien-mga]

Thanks!