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

Implement network serializer (rebased) #1293

Merged

Conversation

HifiExperiments
Copy link
Member

#81 rebased + with one final bug fix

daleglass and others added 19 commits January 11, 2025 18:41
Avoids warning due to glm::vec3 not being trivially copyable
Single class wasn't working well because deserialization may
need to be done on const data. With the split, the deserializer
part can work with const data without issues.

Also cleaned things up a bit.
* Stop trying to be compatible with the old format, and just bump the version number.
* Add uint64_t support to serializer
* A bit improved debug output from serializer
* Add lastAdvance() function to ask the serializer how much data was added/read in the last operation.
This fixes compatibility with older baked assets
@HifiExperiments HifiExperiments added needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested labels Jan 12, 2025
This was referenced Jan 12, 2025
Copy link
Contributor

@daleglass daleglass left a comment

Choose a reason for hiding this comment

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

Bloody hell, how did I miss that one?

I spent a few hours staring at a dump but somehow missed the completely obvious problem.

@HifiExperiments
Copy link
Member Author

sorry I hadn’t tried building the branch myself earlier! if I’d known there was still a bug I would have taken a closer look at things sooner

@ksuprynowicz
Copy link
Member

Awesome!
I just tested it and everything works well now :)

@ksuprynowicz ksuprynowicz added QA approved This pull request has been successfully tested and removed needs QA This pull request needs to be tested labels Jan 13, 2025
Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

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

I think all looks good

@ksuprynowicz ksuprynowicz removed the needs CR This pull request needs to be code reviewed label Jan 13, 2025
@ksuprynowicz ksuprynowicz added the CR approved This pull request has been successfully code reviewed label Jan 13, 2025
@HifiExperiments HifiExperiments merged commit a7805e7 into overte-org:master Jan 14, 2025
8 checks passed
@HifiExperiments HifiExperiments deleted the implement-serializer branch January 18, 2025 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR approved This pull request has been successfully code reviewed QA approved This pull request has been successfully tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants