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

Allow passing UID to importer #97363

Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Sep 23, 2024

This helps, for importers spitting out new resources to the res:// filesystem to actually hash them to generate deterministic UIDs.

This PR also fixes the determinism for translations.

@fire
Copy link
Member

fire commented Sep 24, 2024

Curious, I need to investigate if generates a deterministic uid or merely doesn't repeat because it's from one generator.

Also the problems with duplicate uids.

@reduz
Copy link
Member Author

reduz commented Sep 25, 2024

@fire it should be fine, it uses the generator for the base file, then hashes depending on the generated file, so it pretty much is unique.

@@ -147,6 +147,9 @@ Error ResourceImporterCSVTranslation::import(const String &p_source_file, const
if (r_gen_files) {
r_gen_files->push_back(save_path);
}

ResourceUID::ID save_id = hash64_murmur3_64(translations[i]->get_locale().hash64(), p_source_id);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we put the path to the hash? I'm wondering if there's two identical translation files, what happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

It hashes both the UID of the file and the language, so this should be unique.

Copy link
Member

Choose a reason for hiding this comment

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

One of the biggest features of uid is that you can relocate the files. so hashing the path would remove that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not matter, because it uses the base UID for this, which is generated once and then kept if you move it.
It should continue working fine even if you move the file.

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.

Looks good to me. Needs rebase. Edit: Rebased.

This helps, for importers spitting out new resources to the res://
filesystem to actually hash them to generate deterministic UIDs.

This PR also fixes the determinism for translations.
@akien-mga akien-mga force-pushed the deterministic-gen-suberesources-id branch from c7ae478 to fe34c45 Compare November 11, 2024 14:23
@akien-mga akien-mga requested a review from a team as a code owner November 11, 2024 14:23
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Nov 11, 2024
@Repiteo Repiteo merged commit 2ed6d12 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

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