-
Notifications
You must be signed in to change notification settings - Fork 450
fix!: NativeList<T>
causes crashes when used in RPCs [NCCBUG-106]
#1901
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!: NativeList<T>
causes crashes when used in RPCs [NCCBUG-106]
#1901
Conversation
…nerated for them.
…without a ForceSerializeByMemcpy wrapper due to the ISerializeByMemcpy tag requirement) - Made sure there's at least one test exercising Unity types going through RPCs... more tests exercising all of these are needed later.
# Conflicts: # com.unity.netcode.gameobjects/Runtime/Messaging/Messages/SnapshotDataMessage.cs # com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs # testproject/Packages/packages-lock.json
com.unity.netcode.gameobjects/Runtime/Serialization/ISerializeByMemcpy.cs
Outdated
Show resolved
Hide resolved
@@ -10,6 +10,7 @@ Additional documentation and release notes are available at [Multiplayer Documen | |||
### Added | |||
|
|||
### Changed | |||
- `unmanaged` structs are no longer universally accepted as RPC parameters because some structs (i.e., structs with pointers in them, such as `NativeList`) can't be supported by the universal struct serializer. Structs that are intended to be serialized across the network must add `ISerializeByMemcpy` to the interface list (i.e., `struct Foo : ISerializeByMemcpy`). This interface is empty and just serves to mark the struct as compatible with universal serialization. For external structs you can't edit, you can pass them to RPCs by wrapping them in `ForceSerializeByMemcpy<T>`. (#1901) |
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.
doesn't this also mean that any existing code relying on a struct being serialized without the interface will immediately break after this change?
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.
Yep. But there's really no way to make NativeList <T>
an error without making other things an error. We can choose to leave this part of it out, in which case we can fix template types causing crashes, but people will still be able to pass in NativeList and it will still crash... the alternative to doing this is documentation saying not to use NativeList in RPCs
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.
or we could add explicit support for NativeList and then that'll work, but NativeArray will crash... we can support all the things but there will always be something users can pass in without realizing it's invalid that will cause a crash.
NativeList<T>
causes crashes when used in RPCs [NCCBUG-106]
com.unity.netcode.gameobjects/Runtime/Serialization/ForceSerializeByMemcpy.cs
Outdated
Show resolved
Hide resolved
…ializers for by looking for instances of NetworkVariable<T> and NetworkList<T>. This was required because runtime reflection doesn't work in AoT consoles; the runtime reflection will now only ever be needed if someone does something particularly odd like, for some reason, making a NetworkVariable or NetworkList that's not a field of a NetworkBehaviour. Made an class to go between NetworkVariable and NetworkVariableBase so that future uses for these serialization functions will also be picked up automatically by ILPP. Also, review feedback: - Renamed ISerializeByMemcpy to INetworkSerializeByMemcpy - Reverted Burst json file - Removed an errant empty line
…_in_RPCs' into fix/NativeList_crashes_when_used_in_RPCs
Co-authored-by: Fatih Mar <mfatihmar@gmail.com>
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.
LGTM!
Other than the standards check issue. 😸
…_in_RPCs' into fix/NativeList_crashes_when_used_in_RPCs
|
||
namespace Unity.Netcode | ||
{ | ||
public struct ForceNetworkSerializeByMemcpy<T> : INetworkSerializeByMemcpy, IEquatable<ForceNetworkSerializeByMemcpy<T>> where T : unmanaged, IEquatable<T> |
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.
we need xmldoc
|
||
namespace Unity.Netcode | ||
{ | ||
public interface INetworkSerializeByMemcpy |
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.
we need xmldoc
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.
we need xmldoc for new public ForceNetworkSerializeByMemcpy
& INetworkSerializeByMemcpy
types
// These structs enable overloading of WriteValue with different generic constraints. | ||
// The compiler's actually able to distinguish between overloads based on generic constraints. | ||
// But at the bytecode level, the constraints aren't included in the method signature. | ||
// By adding a second parameter with a defaulted value, the signatures of each generic are different, | ||
// thus allowing overloads of methods based on the first parameter meeting constraints. |
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.
should this be an xmldoc (aka 3 slashes summary block ///
)?
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.
Didn't intend it to be since it's referring to a whole section and not a specific struct or method.
I wasn't really sure what to do with xmldocs for this section because the methods are one line each and adding a 5 line xmldoc for each one seemed overkill.
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.
gtg 🚀
There were two issues here:
To address this, two things have been done:
int
,bool
,byte
, etc)unmanaged
enumsINetworkSerializable
unmanaged
structs that are tagged asISerializeByMemcpy
(an empty interface that the developer can add to their struct to indicate that it's compatible with memcpy serialization and can be sent over the wire)Vector3
,Color
, etc)FastBufferWriter
andFastBufferReader
have been created to enable them to be serializedFor types that aren't natively supported but aren't in the developer's control to be able to add
ISerializeByMemcpy
to it, there isForceSerializeByMemcpy
, a wrapper that adds the tag while supporting implicit conversion to and from its wrapped type. It has the limitation that, in order to be supported by NativeList, the type it wraps must be bothunmanaged
andIEquatable<T>
.Currently,
FixedString
is not in the list of natively-supported types (due to the number of varying sizes it can have) and must be wrapped inForceSerializeByMemcpy
to be used.Changelog
unmanaged
structs are no longer universally accepted as RPC parameters because some structs (i.e., structs with pointers in them, such asNativeList<T>
) can't be supported by the universal struct serializer. Structs that are intended to be serialized across the network must addISerializeByMemcpy
to the interface list (i.e.,struct Foo : ISerializeByMemcpy
). This interface is empty and just serves to mark the struct as compatible with universal serialization. For external structs you can't edit, you can pass them to RPCs by wrapping them inForceSerializeByMemcpy<T>
.Testing and Documentation