chore: review pass over the batched NetworkTransform comments

Six comments asserted something that was not true. None of them fail a test, and none are visible from
the code they sit next to, which is what they have in common: every one is a claim about lifetime,
ordering, or the other implementation rather than about the lines below it.

- NativeInterpolator.ResetTo claimed the managed implementation seeds a baseline measurement and this
  one does not. Neither seeds one. The divergence it spent thirty lines explaining does not exist, and
  the explanation that does belong lives once, on BufferedLinearInterpolator.ResetTo.
- NetworkTransformStateManager said Register and Deregister are the only places the collections change.
  There are two independent sets, and the interpolation set changes in its own pair of methods.
- The WriteBatch guard said only the server registers instances for batching, which contradicts
  RegisterForBatchedStateTracking. A client authority does register. What is client-server only is the
  queuing, and a distributed authority session queues nothing at all.
- The handle allocator field said a handle is assigned to every synchronized instance. Only in batched
  mode.
- DetectTransformDeltaJob said every caller forcing a full state update ticks a nested instance. There
  is one such caller and it is gated on SwitchTransformSpaceWhenParented, which does not exclude an
  instance from batching. It runs from CommitDetectedState after the job, which is the real reason the
  job never needs to be told to force.
- Two test remarks described designs that had moved: the interpolator reset test described the bug in
  the present tense, and the state baseline test attributed the ReliableSequenced derivation to
  NetworkSerialize rather than to UpdateReliability.

The rest is verbosity. Remarks that ran to a paragraph per method are now one statement per line,
big-O notation is replaced by what actually happens, and the justifications for why the code is
correct are gone where the rule alone is enough.

serverTime is dropped from the internal interpolator reset path. BufferedLinearInterpolator.ResetTo
forwards it into an AddMeasurement call it does not make, so nothing reads it in either implementation.
The managed signature is public API and keeps it, with a note to obsolete it later.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Runtime/Components/DetectTransformDeltaJob.cs

@@ -7,24 +7,31 @@ namespace Unity.Netcode.Components
 {
     /// <summary>
     /// Motion Authority Only:
-    /// Detects <see cref="NetworkTransform"/> state changes for every registered instance in parallel.
+    /// Detects <see cref="NetworkTransform"/> state changes for every registered instance in a parallel job.
     /// </summary>
     /// <remarks>
-    /// This reads each transform and defers to the very same
     /// <see cref="CheckForStateChange(ref NetworkTransformState, ref NetworkDeltaPosition, ref TransformDeltaConfig, in TransformSample, bool, bool, bool)"/>
-    /// that the per instance path runs on the main thread to keep the logic between per instance and batched the same.<br />
-    /// Only transform values are read here and only the entries array is written, so there is no hierarchy
-    /// write hazard: nothing in this job touches a transform other than the one at its own index, and nothing
-    /// writes to a transform at all.
+    /// is the common method used for both per instance, runs on the main thread, and batched modes. This assures both paths detect changes in state identically.
     /// </remarks>
     [BurstCompile]
     internal struct DetectTransformDeltaJob : IJobParallelForTransform
     {
         /// <summary>
-        /// The per instance input and output, parallel to the transforms this job is scheduled over.
+        /// The per instance input and output, parallel to the job's scheduled transforms.
         /// </summary>
         public NativeArray<TransformDeltaEntry> Entries;
 
+        /// <summary>
+        /// This job's primary entry point.
+        /// </summary>
+        /// <remarks>
+        /// TODO: Investigate ways to work around the fact that a Rigidbody's position and rotation cannot
+        /// be sampled from a job. As such, any NetworkTransform that is using Rigidbody for motion will
+        /// use the <see cref="UnityEngine.GameObject.transform"/> to detect changes in position and rotation
+        /// states on the authority side.
+        /// </remarks>
+        /// <param name="index">Index for the transform in question.</param>
+        /// <param name="transform">The job safe <see cref="TransformAccess"/></param>
         public void Execute(int index, TransformAccess transform)
         {
             if (!transform.isValid)
@@ -34,17 +41,14 @@ public void Execute(int index, TransformAccess transform)
 
             var entry = Entries[index];
             var flagStates = entry.State.FlagStates;
-            // Only ResolveTransformSpace can raise this on the batched path: every caller that forces a
-            // full state update ticks a nested instance, and a nested instance is never registered for
-            // batching.
+            // Only ResolveTransformSpace can raise this on the batched path. The one caller that forces a full
+            // state update does so from CommitDetectedState on the main thread, after this job has run.
             var forceState = false;
 
             // Resolve the transform space before sampling, otherwise the wrong set of values gets compared.
             var transformSpaceChanged = ResolveTransformSpace(ref entry.Config, ref flagStates, entry.TransformHasParent, false, ref forceState);
             entry.State.FlagStates = flagStates;
 
-            // A rigidbody driven instance cannot be sampled from here, so it is never registered for the
-            // batched path and always falls back to the per instance flow.
             var rotation = entry.Config.InLocalSpace ? transform.localRotation : transform.rotation;
             entry.Sample.Position = entry.Config.InLocalSpace ? transform.localPosition : transform.position;
             entry.Sample.Rotation = rotation;

com.unity.netcode.gameobjects/Runtime/Components/Interpolator/BufferedLinearInterpolator.cs

@@ -274,7 +274,8 @@ public void Clear()
         /// Resets the current interpolator to the target value.
         /// </summary>
         /// <remarks>
-        /// This is used when first synchronizing/initializing and when telporting an object.
+        /// This is used when first synchronizing/initializing and when teleporting an object.<br />
+        /// <paramref name="serverTime"/> is not used. Mark this obsolete and deprecate it at a later date.
         /// </remarks>
         /// <param name="targetValue">The target value to reset the interpolator to</param>
         /// <param name="serverTime">The current server time</param>

com.unity.netcode.gameobjects/Runtime/Components/Interpolator/InterpolateTransformJob.cs

@@ -52,15 +52,15 @@ internal struct InterpolationEntry
 
     /// <summary>
     /// Non-Authority Only:
-    /// Advances the interpolators for every registered non-authority <see cref="NetworkTransform"/> in
-    /// parallel.
+    /// Handles interpolation for every registered non-authority <see cref="NetworkTransform"/> in
+    /// a parallel job.
     /// </summary>
     /// <remarks>
-    /// This performs the buffer consumption and the interpolation math only. Applying the results to the
-    /// transforms stays on the main thread for now (keeps this free of any hierarchy write order of operation complexities).<br />
-    /// <br />
-    /// Also, note that each entry owns its own slice of <see cref="BufferedItems"/> which assures no two indices address the same
-    /// items and that the whole array can be written without aliasing (pointing to the same thing).
+    /// This performs the buffer consumption and interpolation between two state updates only.<br />
+    /// Applying the results to the transforms stays on the main thread, which keeps this job free
+    /// of hierarchy write ordering.<br />
+    /// Each entry owns its own slice of <see cref="BufferedItems"/>, so no two indices address the same items.<br />
+    /// The whole array can be written without aliasing (access is to a distinct, independent memory region).
     /// </remarks>
     [BurstCompile]
     internal struct InterpolateTransformJob : IJobParallelFor

com.unity.netcode.gameobjects/Runtime/Components/Interpolator/NativeInterpolator.cs

@@ -296,37 +296,13 @@ internal static void Reset(ref NativeInterpolatorState state, float4 currentValu
         /// <see cref="BufferedLinearInterpolator{T}.ResetTo"/>.
         /// </summary>
         /// <remarks>
-        /// The managed implementation seeds a baseline measurement here, stamped with the caller's
-        /// <paramref name="serverTime"/>. This one deliberately does not, because that baseline becomes an
-        /// ordering floor that the measurements which follow it cannot clear.<br />
-        /// <br />
-        /// Callers pass <c>NetworkManager.ServerTime.Time</c>, the local current time, while incoming
-        /// measurements are stamped with the tick they were authored on
-        /// (<see cref="NetworkTransform.NetworkTransformState.SentTime"/>), which is always at least a tick
-        /// older. Once the baseline is consumed it becomes <see cref="NativeInterpolatorState.Target"/>, and
-        /// both of the guards that admit a measurement compare against it:
-        /// <see cref="AddMeasurement"/> requires a stamp newer than
-        /// <see cref="NativeInterpolatorState.LastMeasurementAddedTime"/>, and
-        /// <see cref="TryConsumeFromBuffer"/> requires one newer than <c>Target.TimeSent</c>. An instance that
-        /// resets part way through a session therefore rejects everything the authority sends next, and the
-        /// rejection is permanent: with a target already reached and a non empty buffer,
-        /// <see cref="Update"/> neither interpolates nor takes the stale target reset, so elapsed time alone
-        /// never recovers it.<br />
-        /// <br />
-        /// In practice this only reaches an instance that just stopped being the authority, which in a client
-        /// server topology is only ever the server. Leaving the buffer empty puts the interpolator in exactly
-        /// the state a freshly spawned one is in: the value is still held (<see cref="Reset"/> seeds all three
-        /// of the in flight values), the first measurement to arrive is taken unconditionally because
-        /// <see cref="NativeInterpolatorState.BufferCounter"/> is zero, and it is consumed against render time
-        /// alone. Seeding at spawn is unaffected — the baseline stamp there is one tick ahead of the first
-        /// measurement, so the interval the first measurement is interpolated over is the tick length either
-        /// way.
+        /// Clears the buffer and holds <paramref name="targetValue"/> as the current value.<br />
+        /// No baseline measurement is recorded, which is what the managed implementation does as well. See
+        /// <see cref="BufferedLinearInterpolator{T}.ResetTo(T, double)"/> for why.<br />
+        /// This leaves the interpolator in the state a freshly spawned one is in, so the next measurement to
+        /// arrive is taken unconditionally.
         /// </remarks>
-        /// <param name="serverTime">
-        /// Retained for signature parity with <see cref="BufferedLinearInterpolator{T}.ResetTo"/>. Not stored,
-        /// for the reason above.
-        /// </param>
-        internal static void ResetTo(ref NativeInterpolatorState state, ref NativeArray<BufferedItemNative> items, float4 targetValue, double serverTime)
+        internal static void ResetTo(ref NativeInterpolatorState state, ref NativeArray<BufferedItemNative> items, float4 targetValue)
         {
             Clear(ref state);
             state.RateOfChange = float4.zero;
@@ -394,20 +370,17 @@ internal static void ResetCurrentState(ref NativeInterpolatorState state)
         }
 
         /// <summary>
-        /// Re-expresses every buffered measurement, and the values currently in flight, in a different space.
+        /// Re-expresses the buffered measurements and the in flight values in a different transform space.
         /// </summary>
         /// <remarks>
-        /// Invoked when the instance is reparented, which is the only thing that changes the space its
-        /// measurements are interpreted in. Converting the whole buffer at that moment keeps everything in one
-        /// space, which is what allows the interpolation job to have no knowledge of parents at all.<br />
-        /// <br />
-        /// The managed interpolator instead tags each measurement with the parent it arrived under and
-        /// converts lazily as the queue drains past the boundary. The net effect is the same transition; doing
-        /// it here means one conversion at a known instant, using both parents' poses as they are at that
-        /// instant, rather than a conversion per buffered item using poses sampled as each is consumed.
+        /// Invoked when the instance is reparented. Reparenting is the only thing that changes the
+        /// transform space for its measurements.<br />
+        /// Converting everything at that point is what keeps the interpolation job free of any parent
+        /// knowledge.<br />
+        /// The managed interpolator converts lazily as its queue drains instead. Both reach the same result.
         /// </remarks>
-        /// <param name="pointTransform">Converts a position from the old space to the new one.</param>
-        /// <param name="rotationTransform">Converts a rotation from the old space to the new one.</param>
+        /// <param name="pointTransform">Converts a position from the old transform space to the new one.</param>
+        /// <param name="rotationTransform">Converts a rotation from the old transform space to the new one.</param>
         internal static void ConvertSpace(ref NativeInterpolatorState state, ref NativeArray<BufferedItemNative> items, in float4x4 pointTransform, in quaternion rotationTransform)
         {
             for (int i = 0; i < state.BufferCount; i++)

com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs

@@ -1789,11 +1789,11 @@ public Vector3 GetScale(bool getCurrentState = false)
         /// <summary>
         /// When <see cref="TransformSyncModes.Batched"/> mode, this is the instance's index within <see cref="NetworkTransformStateManager"/>.<br />
         /// It is -1 when it is not registered.
-        /// registered.
         /// </summary>
         /// <remarks>
-        /// Cached here (as opposed to using a lookup table) so registering and deregistering is O(1). The
-        /// manager keeps this up to date as instances are swapped between slots.
+        /// Cached here as opposed to using a lookup table, so registering and deregistering do not have to
+        /// search for the instance.<br />
+        /// The manager keeps this up to date as instances are swapped between slots.
         /// </remarks>
         internal int StateManagerIndex = -1;
 
@@ -2605,7 +2605,7 @@ internal void UpdatePositionInterpolator(Vector3 position, double time, bool res
                     var value = new float4(position.x, position.y, position.z, 0.0f);
                     if (resetInterpolator)
                     {
-                        m_CachedNetworkManager.TransformStateManager.ResetTo(InterpolatorIndex, NetworkTransformStateManager.InterpolatorTarget.Position, value, time);
+                        m_CachedNetworkManager.TransformStateManager.ResetTo(InterpolatorIndex, NetworkTransformStateManager.InterpolatorTarget.Position, value);
                     }
                     else
                     {
@@ -2638,7 +2638,7 @@ private void UpdateRotationInterpolator(Quaternion rotation, double time, bool r
                 var value = new float4(rotation.x, rotation.y, rotation.z, rotation.w);
                 if (resetInterpolator)
                 {
-                    m_CachedNetworkManager.TransformStateManager.ResetTo(InterpolatorIndex, NetworkTransformStateManager.InterpolatorTarget.Rotation, value, time);
+                    m_CachedNetworkManager.TransformStateManager.ResetTo(InterpolatorIndex, NetworkTransformStateManager.InterpolatorTarget.Rotation, value);
                 }
                 else
                 {
@@ -2670,7 +2670,7 @@ private void UpdateScaleInterpolator(Vector3 scale, double time, bool resetInter
                 var value = new float4(scale.x, scale.y, scale.z, 0.0f);
                 if (resetInterpolator)
                 {
-                    m_CachedNetworkManager.TransformStateManager.ResetTo(InterpolatorIndex, NetworkTransformStateManager.InterpolatorTarget.Scale, value, time);
+                    m_CachedNetworkManager.TransformStateManager.ResetTo(InterpolatorIndex, NetworkTransformStateManager.InterpolatorTarget.Scale, value);
                 }
                 else
                 {

com.unity.netcode.gameobjects/Runtime/Components/NetworkTransformDeltaCheck.cs

@@ -11,9 +11,8 @@ public partial class NetworkTransform
         /// Everything <see cref="CheckForStateChange"/> needs from the instance it is checking.
         /// </summary>
         /// <remarks>
-        /// This creates the abstraction layer between <see cref="NetworkTransform"/> itself and the configuration
-        /// of the <see cref="NetworkTransform"/> in order to assure the delta check can be run both on the main
-        /// thread and from within a job.<br />
+        /// Separates the delta check from <see cref="NetworkTransform"/> itself, which is what lets it run both
+        /// on the main thread and from within a job.<br />
         /// A few members are read/write: the check can change the transform space it operates in and it
         /// advances the axial frame synchronization bookkeeping, both of which have to make it back to the
         /// instance.
@@ -54,7 +53,8 @@ internal struct TransformDeltaConfig
             internal int HalfFloatTargetTickOwnership;
 
             /// <summary>
-            /// Read/write. The tick slot this instance next sends an axial frame synchronization on.
+            /// Read/write. The next tick to send an axial frame synchronization, if a delta has been sent
+            /// since the last one.
             /// </summary>
             internal int NextTickSync;
 
@@ -156,12 +156,12 @@ internal struct TransformDeltaEntry
 
         /// <summary>
         /// Abstraction Layer Method:
-        /// Resolves which transform space the delta check operates in.
+        /// Resolves the transform space for both per-instance and batched mode delta checks.
         /// </summary>
         /// <remarks>
         /// Runs before the transform is sampled because it determines whether the local or the world values
         /// are the ones being compared. Kept separate (as opposed to being folded into
-        /// <see cref="CheckForStateChange"/>) so that neither caller has to sample both spaces.
+        /// <see cref="CheckForStateChange"/>) so that neither caller has to sample both transform spaces.
         /// </remarks>
         /// <param name="config">The instance configuration. <see cref="TransformDeltaConfig.InLocalSpace"/> may be updated.</param>
         /// <param name="flagStates">The state flags being updated.</param>