feat: pack NetworkTransform into one state variable, prevent sync popping (MTT-818)#964
feat: pack NetworkTransform into one state variable, prevent sync popping (MTT-818)#964
Conversation
| Authority == NetworkAuthority.Shared; | ||
|
|
||
| private void UpdateOneVarPermission<T>(NetworkVariable<T> varToUpdate) | ||
| private bool IsNetworkStateDirty |
There was a problem hiding this comment.
note: implementing threshold values would hopefully be easier with this property's body. we could simply ignore too small changes and count them as "not dirty"
| } | ||
|
|
||
| private Quaternion m_CurrentRotation | ||
| private void UpdateNetworkState() |
There was a problem hiding this comment.
note: implementing specific axis for position/rotation/scale would hopefully be easier with this approach because we could simply switch on what we'll serialize/deserialize here (+ an int mask for what's enabled/disabled for sync in NetworkState would make it even fancier to sync between!)
| } | ||
|
|
||
| private void Awake() | ||
| private void ApplyNetworkState(NetworkState netState) |
There was a problem hiding this comment.
note: interpolation would potentially be implemented under this function in the future
| /// </summary> | ||
| [SerializeField] | ||
| public bool InterpolateServer = true; | ||
| private class NetworkState : INetworkSerializable |
There was a problem hiding this comment.
class is intentional here as we're modifying its props in UpdateNetworkState() instead of swapping m_NetworkState.Value with a brand-new one
There was a problem hiding this comment.
I still think this should be a struct, also just to get rid of the allocations when deserializing this into a new instance. But we can just go ahead with class that's fine.
There was a problem hiding this comment.
Since this won't be used by users, only us internally, I'd be with Luke on that one and avoid the allocation vs having something more friendly.
From what I understand, we're creating a new instance each time we deserialize a type.
There was a problem hiding this comment.
I don't think we create new instances everytime we deserialize a type here. why do you think that's the case @LukeStampfli ?
There was a problem hiding this comment.
Because NetworkVariable ReadDelta is a bit messy.
- It does not read a delta but an absolute value
- It uses
ReadObjectPackedwhich internally creates a new instance ofNetworkState - It also boxes the value an additional time in
ReadObjectPackedwhen returning it, if it is a struct.
https://github.com/Unity-Technologies/com.unity.multiplayer.mlapi/blob/ee59d5fb560bbd6273260c1df144172914b38448/com.unity.multiplayer.mlapi/Runtime/NetworkVariable/NetworkVariable.cs#L187
So it looks like we allocate anyways no matter whether we use a struct or class here 😞
| private void OnNetworkStateChanged(NetworkState oldState, NetworkState newState) | ||
| { | ||
| void SetupVar<T>(NetworkVariable<T> v, T initialValue, ref T oldVal) | ||
| if (!NetworkObject.IsSpawned) |
There was a problem hiding this comment.
This should never happen right? Maybe throw/assert here.
There was a problem hiding this comment.
yeah, I agree it "should never happen" except it does :)
I'm not sure why (haven't digged into this deeply yet), even though we're not setting m_NetworkState or setting any of its values on Awake or anything.
maybe there's something (in Noel's terms) "order of operations" going on behind the scenes I couldn't grok the details yet.
but hey, it's a little innocent early return here :P (and it only happens once as far as I can tell).
I will comment "should never happen but it does" inline too
Co-authored-by: Luke Stampfli <43687322+LukeStampfli@users.noreply.github.com>
| m_CurrentScale != m_OldScale | ||
| ) | ||
| { | ||
| Debug.LogError($"Trying to update transform's position for object {gameObject.name} with ID {NetworkObjectId} when you're not allowed, please validate your {nameof(NetworkTransform)}'s authority settings. It's also possible you have other systems updating your transform's position, such as a rigid body for example.", gameObject); |
There was a problem hiding this comment.
I'd really keep that this error message. It's important to guide users so they don't have silent errors.
There was a problem hiding this comment.
there is no error condition in the new flow. we simply do not care. it's not an error?
There was a problem hiding this comment.
from what I tested with your branch, the transform just doesn't move if it doesn't have the authority. It looks very frustrating. "Why does my code updating that object's transform doesn't do anything??"
There was a problem hiding this comment.
huh, let me try a couple of things there.
|
I really like putting everything in a single state, it simplifies a lot of the |
3 participants
we used to have individual NetVars for UseLocal (now InLocalSpace), position, rotation and scale.
with this PR, we're packing all of them under one single NetworkState variable to prevent individual NetVar updates causing incoherent updates (there popping) between ticks.
this PR also updates tests accordingly (very minor) and fixes issues in GrabbableBall script so that it'd rely on NetworkObject parenting rather than trying to do its own reparenting with RPCs.