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 errors on file rename or move in the Filesystem Dock #84520

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

Jordyfel
Copy link
Contributor

@Jordyfel Jordyfel commented Nov 6, 2023

Fix for #82652 (comment) (the errors, the crash is still in question)

This happens when at least 2 scenes which depend on a .glb are open, one is the currently edited scene, and another gets reloaded before it in FilesystemDock::_update_dependencies_after_move(). This is the reason scene tab order matters.

When attempting to reload another scene which depends on the same glb as the currently edited scene, scene_tabs->set_current_tab(current_tab);, gets called in EditorNode::reload_scene(). This triggers a code path introduced in this commit, which tries to pack the currently edited scene, but it has an invalid dependency path as it has not been reloaded yet.

This results in several errors related to dependencies failing to load.

A simple enough workaround seems to be to always reload the currently edited scene first.

Test project:
test_proj.zip

Uploaded with .godot so the open scenes and scene tab order are preserved.

Test by moving the .glb in and out of the additional dir multiple times. Worth mentioning that on my PC this resulted in a crash only rarely, other times it's error spam.

Note that there is another issue present - after moving the glb, the scene tab is correct, but the edited scene is wrong, and there is a related error in the console, that should be fixed separately.

@fire
Copy link
Member

fire commented Nov 7, 2023

@SaracenOne if you're interested, here's another scene reloading bug.

editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
@Jordyfel Jordyfel force-pushed the just-another-move-bug branch 3 times, most recently from cef5082 to 0c042f4 Compare November 7, 2023 16:01
@YuriSizov YuriSizov changed the title Fix bug on file move Fix crash on file rename or move in the Filesystem Dock Nov 7, 2023
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 7, 2023
@YuriSizov
Copy link
Contributor

Could you please amend your commit message to be a bit more descriptive? The current title of the PR is a good option.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The code looks fine, assuming it does fix the issue. I can't reproduce the crash unfortunately.

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Nov 7, 2023

Took a look at the original issue, and it's probably not the crash OP reported. The video he uploaded crashed at reimport, which means that the fs scan was already reached, which is past this code in the function.

I originally thought I had fixed it because I was reaching the same error in Rindbee's comment, even though it did not crash for me (except rarely, but I couldn't reproduce). Thought it was a difference between win and linux.

I still think this should go in because it fixes that problem (are linux users experiencing it as a crash?) but the issue should probably be unlinked

@KoBeWi
Copy link
Member

KoBeWi commented Nov 7, 2023

I tried moving various files and folders and I don't even get any errors. This should've been fixed by some recent PRs, so the issue is odd.

@YuriSizov
Copy link
Contributor

but the issue should probably be unlinked

Feel free to edit your OP to remove the link!

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Nov 7, 2023

Done, left a reference to the comment so it's still clear where the issue came up

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Nov 10, 2023

I tried investigating the crash again, with little luck. It only happens around 5% of the time when moving the glb in and out of the dir, seemingly only in debug (though that might be related to whatever is causing the variance behaving differently in prod build, maybe it can still crash there) and has a very cryptic stack trace (the same one as in the linked comment). It seems to not occur after the change, but that by itself doesn't mean there isn't a deeper root cause.

Will update the OP to reflect that this conclusively fixes only the error spam.

@Jordyfel Jordyfel changed the title Fix crash on file rename or move in the Filesystem Dock Fix errors on file rename or move in the Filesystem Dock Nov 10, 2023
@akien-mga akien-mga merged commit 2b913cc into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Jordyfel Jordyfel deleted the just-another-move-bug branch December 5, 2023 01:38
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 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.

6 participants