Skip to content

Conversation

@NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Aug 3, 2022

Bandwidth Consumption Fix:
NetworkTransform's NetworkTransformState.Bitset was having bits set but never having bits "un-set" when there was no delta from the previous relative value or the delta had not crossed the threshold. This would result in any axis marked to synchronize previously being always marked to be synchronized which, in turn, would cause all marked axis to be sent anytime a delta in any of the marked axis crossed the threshold value.

On the non-authoritative side, NetworkTransform.ApplyInterpolatedNetworkStateToTransform was using the NetworkTransform.Sync<Position,RotAngle,Scale> (i.e. NetworkTransform.SyncPositionX) as the determining factor as to whether it should apply all associated axis values as opposed to checking the updated NetworkTransformState.Has<Position,RotAngle,Scale> to determine if it should be updated locally.

Example of the Bitset Issue:

  • The authoritative transform detects changes to transform.position.x that exceeds the threshold
    • The transform.position.x bit flag is added to the authoritative state which is then synchronized with all non-authoritative remote instances
  • Some time later, the authoritative transform detects changes to the transform.position.z and transform.localScale.y
    • The transform.position.z and transform.localScale.y bit flags are added to the authoritative state which is then synchronized with all non-authoritative remote instances
      • The state synchronizes the transform.position.x, transform.position.z, and transform.localScale.y values
  • Some time later the authoritative transform only detects a change in transform.position.y that exceeds the threshold.
    • The transform.position.y bit flag is added to the authoritative state which is then synchronized with all non-authoritative remote instances
      • The state synchronizes the entire transform.position and the transform.localScale.y values.

Authoritative Side Interpolation Fix
This includes changes to fix the issue where the authoritative side was interpolating when it should not be.

Client/Owner Authority Adjustments
This includes adjustments to how we handle client-owner authority. Instead of sending a ServerRpc which makes adjustments to a server write authority NetworkVariable, NetworkTransform now creates two versions of the NetworkVariable with one having server write permissions and the other having owner write permissions.

Pertains to #2022 and #2093, BR-694

Changelog

  • Fixed: WIP

Testing and Documentation

  • Includes integration tests. (WIP)

NetworkTransform was setting the value of m_Bitset but not resetting the individual bits if there was no deltas.  This would result in a gradually increasing message size over time when updating the NetworkTransform.
migrating the assignment back down below the CanCommitToTransform
This resolves the issue where clients were applying every value marked to be synchronized even if the update didn't include specific axis values.
This includes some fixes to prefabs that hadn't been updated to the more recent changes in NetworkTransform that limited the minimum threshold (i.e. they were set to 0 which causes issues)
Since we are now no longer sending the entire transform values, this addition prevents the non-authoritative side from making changes to the transform.
removing whitespace
Adding some test adjustments and including rotation to interpolation measurements.
removing an unused namespace.
fixing issue with reference to UnityEngine's Object after I removed the system namespace.
This includes the adjustments for client authoritative mode to work without sending RPCs.
This update fixes the issue where the authoritative side was interpolating when interpolate is enabled.
Now, the authoritative side no longer interpolates.
These changes make the player client authoritative within the Samples Menu->Additive Scene Loading manual test/sample.
@SamuelBellomo SamuelBellomo self-requested a review August 5, 2022 16:08
reverting the change of m_Bitset to an internal.
updating test to account for reverting back to the private m_Bitset.
private readonly NetworkVariable<NetworkTransformState> m_ReplicatedNetworkStateOwner = new NetworkVariable<NetworkTransformState>(new NetworkTransformState(), NetworkVariableReadPermission.Everyone, NetworkVariableWritePermission.Owner);


internal NetworkVariable<NetworkTransformState> GetReplicatedNetworkState()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Consider a getter property instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Getter properties with logic are more expensive than methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, they're identical (and even if they weren't, this would be firmly in the micro-optimization territory). Properties ARE methods, and access to a property getter will be a callvirt, just like access to a method.

    {
        int result = app.MyProp;
        int res2 = app.MyMethod();
    }

    public int MyProp { get { /* ... */ } }

    public int MyMethod() { /* ... */ }
  // [17 9 - 17 33]
    IL_0015: ldloc.0      // app
    IL_0016: callvirt     instance int32 HelloWorld::get_MyProp()
    IL_001b: stloc.1      // result

    // [18 9 - 18 35]
    IL_001c: ldloc.0      // app
    IL_001d: callvirt     instance int32 HelloWorld::MyMethod()
    IL_0022: stloc.2      // res2

  .method public hidebysig specialname instance int32
    get_MyProp() cil managed

  .method public hidebysig instance int32
    MyMethod() cil managed

Perhaps you're thinking of the general guidance that "getters shouldn't be used to hide mutations or heavy calculations"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is what I was thinking of...
Will change this to a getter. 👍

}
else
{
networkState.HasPositionX = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safer (and cleaner to read) to set this unconditionally and then change it only if there is a substantial change?

networkState.HasPositionX = false;
if (syncposx && abovethreshold)
{
  haspositionx = true
}

Would safeguard against someone changing the SyncPositionX from true to false and having HasPositionX be stuck true with an old value? Not sure if that's an actual scenario that can happen - I'm not familiar enough with the code yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From the non-authority side it means that by disabling SyncPositionX all updates to the x axis would cease for that instance. Upon re-enabling SyncPositionX, the HasPositionX value would be re-synchronized (the new state would either have a bitset or not) and will be reset at that time.

For this PR I am going to say that we might just keep it the way it is. PR #2110 has some more clean up and additional fixes that help with the readability.

Migrated the code that updates interpolators with the current value if the axis is not set into the AddInterpolatedState method as opposed to it being in the ApplyInterpolatedNetworkStateToTransform
NoelStephensUnity and others added 3 commits August 9, 2022 09:14
applying suggested changes
replacing the GetReplicatedNetworkState() method with a getter ReplicatedNetworkState property.
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.

4 participants