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

Core: Do not generate *.uid files for JSON, certificates, and translations #99540

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev added this to the 4.4 milestone Nov 22, 2024
@dalexeev dalexeev requested a review from a team as a code owner November 22, 2024 13:15
@dalexeev dalexeev force-pushed the core-dont-gen-json-uid-files branch from b205139 to 98398a4 Compare November 22, 2024 13:27
@@ -105,6 +105,9 @@ class ResourceFormatLoaderJSON : public ResourceFormatLoader {
virtual void get_recognized_extensions(List<String> *p_extensions) const override;
virtual bool handles_type(const String &p_type) const override;
virtual String get_resource_type(const String &p_path) const override;
// Treat JSON as a text file, do not generate a `*.json.uid` file.
virtual ResourceUID::ID get_resource_uid(const String &p_path) const override { return ResourceUID::INVALID_ID; }
virtual bool has_custom_uid_support() const override { return true; }
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that both overrides are required. This may seem odd, since in fact JSON does not have a UID. But we define "support" for custom UID... which is invalid. It might make sense to separate these three lines with a blank line for better readability.

ResourceUID::ID ResourceFormatLoader::get_resource_uid(const String &p_path) const {
int64_t uid = ResourceUID::INVALID_ID;
if (has_custom_uid_support()) {
GDVIRTUAL_CALL(_get_resource_uid, p_path, uid);
} else {
Ref<FileAccess> file = FileAccess::open(p_path + ".uid", FileAccess::READ);
if (file.is_valid()) {
uid = ResourceUID::get_singleton()->text_to_id(file->get_line());
}
}
return uid;
}

godot/editor/editor_node.cpp

Lines 1457 to 1465 in f952bfe

void EditorNode::ensure_uid_file(const String &p_new_resource_path) {
if (ResourceLoader::exists(p_new_resource_path) && !ResourceLoader::has_custom_uid_support(p_new_resource_path) && !FileAccess::exists(p_new_resource_path + ".uid")) {
Ref<FileAccess> f = FileAccess::open(p_new_resource_path + ".uid", FileAccess::WRITE);
if (f.is_valid()) {
const ResourceUID::ID id = ResourceUID::get_singleton()->create_id();
f->store_line(ResourceUID::get_singleton()->id_to_text(id));
}
}
}

if (fi->uid == ResourceUID::INVALID_ID && ResourceLoader::exists(path) && !ResourceLoader::has_custom_uid_support(path) && !FileAccess::exists(path + ".uid")) {
// Create a UID.
Ref<FileAccess> f = FileAccess::open(path + ".uid", FileAccess::WRITE);
if (f.is_valid()) {
fi->uid = ResourceUID::get_singleton()->create_id();
f->store_line(ResourceUID::get_singleton()->id_to_text(fi->uid));
}
}

Copy link
Member

Choose a reason for hiding this comment

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

We could make it into a macro called DISABLE_UID or something. These 2 lines will be the same for every class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is redundant for now. But we may reconsider this in the future if more exceptions are added.

@akien-mga akien-mga requested a review from KoBeWi November 22, 2024 13:49
@dalexeev
Copy link
Member Author

I checked all ResourceFormatLoaders. Scripts, shaders, media files and other Godot files (*.gdextension) obviously need to have UIDs. But I found two potential candidates for which it might make sense to disable UIDs:

  • ResourceFormatLoaderCrypto (*.crt, *.key, *.pub);
  • TranslationLoaderPO (*.po, *.mo).

@akien-mga
Copy link
Member

  • ResourceFormatLoaderCrypto (*.crt, *.key, *.pub);
  • TranslationLoaderPO (*.po, *.mo).

Yeah I think disabling .uid files for those would make sense.

We can always re-assess if some users report issues due to not having UIDs for those or .json, but I think it's better to start conservative (less intrusive to user projects) and adjust later.

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.

Makes sense to me.

@dalexeev dalexeev force-pushed the core-dont-gen-json-uid-files branch from 98398a4 to eb216c6 Compare December 4, 2024 11:25
@dalexeev dalexeev requested a review from a team as a code owner December 4, 2024 11:25
@dalexeev dalexeev force-pushed the core-dont-gen-json-uid-files branch from eb216c6 to b91bacb Compare December 4, 2024 11:26
@dalexeev dalexeev changed the title Core: Do not generate *.json.uid files Core: Do not generate *.uid files for JSON, certificates, and translations Dec 4, 2024
@Repiteo Repiteo merged commit 451fd7a into godotengine:master Dec 4, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 4, 2024

Thanks!

@dalexeev dalexeev deleted the core-dont-gen-json-uid-files branch December 4, 2024 17:49
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.

JSON files produce *.uid files
4 participants