-
Notifications
You must be signed in to change notification settings - Fork 450
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
Conversation
…ll versionf of unity, and it's not used anyway so I just set it to return 0.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
There was a problem hiding this 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!
Unfortunate... this code fails on consoles. I'm going to close this for now and will re-open once I get consoles working again. |
… 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.
Split |
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. |
…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.
Back to only one type. |
…iable' into feat/managed_types_in_networkvariable
|
||
/// <summary> | ||
/// Read a FixedString value. | ||
/// |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a...?
There was a problem hiding this comment.
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...
There was a problem hiding this 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!
There was a problem hiding this 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?)
Description updated. Had already updated changelog, but forgot to change the description. |
// 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>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
com.unity.netcode.gameobjects/Tests/Runtime/Components/NetworkVariableTestComponent.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
…VariableTestComponent.cs
…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>
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:
INetworkSerializable
, orUserNetworkVariableSerialization<T>.WriteValue
andUserNetworkVariableSerialization<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
NetworkVariable<>
now supports managedINetworkSerializable
types, as well as other managed types with serialization/deserialization delegates registered toUserNetworkVariableSerialization<T>.WriteValue
andUserNetworkVariableSerialization<T>.ReadValue
NetworkVariable<>
andBufferSerializer<BufferSerializerReader>
now deserializeINetworkSerializable
types in-place, rather than constructing new ones.Testing and 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 from
NetworkVariable`, 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.