Add int/float widening from 32 to 64 bits for rbx_binary #305
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change allows us to accept a Int32 even when the canonical type is Int64, and automatically migrate it over. This would fix rojo-rbx#301.
This change allows us to accept a Float32 even when we expect a Float64, seamlessly migrating it over. See rojo-rbx#301.
Dekkonot
approved these changes
Jun 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR supports widening ints & floats from 32 bits to 64 bits.
Basically, if we deserialize a property we expect to be 64 bits and it's actually 32 bits, we can safely convert over.
This fixes issues where Roblox previously had values serialized as 32 bits, but then they got upgraded to 64 bits later on. This only really occurs in older files, but it's a good blanket fix to have regardless.
There might be a better solution with migrations but I'm not as familiar with that; we can always remove this change if that gets figured out. I don't think this will break anything, unless Roblox goes from 64 back to 32 for a value and rbx_dom doesn't update in time, though that doesn't seem very likely. It's more likely that this will automatically migrate a value if Roblox changes something from 32 bits to 64.
This fixes #301, and rbx_xml's fix is tracked in #303.
Let me know if there's anything else I need to change! I wanted to make a test for this, but I couldn't figure out how to get a binary file in the wrong format other than my repo file, and it looks like the file resolutions paths are broken on my cursed Cygwin setup, so I couldn't run any tests. Hopefully this is good!