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

Create .uid files in EditorFileSystem #101650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 16, 2025

Fixes #101591

Turns out the ensure_uid_file() method I once added was wrong, because EditorFileSystem has its own uid cache. The bug occured, because the new UID was not registered in the EditorFileSystem, which is used by ResourceSaver to determine UIDs when saving.
I fixed the bug for scripts and shaders, but I see more places where FileInfo is created. There might be some edge case where either UID is not registered or .uid file is not created, idk.

@KoBeWi KoBeWi added this to the 4.4 milestone Jan 16, 2025
@KoBeWi KoBeWi requested review from a team as code owners January 16, 2025 20:51
@@ -1281,6 +1281,7 @@ void EditorFileSystem::_process_file_system(const ScannedDirectory *p_scan_dir,
if (f.is_valid()) {
if (fi->uid == ResourceUID::INVALID_ID) {
fi->uid = ResourceUID::get_singleton()->create_id();
ResourceUID::get_singleton()->add_id(fi->uid, path);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is another problem I noticed. The UID created here was not assigned to the path.

Copy link
Contributor

@Hilderin Hilderin Jan 18, 2025

Choose a reason for hiding this comment

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

Not sure you should call ResourceUID::get_singleton()->add_id(id, file); because it will be called again at line 1305. If the id is added here, it will be regenerated just after because the if (ResourceUID::get_singleton()->has_id(fi->uid)) { at line 1294 will see it as a duplicate.

Not sure what this added line is suppose to fix, the ResourceUID::get_singleton()->add_id(fi->uid, path); just below should have been suffisiant?

Comment on lines +2372 to +2381
} else {
if (ResourceLoader::exists(file) && !ResourceLoader::has_custom_uid_support(file) && !FileAccess::exists(file + ".uid")) {
Ref<FileAccess> f = FileAccess::open(file + ".uid", FileAccess::WRITE);
if (f.is_valid()) {
const ResourceUID::ID id = ResourceUID::get_singleton()->create_id();
ResourceUID::get_singleton()->add_id(id, file);
f->store_line(ResourceUID::get_singleton()->id_to_text(id));
fi->uid = id;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The actual fix is here. The changes around are because of the fi variable.

@Repiteo

This comment was marked as resolved.

@KoBeWi KoBeWi force-pushed the duality_of_uid_caches branch from 339ddb9 to 948bff0 Compare January 17, 2025 00:45
@geowarin
Copy link
Contributor

It seems to solve the original problem. 👍

However (and I don't know if it's in the scope of this PR), when a script is moved, its path in the scene does not change.

image

Which mean that if the original resource moved, the button "open script" will open a new file.

The path never changes, even after scene reload or editor restart.
I think it's ok if this is treated at a later date but I would have expected the path to change at some point.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 17, 2025

@geowarin I can't reproduce this problem, both with and without this PR, so it's likely unrelated. Open a new issue.

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.

Attaching a script to a scene does not use its uid
4 participants