Skip to content

fix: Added support for byte on NetworkVariable through codegen #2953

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

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

Ufabsther
Copy link
Contributor

@Ufabsther Ufabsther commented Jun 17, 2024

the codegen wasn't picking byte - it's because a variable of a type other than NetworkVariable<byte> (such as NetworkVariable<FixedString32Bytes>) is serializing a byte value as part of its delta serialization. Delta serialization was added in NGO 1.9, which is why it doesn't happen in previous versions. This code assumes the existence of a byte serializer, but if the user doesn't have a NetworkVariable in their code, the byte serializer won't be initialized by the codegen pass.

fix: #2920

Changelog

  • Fixed: Issue where internal delta serialization could not have a byte serializer defined when serializing deltas for other types. Added [GenerateSerializationForType(typeof(byte))] to both the NetworkVariable and AnticipatedNetworkVariable classes to assure a byte serializer is defined.

Testing and Documentation

  • No tests were added or updated.
  • No documentation changes or additions were necessary.

the codegen wasn't picking byte  - it's because a variable of a type other than NetworkVariable<byte> (such as NetworkVariable<FixedString32Bytes>) is serializing a byte value as part of its delta serialization. Delta serialization was added in NGO 1.9, which is why it doesn't happen in previous versions. This code assumes the existence of a byte serializer, but if the user doesn't have a NetworkVariable<byte> in their code, the byte serializer won't be initialized by the codegen pass.
credits: Kitty Drapper
@unity-cla-assistant
Copy link

unity-cla-assistant commented Jun 17, 2024

CLA assistant check
All committers have signed the CLA.

…terparts

Fixed safety issue that would trigger exception on runtime
@Ufabsther
Copy link
Contributor Author

@Unity-Technologies/multiplayer-sdk I'm part of Unity, could you MP a proper way to fix the CLA situation

Using WriteByteSafe as opposed to WriteValueSafe.
adding changelog entry
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Made an adjustment to the NetworkVariableSerialization where I changed the WriteValueSafe to WriteByteSafe, but other than that this looks good.

👍

NoelStephensUnity added a commit that referenced this pull request Jun 25, 2024
Up port of #2953
@NoelStephensUnity NoelStephensUnity merged commit 7915bd4 into develop Jun 25, 2024
24 checks passed
@NoelStephensUnity NoelStephensUnity deleted the 2920-Missing-byte-serializer branch June 25, 2024 17:12
NoelStephensUnity added a commit that referenced this pull request Jun 26, 2024
* chore: updating changelog and package.json (#2952)

* updating changelog and package.json

* adding date

* fix: rotation not being 1.0 issue using rigid body for motion (#2954)

* fix

this fixes the issue where a quaternion could potentially be close to 1.0 but off by a very very very small amount. Under this scenario, we Normalize the quaternion prior to invoking MoveRotation.

* feat: up-port of network variable traits, anticipation, serialized null references, and removal of animator component requirement (#2957)

* update

This contains the updates for PR-2820 (Anticipated NetworkVariable and NetworkTransform) with minor adjustments based on updates in the v2.0.0 branch.

Updating the package version.

Adding updated changelog entries.

up-port of PR-2872

up-port of PR-2874.

updating change log file

* fix

Even if the service specifies to synchronize, if scene management is disabled then do not synchronize.

* fix

Removing some of the logic to determine if session owner should synchronize a connecting client or not.

* fix

Fixing issue with spawning objects within OnInSceneObjectsSpawned and OnNetworkSessionSynchronized primarily on the session owner side.

* update

Reducing the notification down to just synchronization since in-scene notification is called on the spawn sweep.

* test

Adding test for preventing spawning prior to being approved.
Adding tests for when spawning during NetworkBehaviour.OnInSceneObjectsSpawned or NetworkBehaviour.OnNetworkSessionSynchronized or both.

* fix

Up port of #2953

* update

Adding change log entries.

* style

removing white space

* update

reducing GC overhead during deferred despawn

* style

Removing whitespaces

* revert

Reverting deferred spawn update.

---------

Co-authored-by: Frank Luong <[email protected]>
@matus-d-ui42
Copy link

matus-d-ui42 commented Jul 25, 2024

hello @Ufabsther @NoelStephensUnity
im really sorry, but the fix does not work for me after upgrading to 1.10.0 :/

I do have the supposed fix in the code
image

image
also again if i add

NetworkVariableSerialization<byte>.AreEqual = NetworkVariableSerialization<byte>.ValueEquals;

to NetworkVariableSerialization:InitializeIntegerSerialization
it starts working

should I do something extra to trigger the codegen, or is there still something wrong with the code?
thanks! :)

@xSkeletor
Copy link

I also noticed that after upgrading to version 1.10.0 that I still continue to get the same error. But if I add the workaround that was mentioned previously ([GenerateSerializationForType(typeof(byte))]) to one of my scripts, it fixes the error.

@NoelStephensUnity
Copy link
Collaborator

NoelStephensUnity commented Jul 27, 2024

We apologize for any confusion this might have caused.
We are aware that this PR does not resolve the issue and have an alternate approach that will be reviewed early next week.
If it gets a sign off on the different approach, then I will start a patch update to NGO v1.10.1 that will include the alternate fix.

If you are having this issue and using fixed strings, then you could update your manifest with this:
"com.unity.netcode.gameobjects": "https://github.com/Unity-Technologies/com.unity.netcode.gameobjects.git?path=com.unity.netcode.gameobjects#fix/fixedstring-byte-writing-and-comparing",

And report back here if that resolves your issue.

@LightPat
Copy link

LightPat commented Aug 5, 2024

We apologize for any confusion this might have caused. We are aware that this PR does not resolve the issue and have an alternate approach that will be reviewed early next week. If it gets a sign off on the different approach, then I will start a patch update to NGO v1.10.1 that will include the alternate fix.

If you are having this issue and using fixed strings, then you could update your manifest with this: "com.unity.netcode.gameobjects": "https://github.com/Unity-Technologies/com.unity.netcode.gameobjects.git?path=com.unity.netcode.gameobjects#fix/fixedstring-byte-writing-and-comparing",

And report back here if that resolves your issue.

I have tested this very extensively for my game, and we have had no issues with fixedstrings. We use a lot of fixedstrings in INetworkSerializable structs to contain web IDs that don't change after the player connects. We also use some fixedstrings as NetworkVariables to store JSON data of things like dictionaries of game settings. So far, it's behaving just like 1.8.1 had been, no bugs or errors being logged server side or client side. This is with a ping upwards of 200 ms.

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