Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run resource previewer on the main thread if using GL compatibility #87229

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jan 15, 2024

I don't know why the issue was surfaced by #86587, but to my understanding, it was already there previously. The issue is that with OpenGL (Compatibility rendering method) you can't deal with textures from arbitrary threads. The rendering server tries to do the right thing by sending the tasks to a queue, but the resource importer will blindly continue doing its job without waiting for them to be created. The worst consequence is that, given the relevant texture-related functions take references to resource references (Ref<>&), by the time the previewer thinks it's done with some images it can decrease their refcount to zero. Because of that, there are dangling references by the time the queue is flushed later.

This PR fixes the ill situations by just letting the main thread flush any outstanding resource importing tasks every idle frame. That ensures that any previewer, either one that directly deals with rendering or one that doesn't, because in the end all of them have to create the preview images, don't incurr in sync issues.

This will affect the responsiveness of the editor while there are queued preview tasks. The code tries to let the editor breathe a bit to avoid looking frozen.

#77004 should fix the issue at some point, so this PR may be reverted or the approach re-assessed. Some care will be needed anyway in the resource previewing code so that lifespans, etc. are properly handled. Pre-4.3 there's no other fix than this.

Finally, some extra testing would be appreciated.

Fixes #87196.

@RandomShaper RandomShaper added bug topic:rendering topic:editor topic:import cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 15, 2024
@RandomShaper RandomShaper added this to the 4.3 milestone Jan 15, 2024
@YuriSizov
Copy link
Contributor

TilesEditorUtils is a very close copy of the entire EditorResourcePreview class. I assume it needs the same fix?

@RandomShaper
Copy link
Member Author

TilesEditorUtils is a very close copy of the entire EditorResourcePreview class. I assume it needs the same fix?

Good question. I've checked the code and it seems it's fine already. The code there is already safe by using several sync mechanisms to let the main thread deal with the viewports and eventually, only when it's known the texture is ready, claiming the results.

By the way, EditorResourcePreview may want to do something similar, which would still allow some work to happen in parallel on another thread, but such an approach would involve adding the needed "sync points" across both that class and all the preview plugins. I don't think it's very worth the work, when an upcoming PR will fix the underlying issue for good, unless the approach in this PR turned out to be too impractical in practice (he-he, what an expression).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@akien-mga
Copy link
Member

Tested and confirmed this fixes #87196.

@akien-mga akien-mga merged commit 6bb89c7 into godotengine:master Jan 16, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the gl_preview_goodboy branch January 16, 2024 12:35
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling the compatibility renderer leaks Image objects when using the Editor
4 participants