fix: Parenting does not preserve WorldPositionStays option and other parenting related issues #2146
Conversation
Code clarity. Co-authored-by: Jesse Olmer <[email protected]>
Code clarity. Co-authored-by: Jesse Olmer <[email protected]>
|
Code looks good... will approve once tests are committed. |
Setting WorldPositionStays when sending ParentSyncMessage in OnTransformParentChanged. To better handle late joining players with synchronizing NetworkObjects, added parent child sorting to the SceneEventData's synchronization serialization process. Added world position stays parameter to CreateLocalNetworkObject to preserve worldPostionStays.
Testing with and without WorldPositionStays.
| @@ -1007,7 +983,6 @@ public override void OnNetworkSpawn() | |||
| { | |||
| m_CachedIsServer = IsServer; | |||
| m_CachedNetworkManager = NetworkManager; | |||
| m_TickFrequency = 1.0 / NetworkManager.NetworkConfig.TickRate; | |||
There was a problem hiding this comment.
💯 nice unused variable
com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ParentSyncMessage.cs
Outdated
Show resolved
Hide resolved
JesseOlmer
dismissed
their stale review
No time to review - don't block on me.
| return true; | ||
| var parentNetworkObject = transform.parent.GetComponent<NetworkObject>(); | ||
|
|
||
| // If the parent is a GameObject then preserve that hierarchy |
There was a problem hiding this comment.
Nit... Comment says "if the parent is a GameObject", but the check is actually checking if it's a NetworkObject
There was a problem hiding this comment.
Ahh... yeah I need to improve the clarity here...
If the parent is just a GameObject (without a NetworkObject...which would mean parentNetworkObject is null)... but it could be rephrased to be less confusing indeed.
| if (!reader.TryBeginRead(readSize)) | ||
| { | ||
| throw new OverflowException("Could not deserialize SceneObject: Out of buffer space."); | ||
| } | ||
|
|
||
| if (Header.HasParent) | ||
| if (IsLatestParentSet) |
There was a problem hiding this comment.
Not entirely related to this change, but it's just occurred to me that we could make these ulongs into longs and effectively use the sign bit to encode whether they're set or not... read one byte, if it's negative it's unset, if it's positive then seek back one byte and read it as ulong. That is, assuming the actual value of this doesn't ever need that bit...
Could be a useful function in BytePacker... WriteOptionalValue() that writes 8 bytes with a 0 leading bit if set, or 1 byte with a 1 leading bit if unset.
Curious what you'd think of that (not an action item for this PR, just a thought)
There was a problem hiding this comment.
Actually it also occurs to me that there are several boolean values here...
What about writing them as a bitmask using writer.EnterBitwiseContext()?
There was a problem hiding this comment.
I think we should have a separate PR where we go through all INetworkMessage implementations and see if we can reduce the total size for each message type. Perhaps not in this PR?
| if (IsLatestParentSet) | ||
| { | ||
| writer.WriteValueSafe((ulong)LatestParent); | ||
| BytePacker.WriteValuePacked(writer, (ulong)LatestParent); |
There was a problem hiding this comment.
ooh, fancy.
...but might I suggest looking at BytePacker.WriteValueBitPacked() and see if it's viable? It may not be. It has better compression than WriteValuePacked() but for a ulong it does limit the allowed value to only 61 bits, as it uses the first 3 bits to encode a byte count. (As opposed to WriteValuePacked(), which uses a whole byte to encode the byte count on top of the actual value itself, and if all bytes of the value are needed, it actually makes the encoded value larger than the original instead of smaller)
There was a problem hiding this comment.
(In contrast, WriteValueBitPacked() guarantees it will never increase the size of the value, but does have a limitation of only 61 bits... also there's a typo in the comment that says it's limited to 31 bits, but it's not, it's 61.)
NoelStephensUnity
deleted the
fix/parenting-does-not-preserve-worldpositionstays-option
branch
…parenting related issues (Unity-Technologies#2146) * fix Parenting: Fixed the issue where WorldPositionStays was not being synchronized with clients. Removed m_IsReparented and associated properties. Added world position stays parameter to CreateLocalNetworkObject to preserve worldPostionStays. This fixes some edge case scenarios and issues with parenting in-scene placed NetworkObjects. One in particular is to provide the local space position and rotation in ParentSyncMessage if the WorldPositionStays value is false. The server might maintain the original local space values and upon being reparented it re-applies the local space values. This is only pertinent if the user decides they don't want to use a NetworkTransform for the NetworkObject in question. Now, a users can control what the final position, rotation, and scale of a NetworkObject is when being added/removed to/from a parent's sibling list ("parented or de-parented"). Parenting and Scale: Fixed issue where GetMessageSceneObject wasn't using the NetworkObject's lossy scale value. This would result in improper scaling of parented (WorldPositionStays = true) child NetworkObjects for late joining clients. Added comments with details on this. Fixed edge case scenario where nested in-scene placed NetworkObject children are de-parented and then a client late joins. Under this scenario (only for in-scene placed NetworkObjects) we want the late joining client to remove its parent, then set the transform values, and then get spawned. NetworkTransform: Fixed issue where TryCommitTransformToServer was not honoring the NetworkTransform.InLocalSpace property. Fixed issue where teleporting did not honor the NetworkTransforms's current synchronize axis settings. Removed the private m_TickFrequency property as it is no longer being used. * refactor CreateLocalNetworkObject now just accepts the NetworkObject.SceneObject structure as opposed to the 10 properties of the NetworkObject.SceneObject structure passed in as parameters. Provided the ability to invoke NetworkObject.TrySetParent with a null value in order to be able to remove a parent while also selecting whether they want WorldPositionStay to be true or false (when removing a parent WorldPositionStays impacts the final transform values and can lead to scale issue if you parent with WorldPositionStays set to false and then it remove the parent with WorldPositionStays set to true). Consolidated the CreateLocalNetworkObject's assignment of the transform values, parenting, and DDOL migration. Removed parenting from CreateLocalNetworkObject Removed setting the cached parent directly within SpawnNetworkObjectLocallyCommon as this is done within NetworkObject.ApplyNetworkParenting. * add Added RemoveParent to the ParentSync message. Added applying scale to CreateLocalNetworkObject. Added parent child sorting to the SceneEventData's synchronization serialization process. Added the TryRemoveParent helper method to make removing a parent easier. Without this method users will have to use TrySetParent and cast the null value to either GameObject or NetworkObject. * test Added ParentingWorldPositionStaysTest: Testing with and without WorldPositionStays while also varying child position, rotation, and scale values. Validates scale with multi-generation nested children where most of the parents have a scale other than Vector3.one. Created abstract IntegrationTestWithApproximation to avoid replicating the exact same set of Approximately methods. Added ParentingInSceneObjectsTests: This includes all of the scripts and assets that the ParentingInSceneObjectsTests needs. It includes adding the ParentingInSceneObjects scene to the scenes in build list as well. This also verifies an in-scene placed NetworkObject parented under a GameObject will preserve that hierarchy. NetworkTransformStateTest: Updated the bool parameters to be more meaningful named enums so you know what exactly is being tested. Added additional NetworkTansformState.IsTeleportingNextFrame logic to the existing test. Added an additional "step" which verifies the NetworkTransformState is additively collapsing for each detected axial delta over the relative threshold value. Co-authored-by: Jesse Olmer <[email protected]> Co-authored-by: Unity Netcode CI <[email protected]>
When parenting and setting the WorldPositionStays to false on the server, clients would not apply the same WorldPositionStays setting because it was not being synchronized. This PR also provides parent child serialization ordering during the initial client synchronization that happens when the connection is approved.
MTT-4616
For Visual Validation via Manual Testing:
Use the companion test project PR #2207
Changelog
Added: Position, rotation, and scale to the
ParentSyncMessagewhich provides users the ability to specify the final values on the server-side whenOnNetworkObjectParentChangedis invoked just before the message is created (when theTransformvalues are applied to the message).Added:
NetworkObject.TryRemoveParentmethod for convenience purposes opposed to having to cast null to eitherGameObjectorNetworkObject.Fixed: Issue where the
WorldPositionStaysparenting parameter was not being synchronized with clients.Fixed: Issue where parented in-scene placed
NetworkObjects would fail for late joining clients.Fixed: Issue where scale was not being synchronized which caused issues with nested parenting and scale when
WorldPositionStayswas true.Fixed: Issue with
NetworkTransform.ApplyTransformToNetworkStateWithInfowhere it was not honoring axis sync settings whenNetworkTransformState.IsTeleportingNextFramewas true.Fixed: issue with
NetworkTransform.TryCommitTransformToServerwhere it was not honoring theInLocalSpacesetting.Testing and Documentation
WorldPositionStays.NetworkObjects.NetworkObjectnested under a GameObject with no NetworkObject