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

Fix missing dependency warning popup #82244

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

SaracenOne
Copy link
Member

Closes #82243

In older versions of Godot, a popup would appear when a scene was loaded which contained broken dependencies and would give the user an oppertunity to either cancel loading the scene or attempt to fix the dependencies. This seems may have been broken for a while due to changes the ResourceLoader. Since the ResourceLoader is now multithreaded, the callback for broken dependencies now uses a deferred call, but the load_scene function used by the EditorNode only ever requests that loading be done from the main thread. Immediately after the loading is performed, it would check for any dependency_errors list. However, since the callback is now deferred, the updates to the dependency_errors list would not be performed until the end of the frame, skipping the check entirely.

This PR attempts to fix this bug by making it so that if a broken dependency is detected, but the loading is being done on the main thread, it will do a direct callback rather than a deferred call. This fixes the bug and allows the warning popup to be seen again in the event of a broken scene.

godot windows editor dev x86_64_UWiLWOV8SJ

@RandomShaper
Copy link
Member

RandomShaper commented Sep 25, 2023

This PR makes sense, especially given how important a regresion it fixes. I'd like to bring a few points up, though:

  1. The load error notification and dependency error notification mechanisms would no longer have the same thread-wise semantics. Maybe this PR can just apply the same change to the other one.
  2. I'd swear that I did something similar in one of my related PRs and someone raised concerns about the fact that some callbacks happening deferredly while some others don't would lead to problems in the order of errors in the dialog. However, I'm not able to find that discussion and this rationale I remember doesn't seem to be right. Just for the records.
  3. Since this is not exposed (it's editor-only), we can afford to apply this patch and down the road aim for a more robust design. The current idea is that those ResourceLoader static functions are the ones in charge of dealing with cross-thread boundaries to the editor API is still main-thread aware only. That part of the machinery may be still good. However, a different mechanism could be added in the future for the notification part (e.g., collect the errors in a callback with a mutex so they are ready by the end of the load regardless which thread issued them).

@SaracenOne SaracenOne force-pushed the fix_dependency_error_popup branch from d71419b to 0b0a610 Compare September 26, 2023 02:36
@SaracenOne
Copy link
Member Author

I agree with your assessments here. I do get the feeling that a lot of the editor code feels like a legacy system awkwardly wrapping itself around more modern subsystems, a pretty common scenario. A broader overhaul which could make use of callback mechanisms would certainly make would certainly make a lot of sense.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Let's merge (remembering the caveats). I guess this warrants cherry-picking into 4.1, but I'm not sure.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 26, 2023
@SaracenOne
Copy link
Member Author

It might be worth it. My reason for investigating this was a friend mentioning how Godot can easily corrupt your scenes when resources are moved around, and I think any issue related to data corruption is important

@akien-mga akien-mga merged commit 666a92c into godotengine:master Sep 26, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Note: If we cherry-pick this, we should likely include #83024 with it.

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
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.

Dependency warning popup no longer works.
4 participants