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

dcparser: Fix a typo where an unpacked uint64 or int64 might shave off a few bits #751

Closed

Conversation

@TheRandomDog
Copy link

TheRandomDog commented Oct 2, 2019

Just was rummaging through Panda3D source after I encountered an issue in my own project that ended up giving me different numbers after packing and unpacking a large uint64 value. I believe this is the culprit.

@rdb

This comment has been minimized.

Copy link
Member

rdb commented Oct 2, 2019

Are you willing to add a unit test for DCPacker that just packs and unpacks various values for uint64?

@CFSworks

This comment has been minimized.

Copy link
Member

CFSworks commented Dec 24, 2019

@rdb The change itself is definitely correct; should I get the unit test?

@rdb

This comment has been minimized.

Copy link
Member

rdb commented Dec 24, 2019

@CFSworks I'd appreciate your opinion on whether this could cause compatibility issues. Anyone can do the unit test

@CFSworks

This comment has been minimized.

Copy link
Member

CFSworks commented Dec 24, 2019

@rdb Highly unlikely. This only affects integers larger than 56-bit, and as-is the packing is mismatched with the unpacking code - if somebody else had encountered this, we'd have heard about it sooner. The users of this code are also only using it for networked purposes (afaik nobody serializes to nonvolatile storage using DCPacker) so any compatibility issues would be temporary anyway.

The only compatibility issue I could see is if there's a networked game that sends some 64-bit random seed and there are mixed clients running Panda versions before/after this change lands which end up disagreeing on the seed. My stance is that's a case that's unlikely enough, and easily resolved enough, that we can merge this without fear of inconvenience to our users.

@rdb rdb added this to the 1.11.0 milestone Dec 25, 2019
@rdb rdb self-assigned this Dec 25, 2019
@rdb rdb added the bug label Dec 25, 2019
@rdb rdb modified the milestones: 1.11.0, 1.10.5 Dec 25, 2019
rdb added a commit that referenced this pull request Dec 25, 2019
@rdb rdb closed this in cb53e5d Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.