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 file disappearing when renaming dependencies #86177

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Dec 14, 2023

Fixes #86174

When renaming a file, the editor will try to fix dependencies, but it will save affected scenes before that. However as a result of saving, the dependency might get removed thus a rename is performed on a dependency that no longer exists.

Then there is rename method that creates a .depren file after renaming dependency. However it treats "no rename" as success

//nothing was done
if (fw.is_null()) {
return OK;
}

meaning the file is not created, but the code continues normally. This results in a disaster.

The fix checks whether .depren file actually exists before removing the old file. Another option would be making the rename method return an error if the dependency can't be found, but I'm not sure if it's always expected to exist (in case of the fixed issue it didn't, but maybe it's expected to fail, idk).
I modified the binary format too, but I didn't test if the same bug occurs. Better to be safe anyway.

This was probably regression from #81725 or #81657

@KoBeWi KoBeWi added this to the 4.3 milestone Dec 14, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner December 14, 2023 21:38
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

@YuriSizov This should be a patch for 4.2.2

@adamscott adamscott added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 14, 2023
@Jordyfel
Copy link
Contributor

Jordyfel commented Dec 15, 2023

Since saving scenes can change dependencies, what do you think about swapping these calls:
Impossible, my bad.

// Look for files that use these moved/renamed resource files.
_find_file_owners(EditorFileSystem::get_singleton()->get_filesystem(), renamed_files, r_file_owners);
// Open scenes with dependencies on the ones about to be moved will be reloaded,
// so save them first to prevent losing unsaved changes.
EditorNode::get_singleton()->save_scene_list(r_file_owners);

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 15, 2023

save_scene_list() uses r_file_owners(), which is filled by _find_file_owners(), so they can't be swapped. Maybe an alternative is simply saving every scene, but changing behavior would require more testing. The current solution is a safer option.

@YuriSizov YuriSizov merged commit df9be54 into godotengine:master Dec 16, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@KoBeWi KoBeWi deleted the depren_never_forgets branch December 16, 2023 17:00
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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.

Renaming a .gd file deletes the previously linked .tscn
4 participants