Skip to content

feat: Supporting managed types in NetworkVariable [MTT-4594] #2219

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 17 commits into from
Oct 6, 2022

Conversation

ShadauxCat
Copy link
Collaborator

@ShadauxCat ShadauxCat commented Sep 26, 2022

Also changes NetworkVariable to deserialize INetworkSerializable types in-place - this is done to avoid having a GC alloc every time the value changes. There will only be a GC alloc if it receives an updated value and the current value is null.

Managed types are supported in two ways:

  1. By making them INetworkSerializable, or
  2. By assigning the delegates UserNetworkVariableSerialization<T>.WriteValue and UserNetworkVariableSerialization<T>.ReadValue

If a managed type is passed in without meeting one of those criteria (or any unsupported type), then an exception will be thrown the first time the NetworkVariable is read or written. That timing is to ensure no weird behavior with exceptions being thrown before user code is able to assign the delegates.

Changelog

  • Changed: NetworkVariable<> now supports managed INetworkSerializable types, as well as other managed types with serialization/deserialization delegates registered to UserNetworkVariableSerialization<T>.WriteValue and UserNetworkVariableSerialization<T>.ReadValue
  • Changed: NetworkVariable<> and BufferSerializer<BufferSerializerReader> now deserialize INetworkSerializable types in-place, rather than constructing new ones.

Testing and Documentation

  • Includes integration tests.
  • Includes edits to existing public API documentation.

Generated ILPP output:

The ILPP generates two function calls for each type it finds being used in a NetworkVariable - one to set a serializer for it, one to set an equality operator for it. The methods being called are all found in NetworkVariableSerialization.cs (and all added in this review so they're easy to see there). The reason ILPP is used is because the methods being called for various types have constraints (for example, FastBufferWriter.WriteUnmanagedSafe" has an unmanagedconstraint). Since that constraint was removed fromNetworkVariable`, it's no longer possible to call that method, nor even to initialize the serializer as there's no way to cast a type to say "this type is definitely unmanaged, compiler" in order to create an instance of that class, and using reflection to create it doesn't work in IL2CPP... but initializing a delegate based on that type does work IF the type is known at compile time in the location it's being initialized. ILPP is used to call those initialization functions to save our users the headache of having to remember to initialize all their network variable types - without ILPP, they'd have to call all the initialization manually.

In the future, Code Generators will offer better solutions for this.

image

@ShadauxCat ShadauxCat requested a review from a team as a code owner September 26, 2022 22:04
@@ -16,6 +16,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
### Changed

- The debug simulator in `UnityTransport` is now non-deterministic. Its random number generator used to be seeded with a constant value, leading to the same pattern of packet drops, delays, and jitter in every run. (#2196)
- `NetworkVariable<>` now supports managed INetworkSerializable types, as well as other managed types with serialization/deserialization delegates registered to `UserNetworkVariableSerialization<T>.WriteValue` and `UserNetworkVariableSerialization<T>.ReadValue` (#2219)
- `NetworkVariable<>` and `BufferSerializer<BufferSerializerReader>` now deserialize `INetworkSerializable` types in-place, rather than constructing new ones. This makes using native collections (such as `NativeArray<>`) easier as they can be reused once they're constructed rather than having to construct new ones and free old ones on every update, and prevents unnecessary GC allocations with managed types. (#2219)
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

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.

Minor nit with changelog.
Everything else LGTM!

@ShadauxCat
Copy link
Collaborator Author

Unfortunate... this code fails on consoles. I'm going to close this for now and will re-open once I get consoles working again.

@ShadauxCat ShadauxCat closed this Sep 27, 2022
… because C# generics just aren't flexible to do it in one class without reflection, and I couldn't find a way to make the reflection work with il2cpp.
@ShadauxCat ShadauxCat reopened this Sep 28, 2022
@ShadauxCat
Copy link
Collaborator Author

ShadauxCat commented Sep 28, 2022

Split NetworkVariable into NetworkVariable and ManagedNetworkVariable because C# generics just aren't flexible to do it in one class without reflection, and I couldn't find a way to make the reflection work with il2cpp.

@ShadauxCat
Copy link
Collaborator Author

Jesse has asked me to spend some more time trying to find a way around needing a separate class. I'm closing this again because it shouldn't be reviewed until it's finalized.

@ShadauxCat ShadauxCat closed this Sep 29, 2022
…d went back to an ILPP-based approach.

Limitation to this approach is that only NetworkVariables which are fields of NetworkBehaviours will get initialized... but as we don't really support any other usage of NetworkVariables, this seems like an acceptable limitation.

Also upgraded the project to 2020.3.40, as older versions fail to compile some of the new tests due to IL2CPP bugs.
@ShadauxCat ShadauxCat reopened this Sep 30, 2022
@ShadauxCat
Copy link
Collaborator Author

Back to only one type.

@ShadauxCat ShadauxCat requested a review from 0xFA11 September 30, 2022 21:01

/// <summary>
/// Read a FixedString value.
///
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity Oct 1, 2022

Choose a reason for hiding this comment

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

nit-pick?
good place for a <remarks></remarks> as opposed to the black comment line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For a...?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, forgot to mark the remarks as code...

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.

Would definitely get @ThusSpokeNomad to look over this as well.
Minor nit on possibly using remarks for a public API method.
Just commented for verification on the ProjectVersion and associated json files that were changed.
Other than that it LGTM!

Copy link
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

Haven't had the full code-review yet but I noticed the description is out-of-date:

Changed: Added a new ManagedNetworkVariable<> with support for managed INetworkSerializable types

Do you mind updating the description to reflect up-to-date changes? (Also perhaps CHANGELOG.md too?)

@ShadauxCat
Copy link
Collaborator Author

Description updated. Had already updated changelog, but forgot to change the description.

Comment on lines +322 to +323
// CodeGen cannot reference the collections assembly to do a typeof() on it due to a bug that causes that to crash.
private const string k_INativeListBool_FullName = "Unity.Collections.INativeList`1<System.Byte>";
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Copy link
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

Left a few nits but overall it's looking good.

@ShadauxCat ShadauxCat enabled auto-merge (squash) October 6, 2022 15:10
@ShadauxCat ShadauxCat merged commit 26577d6 into develop Oct 6, 2022
@ShadauxCat ShadauxCat deleted the feat/managed_types_in_networkvariable branch October 6, 2022 16:08
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
…echnologies#2219)

* feat: Supporting managed types in NetworkVariable.

Also changes NetworkVariable to deserialize INetworkSerializable types in-place - this is done to avoid having a GC alloc every time the value changes. There will only be a GC alloc if it receives an updated value and the current value is null.

Managed types are supported in two ways:

1. By making them INetworkSerializable, or
2. By assigning the delegates UserNetworkVariableSerialization<T>.WriteValue and UserNetworkVariableSerialization<T>.ReadValue

If a managed type is passed in without meeting one of those criteria (or any unsupported type), then an exception will be thrown the first time the NetworkVariable is read or written. That timing is to ensure no weird behavior with exceptions being thrown before user code is able to assign the delegates.

Co-authored-by: Fatih Mar <mfatihmar@gmail.com>
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.

3 participants