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

ResourceUID doesn't use z and 9 #83843

Closed
ttencate opened this issue Oct 23, 2023 · 3 comments
Closed

ResourceUID doesn't use z and 9 #83843

ttencate opened this issue Oct 23, 2023 · 3 comments

Comments

@ttencate
Copy link
Contributor

Godot version

c21c270

System information

n/a

Issue description

These lines have an off-by-one error:

https://github.com/godotengine/godot/blob/master/core/io/resource_uid.cpp#L38-L39

No big deal, but it makes encoded RIDs potentially longer than they need to be.

If fixing the bug would cause trouble reading older files, then at least it should be commented.

Steps to reproduce

n/a

Minimal reproduction project

n/a

@Mickeon
Copy link
Contributor

Mickeon commented Oct 24, 2023

This is probably reading too deep into the semantics, but why is this not classified as a bug? It all works fine, but it clearly is not what the author intended. I'm aware it should not be fixed anytime soon, is that why?

@AThousandShips
Copy link
Member

AThousandShips commented Oct 24, 2023

It's not really a bug that something is minorly inefficient, when said behaviour works with no issues, and changing it is destructive, nothing is broken

If also carries importance in the sense of how to approach it

A bug is (and needs to be) fixed, this is not that IMO

Fixing this, even for 5.0, would involve navigating the consequences, and I'm not convinced it's worth it for this minor issue of two missing base values (it will only ever save a single character in the output and only for a very small range of values, in the gaps between some ranges, generally UIDs will be the same length with some exceptions)

Edit: While yes, many, many UIDs (by human standards) will be shorter, but the fact remains that the majority of UIDs will still be 12 or 13 digits long, and (unless my math is completely off) the average length of UIDs will be 12.7 and 12.4 respectively between the current and enhanced code, so you still get roughly the same space efficiency (if one can talk about efficiency for this kind of encoding)

@akien-mga
Copy link
Member

As discussed, changing this would break compatibility for a gain that's minimal aside from fixing a harmless off by one error.

We could consider doing that change for 5.0, but that release isn't planned for the time being, and even then we'd want to ensure we can still handle UIDs from 4.x project that get migrated to 5.0. So it's likely we might not fix this at all.

Next time one of us tweaks ResourceUID, we could add a comment pointing out the mistake, in case we figure out a good way to do this change without breaking projects in the future.

ttencate added a commit to ttencate/godot that referenced this issue Nov 1, 2023
See godotengine#83843

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
orianbsilva pushed a commit to orianbsilva/godot that referenced this issue Nov 1, 2023
See godotengine#83843

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
GuybrushThreepwood-GitHub pushed a commit to GuybrushThreepwood-GitHub/godot that referenced this issue Jan 27, 2024
See godotengine#83843

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants