fix: NetworkTransform was not ending extrapolation for some states. [MTT-4521] #2170
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NoelStephensUnity
changed the title
JesseOlmer
reviewed
Aug 30, 2022
Added a form of tick synchronization prior to making changes to the authoritative transform state.
JesseOlmer
approved these changes
Aug 31, 2022
There was a problem hiding this comment.
I've approved, but honestly, NetworkTransform is still really hard for me to follow all of the authoritative/non-authoritative/local/interpolator/tick fields and their relationships through this code, so I'm mostly going on faith & our manual test verification here.
ShadauxCat
reviewed
Aug 31, 2022
ShadauxCat
reviewed
Aug 31, 2022
ShadauxCat
reviewed
Aug 31, 2022
Updated ApplyAuthoritativeState so that it is less complicated to follow. Fixed another issue discovered where the replicated network state values were being used for resetting interpolators during spawn initialization. Since the replicated network state only has the most recent deltas at the time the NetworkObject is serialized, this would cause "seemingly random" late joining issues.
This validates the issue with the more recent updates that revealed the network state only having the most recent deltas and not the full transform state. This resulted in various "seemingly" random late join transform synchronization issues (specifically the scale that is not being synchronized when spawning NetworkObjects).
removed the unnecessary else if (useInterpolatedValue) condition and re-organized it such that it checks to see if it needs to apply interpolation or just depends upon the state to apply the values directly. Added additional comments about this area of the code and future improvements.
This was referenced Sep 8, 2022
jakobbbb
pushed a commit
to GooseGirlGames/com.unity.netcode.gameobjects
that referenced
this pull request
Feb 22, 2023
…MTT-4521] (Unity-Technologies#2170) * update working version that needs clean up * update Some touch ups and adjustments to the fix for the extrapolation issue. * style adding some minor comments * update Added comments and renamed a property. * update some code clean up and additional comments * update MTT-4521 updating changelog file * update MTT-4521 Renamed StopExtrapolatingLastState to TryToStopExtrapolatingLastState. Added/adjusted comments. * update Applying suggested changes. * update removing some changes I made while testing out extrapolating past the target. adjusted comment. * style adjusted comment for clarity purposes. * test Adding some of the tests. Still WIP, but wanted to see how the first completed test fairs in Yamato and on consoles. * style removing unused namespace. * test update Added a form of tick synchronization prior to making changes to the authoritative transform state. * style adjusting comments. * test - Added NetworkTransformParentedLocalSpaceTests with associated helper methods, components, and properties. - Added an "Authority" suffixe to the Authority enums for better clarity when looking at the tests within test runner. - Updated/added additional comments * style Making NetworkTransformParentedLocalSpaceTests singular (NetworkTransformParentedLocalSpaceTest) * style adding remarks around comments * style splitting the comment into a summary with remarks. * style updating a comment on the UpdateAuthoritativeState method. * fix Copy paste issue. (need to revisit tests to see why they didn't catch this) * fix z component * fix using the eurlerAngles... that section of code was a mess! (sorry!) * update Just adding some parenthesis per Kitty's suggestion. * update and fix Updated ApplyAuthoritativeState so that it is less complicated to follow. Fixed another issue discovered where the replicated network state values were being used for resetting interpolators during spawn initialization. Since the replicated network state only has the most recent deltas at the time the NetworkObject is serialized, this would cause "seemingly random" late joining issues. * test update This validates the issue with the more recent updates that revealed the network state only having the most recent deltas and not the full transform state. This resulted in various "seemingly" random late join transform synchronization issues (specifically the scale that is not being synchronized when spawning NetworkObjects). * update removed the unnecessary else if (useInterpolatedValue) condition and re-organized it such that it checks to see if it needs to apply interpolation or just depends upon the state to apply the values directly. Added additional comments about this area of the code and future improvements. * fix Removing unclamped slerp and lerp due to a few timing related issues. These issues should be solved before re-enabling any form of unclamped lerping/slerping with BufferedLinearInterpolator. Added additional comments in some areas to be investigated for a potential fix. * fix temporarily disabling two editor based interpolator extrapolation based tests until I can refactor them to not take extrapolated values into consideration. * fix removing debug code used to verify NetworkTransform is not the only thing within NGO that fails when dropping packets (most likely to occur 20-30% drop rate). * fix Make sure to initialize based on whether it is set to world or local space. * fix When authority is applying the current position and rotation during OnNetworkSpawn, use the appropriate local vs world space values. * update Adding another fix to the change log Co-authored-by: ashwini <36935028+ashwinimurt@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The most recent
NetworkTransformupdate had a regression bug that exposed some potential timing issues with ending extrapolation. This resolves the issue by migrating the "stop extrapolating the last state" logic over to the non-authoritative side. This also fixes an issue where axis marked to be synchronized would not continue to interpolate/extrapolate even if the next state did not include deltas. This also helped resolve the over-all extrapolation issue where axial deltas would start to interpolate towards the target value but could potentially be stopped if another NetworkTransformState was received before the current state was ended.MTT-4521
Changelog
NetworkTransformwas not ending extrapolation for the previous state causing non-authoritative instances to become out of synch.NetworkTransformwas not continuing to interpolate for the remainder of the associated tick period.NetworkTransform.OnNetworkSpawnfor non-authoritative instances where it was initializing interpolators with the replicated network state which now only contains the transform deltas that occurred during a network tick and not the entire transform state. (WIP-Pending scale synchronization duringNetworkObjectserialization)NetworkTransformwas not honoring the InLocalSpace property on the authority side during OnNetworkSpawn.Testing and Documentation