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

bug in MapSchema encode? #137

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

amir-arad
Copy link
Contributor

re-encoding a MapSchema with primitive values fails. added a failing test but could not grasp the changeTree mechanism enough to fix it.

@amir-arad amir-arad force-pushed the mapschema-encode-primitive-bug branch 2 times, most recently from ef02876 to 55c7403 Compare July 25, 2022 07:44

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
re-encoding MapSchema with primitive values
@amir-arad amir-arad force-pushed the mapschema-encode-primitive-bug branch from 55c7403 to eb1add9 Compare July 25, 2022 07:46
@amir-arad
Copy link
Contributor Author

This workaround works for me (FYI): cloning the inflated object before encoding,
const secondEncoded = decodedState1.clone().encodeAll();

@amir-arad amir-arad marked this pull request as ready for review July 25, 2022 07:49
@amir-arad
Copy link
Contributor Author

amir-arad commented Jul 25, 2022

@endel give me a clue here? how to fix?

@endel
Copy link
Member

endel commented Jul 25, 2022

Hi @amir-arad, thanks for the reproduction case, this sounds the same as #115.

The use-case of re-encoding a decoded structure was not planned initially. Inside the $changes structure there is an allChanges property that contains the indexes of fields that should be encoded during .encodeAll().

When decoding a MapSchema (and every other "collection"), the allChanges property is not getting populated

allChanges = new Set<number>();

I think ideally we should have a "encoding" and "decoding" mode to avoid having unnecessary extra overhead in the client-side (such as the allChanges = new Set() above). Current architecture is a bit messy and can't easily support this.

If we can support this without adding too much extra overhead it would be nice though!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
update ChangeTree.allChanges in Mapschema when decoding
@amir-arad
Copy link
Contributor Author

@endel I can't pretend to understand the mechanism yet, but I hope this is an appropriate fix.
taking on bigger changes ("encoding" and "decoding" modes) is a bit too much for me ATM.

BTW: my first and most naïve fix attempt broke the filters test, and when debugging that I got a glimpse of that feature. Very impressive! Simplifying the architecture is a challenge, no doubt.

lpsandaruwan added a commit to lpsandaruwan/benchmark-colyseus-schema-reencode that referenced this pull request Aug 15, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@endel endel merged commit 053bf30 into colyseus:master Aug 16, 2022
@amir-arad amir-arad deleted the mapschema-encode-primitive-bug branch August 16, 2022 12:01
@endel
Copy link
Member

endel commented Aug 16, 2022

Thanks for this @amir-arad 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants