fix: NetworkTransform rotation delta check false positives #1890
Conversation
NoelStephensUnity
changed the title
…ttps://github.com/Unity-Technologies/com.unity.netcode.gameobjects into fix/mtt-3370-networktransform-delta-rotation-check
| public const float ThresholdMinimum = 0.001f; | ||
| public const float PositionThresholdDefault = 0.001f; | ||
| public const float RotAngleThresholdDefault = 0.01f; | ||
| public const float ScaleThresholdDefault = 0.01f; |
There was a problem hiding this comment.
I was wondering about using Mathf.Epsilon for these... any thoughts?
There was a problem hiding this comment.
I think that my original thought on that is it could encourage a flawed design for detecting a change in a value to update over a network. Setting it to something like the Epsilon value ( 1.401298E-45) is such a small value that it is about the same as setting it to zero which can lead to scenarios where you are sending changes close to every frame due to minor jitter in things like rotation and/or moving along a slope with gravity applied.
Of course....I can think of scenarios where someone uses something like 0.001 as their game's 1 distance unit which would make detecting changes to that difficult...so setting the "minimum threshold" to a very low value could result in sending updates more frequently than desired (by default) but setting it too high could cause issues where the users world scale is not the default unity world scale (i.e. 1.0 tends to be the default in Unity).
(shrug)
I can see pro's and con's both ways. I picked 1/1000th of a "default world space unit" and 1/100th of a Euler degree for rotation....but maybe I just remove the restriction on scale and position values and delete ThresholdMinimum?
We want to keep the rotation thresholds as those make sense no matter what the user's world scale.
There was a problem hiding this comment.
I think 0.001f as defaults is fine but making 0.001f the threshold minimum might be a little limiting. If I disable gravity or make my rigidbody kinematic, it wouldn't move at all, not even epsilon and that shouldn't cause a re-replication since oldValue - newValue > epsilon would be false, right?
also I feel like we could have tests just testing these ranges too.
so if I were to sum it up, I think we should keep sensible defaults as 0.01f & 0.001f but make epsilon the min value for the ranges.
Making the StopStartRuntimeTests derive from the NetcodeIntegrationTest to avoid the scenario where a specific platform/desktop is running slow for some reason and it take longer than the default NetcodeIntegrationTestHelpers.WaitForClientsConnected's 1 second. Since it now derives from NetcodeIntegrationTest it will also handle cleaning up everything properly in the event the clients truly cannot connect. and not leave behind artifacts in the test runner scene and/or DDOL that would cause all tests that proceeded StopStartRuntimeTests to fail as well.
| /// </summary> | ||
| /// <param name="transform">transform to apply</param> | ||
| /// <returns>bool isDirty, bool isPositionDirty, bool isRotationDirty, bool isScaleDirty</returns> | ||
| internal (bool isDirty, bool isPositionDirty, bool isRotationDirty, bool isScaleDirty) ApplyLocalNetworkState(Transform transform) |
There was a problem hiding this comment.
I know this is following the paradigm but at this point, should this mega-tuple be turned into a struct? The tuple signature is replicated at least 3 times in this file, and with this internal API you're exposing the tuple to outside classes.
There was a problem hiding this comment.
Yeah... I know... :(
I am trying to be very selective about what I am changing in these PRs...actually this PR is only a portion of #1846 just so I can get this rotation fix in without any other changes blocking this (a higher priority fix).
@ThusWroteNomad What do you think? Switch that to a struct in this PR or make that a separate PR?
There was a problem hiding this comment.
I think a nice and simple separate PR proposing tuple -> struct would be great :)
There was a problem hiding this comment.
Ok, once this gets merged I will whip up a quick PR for that.
NoelStephensUnity
deleted the
fix/mtt-3370-networktransform-delta-rotation-check
branch
Delta rotation checking was producing false positive results. This PR fixe this issue by using Mathf.DeltaAngle.
MTT-3370
Changelog
NetworkTransfromgenerating false positive rotation delta checks when rolling over between 0 and 360 degrees.Testing and Documentation