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

fix: Add OP_RENEW to SyncDictionary #3614

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Moddingdudes
Copy link

When all SyncDictionary values are set simultaneously, this treats the serialization/deserialization as "all" instead of "delta" which means Callback will not be invoked, so you will miss data changes from the host.

When all SyncDictionary values are set simultaneously, this treats the serialization/deserialization as "all" instead of "delta" which means Callback will not be invoked, so you will miss data changes from the host.
@miwarnec
Copy link
Collaborator

interesting. @Moddingdudes any way we can reproduce the bug?
then we can add a unit test if you let us know the initial issue

@Moddingdudes
Copy link
Author

Yes. Create a SyncDictionary with 2 keys and values. Add a callback to the SyncDictionary and have it log something. Iterate over all of the SyncDictionary keys and set their values to something new to trigger the Set overload of the SyncDictionary. The SyncDictionary seems to treat this as a new snapshot instead of a delta and calls OnDeserializeAll instead of OnDeserializeDelta, which means Callback is not fired. That's why I added an AddOperation function call in OnDeserializeAll with a new OP_RENEW addition to the Enum. In my testing, this worked well.

@miwarnec
Copy link
Collaborator

thx will take a look soon ish!

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.

2 participants