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

CSV import: Generate positive UID for .translation and follow renames #103120

Merged
merged 1 commit into from
Feb 23, 2025

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Feb 21, 2025

Fixes #103118

Mask with INT64_MAX to avoid negative UIDs which cause bugs.
If the generated .translation UID is already in use (renamed), overwrite that file instead.

@lyuma lyuma added this to the 4.4 milestone Feb 21, 2025
@lyuma lyuma requested a review from a team as a code owner February 21, 2025 09:26
@akien-mga akien-mga changed the title csv import: generate positve UID for .translation and follow renames CSV import: Generate positve UID for .translation and follow renames Feb 21, 2025
@akien-mga akien-mga requested a review from KoBeWi February 21, 2025 10:46
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.

Looks correct, but the comments above should be addressed.

@lyuma lyuma force-pushed the fix_translation_uid_hash branch from 7131b52 to bb5bf26 Compare February 21, 2025 19:33
@lyuma
Copy link
Contributor Author

lyuma commented Feb 21, 2025

Added an if to avoid a redundant set_uid. The way I'm resolving the conflict here is to prefer saving to whatever path has the matching uid, rather than generating a new uid. I think the usecase is users might move their generated .translation files to a folder, and that should be okay now since the UID for a given .translation is deterministic.

Re: real collision, we're talking 1 in 2^64 per file. I think the probability is something like if every human on earth made a few Godot projects for the next 100 years, you might get one collision ever. My opinion is this is not worth worrying about, since the consequence of a UID collision in the worst case is also not too bad. Also, this risk affects all code using UIDs, not just translation.

Rather than add a special case for 1 in 2^64 collision, we should change ResourceUID or ResourceSaver to emit an error if a collision is detected or uid:// or any other type of UID mismatch happens, which would have caught this bug much sooner.

Mask with INT64_MAX to avoid negative UIDs which cause bugs.
If the generated .translation UID is already in use (renamed), overwrite that file instead.
@lyuma lyuma force-pushed the fix_translation_uid_hash branch from bb5bf26 to f2ad430 Compare February 22, 2025 09:08
@akien-mga akien-mga merged commit b99a971 into godotengine:master Feb 23, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@MarianoGnu
Copy link
Contributor

oh, .translation files generates .uid files? these files are tipically ignored in git because they are autogenerated from csv 🤔 but makes total sense to ensure the uid remains unique across different devices never tought of it👍

@akien-mga akien-mga changed the title CSV import: Generate positve UID for .translation and follow renames CSV import: Generate positive UID for .translation and follow renames Mar 2, 2025
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.

Translation CSV importer produces invalid or duplicate UIDs
5 participants