Prediction policies - #58
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds prediction-policy infrastructure for full prediction, server relay, soft correction, and ownership-based prediction. Updates identity lifecycles, replay handling, state history, physics reconciliation, pooling, spawning, scenario coordination, and editor validation. ChangesPrediction runtime and policy system
Prediction scenarios and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9db1763def
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _rigidbody.collisionDetectionMode = _defaultCollisionMode; | ||
| _rigidbody.isKinematic = _defaultKinematic; |
There was a problem hiding this comment.
When a relayed Rigidbody becomes locally predicted again and its default collision mode is Continuous or ContinuousDynamic, this restores _defaultCollisionMode while the body is still kinematic from ForceRelayKinematic(). Unity only supports those continuous modes on non-kinematic bodies, so the assignment can be rejected or coerced, leaving the body dynamic again but not back on its original CCD mode; restore isKinematic first when returning to a dynamic default, then restore the collision mode.
Useful? React with 👍 / 👎.
| else if (!preserveInterpolation) | ||
| _interpolatedState.Teleport(fullPredictedState.DeepCopy()); |
There was a problem hiding this comment.
When a pooled object with the same PredictedObjectID is re-registered through PredictedHierarchy with a non-SoftCorrection policy, preservesStateOnSetup is still true, so this branch skips teleporting the interpolation buffer even though the code immediately above resets fullPredictedState and the history. That leaves the view/interpolation queue from the object's previous pooled lifetime attached to the new state, causing reused FullPrediction/ServerRelay objects to render from stale poses after rollback-driven delete/recreate cycles; only the SoftCorrection early-return path should preserve the old interpolation state.
Useful? React with 👍 / 👎.
| if (preservesStateOnSetup) | ||
| ModuleSetup(world); | ||
| else ResetModulesForReuse(world); |
There was a problem hiding this comment.
When rollback deletes and recreates a pooled object with the same PredictedObjectID, preservesStateOnSetup is true for every policy, not just SoftCorrection. This branch then skips ResetModulesForReuse, so FullPrediction/ServerRelay objects keep dynamic modules that were registered in the previous pooled lifetime; those stale modules can continue simulating and serializing on what should be a fresh spawn. Gate preservation to SoftCorrection or reset modules for the non-soft policies.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs (1)
446-450: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare magnitude to magnitude, not squared distance.
positionErroris a magnitude, butminThresholdis squared. This makes the minimum correction clamp trigger at the wrong threshold.Proposed fix
- float minThreshold = posThreshold.x * posThreshold.x; + float minThresholdSqr = posThreshold.x * posThreshold.x; float corrMag = correction.sqrMagnitude; - if (corrMag < minThreshold && positionError > minThreshold) + if (corrMag < minThresholdSqr && positionError > posThreshold.x) correction = correction.normalized * posThreshold.x;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs` around lines 446 - 450, In PredictedTransform’s correction clamping logic, the comparison mixes a magnitude value with a squared threshold, so the minimum correction check triggers at the wrong point. Update the condition around the `positionError`, `minThreshold`, and `correction.sqrMagnitude` check to compare like-for-like values, using either magnitudes on both sides or squared values on both sides, and keep the clamp behavior in the same `PredictedTransform` method.
🧹 Nitpick comments (16)
Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the private marker field to
_camelCase.Proposed fix
- static readonly ProfilerMarker SimulateMarker = new("DeepCopy.Module." + typeof(T).FullName); + static readonly ProfilerMarker _simulateMarker = new("DeepCopy.Module." + typeof(T).FullName); @@ - using (SimulateMarker.Auto()) + using (_simulateMarker.Auto())As per coding guidelines, “Private fields must use _camelCase prefix naming convention.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs` around lines 20 - 24, The private ProfilerMarker field in ModulePredictedState<T> does not follow the required private field naming convention. Rename SimulateMarker to _camelCase style in the ModulePredictedState<T> type and update its use inside DeepCopy() so the private static marker field matches the project guideline for private fields.Source: Coding guidelines
Assets/PurrDiction/Runtime/Core/PredictedIdentity.cs (1)
161-165: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInline these replay-state helpers.
IsSoftCorrectionReplaySimulating()participates in replay save decisions, and these helpers are simple hot-path accessors; add the sameMethodImpl(AggressiveInlining)treatment used by the neighboring policy helpers. As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".Suggested patch
+ [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal bool IsSoftCorrectionReplaySimulating() => _simulateSoftCorrectionDuringReplay; + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal bool SkipsReplaySpawnInitialization() => _skipReplaySpawnInitialization;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/PredictedIdentity.cs` around lines 161 - 165, Add MethodImpl(MethodImplOptions.AggressiveInlining) to the replay-state accessor helpers in PredictedIdentity, matching the neighboring policy helpers. Update IsSoftCorrectionReplaySimulating() and SkipsReplaySpawnInitialization() so these hot-path methods are explicitly marked for aggressive inlining, keeping the existing return behavior unchanged.Source: Coding guidelines
Assets/PurrDiction/Runtime/Core/PredictionManager.cs (2)
1138-1311: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider splitting replay/speculative-relay orchestration into a partial.
This new replay block significantly expands
PredictionManager; moving the replay/relay helpers into aPredictionManager.Replay.cspartial would keep the core manager easier to navigate. As per coding guidelines, "Large types should use partial classes for organization".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/PredictionManager.cs` around lines 1138 - 1311, The replay and speculative-relay orchestration has grown large inside PredictionManager, so split these helpers into a dedicated partial to keep the class organized. Move the replay-related methods and state such as OnPostTick, LockSpeculativeRelayStates, HasSpeculativeRelayLock, RestoreSpeculativeRelayStates, ClearSpeculativeRelayLocks, NotifyReplayStart, FreezeForReplay, and NotifyReplayEnd into a PredictionManager.Replay.cs partial, preserving their current behavior and references to shared fields and methods.Source: Coding guidelines
1219-1276: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInline the speculative-relay helpers used in replay/tick loops.
LockSpeculativeRelayStates,HasSpeculativeRelayLock,RestoreSpeculativeRelayStates, andClearSpeculativeRelayLockssit on the prediction hot path; mark them for aggressive inlining. As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/PredictionManager.cs` around lines 1219 - 1276, Mark the speculative-relay hot-path helpers in PredictionManager for aggressive inlining: add [MethodImpl(MethodImplOptions.AggressiveInlining)] to LockSpeculativeRelayStates, HasSpeculativeRelayLock, RestoreSpeculativeRelayStates, and ClearSpeculativeRelayLocks. Keep the implementations unchanged and ensure the MethodImpl attribute is applied consistently to these replay/tick loop helpers so the runtime can inline them on the prediction path.Source: Coding guidelines
Assets/PurrDiction/Runtime/Core/PredictedIdentity.Lifecycle.cs (1)
9-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMark these lifecycle wrappers for aggressive inlining.
These wrappers are called per system across tick/replay loops; add
MethodImpl(AggressiveInlining)consistently. As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".Suggested patch
+ [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunSimulateTick(ulong tick, float delta) @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunLateSimulateTick(float delta) @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunPrepareSimulationInputs(ulong tick, float delta) @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunPostSimulate() @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunGetLatestUnityState() @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunSaveState(ulong tick) @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunSaveStateUnchecked(ulong tick) @@ + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] internal void RunUpdateRollbackInterpolation(float delta, bool accumulateError)Also applies to: 70-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/PredictedIdentity.Lifecycle.cs` around lines 9 - 49, Add MethodImpl(MethodImplOptions.AggressiveInlining) consistently to the hot-path lifecycle wrapper methods in PredictedIdentity.Lifecycle, including RunSimulateTick, RunLateSimulateTick, RunPrepareSimulationInputs, RunPostSimulate, and RunGetLatestUnityState (and the related wrappers in the 70-90 range). Keep the existing guard-and-forward behavior unchanged, but ensure each wrapper is marked for aggressive inlining alongside the other lifecycle methods in this class.Source: Coding guidelines
Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.cs (1)
250-250: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd the required hot-path inlining attribute.
As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.cs` at line 250, The Simulate method in PredictedRigidbody2D is a hot-path override and needs the required inlining annotation. Add [MethodImpl(MethodImplOptions.AggressiveInlining)] to Simulate, and make sure the needed System.Runtime.CompilerServices import is available so the attribute resolves correctly.Source: Coding guidelines
Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs (2)
160-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd the required hot-path inlining attribute.
As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs` at line 160, The Simulate method in PredictedProjectile3D is a hot-path override and must be marked for aggressive inlining. Add the required MethodImpl(MethodImplOptions.AggressiveInlining) attribute to Simulate, keeping the change localized to this hot-path method so it follows the coding guideline.Source: Coding guidelines
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the private constant to match field naming rules.
As per coding guidelines, "
**/*.cs: Private fields must use _camelCase prefix naming convention".Proposed fix
- private const float SafetyMarginFactor = 0.1f; + private const float _safetyMarginFactor = 0.1f; - float safetyMargin = Mathf.Max(state.radius * SafetyMarginFactor, 0.001f); + float safetyMargin = Mathf.Max(state.radius * _safetyMarginFactor, 0.001f);Also applies to: 176-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs` at line 34, The private constants in PredictedProjectile3D, including SafetyMarginFactor and the other flagged constant, do not follow the required _camelCase prefix convention. Rename them to private field-style names with a leading underscore and update any references within the class so the naming matches the coding guidelines consistently.Source: Coding guidelines
Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs (1)
313-313: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd the required hot-path inlining attribute.
As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs` at line 313, The Simulate method in PredictedRigidbody is a hot-path override and is missing the required inlining hint. Add the MethodImplAttribute with MethodImplOptions.AggressiveInlining to Simulate(ref UnityRigidbodyState state, float delta), matching the project’s hot-path guideline and keeping the attribute on the method declaration so it remains easy to locate if signatures change.Source: Coding guidelines
Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs (1)
299-299: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd the required hot-path inlining attribute.
Simulateruns on the prediction tick path.As per coding guidelines, "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs` at line 299, The Simulate method in PredictedTransform is part of the prediction tick hot path, so add the required MethodImpl(MethodImplOptions.AggressiveInlining) attribute to this override. Update the Simulate(ref PredictedTransformState state, float delta) declaration to include the inlining attribute consistent with the coding guidelines for hot-path methods.Source: Coding guidelines
Assets/PurrDiction/Runtime/Core/AppliedCorrectionRing.cs (1)
7-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the private-field naming convention to the ring size constant.
Rename
Sizeand its references to use the required_camelCaseprivate-field prefix.As per coding guidelines, "
**/*.cs: Private fields must use _camelCase prefix naming convention".Proposed fix
- private const int Size = 128; + private const int _size = 128; - private readonly ulong[] _ticks = new ulong[Size]; - private readonly T[] _values = new T[Size]; - private readonly bool[] _valid = new bool[Size]; + private readonly ulong[] _ticks = new ulong[_size]; + private readonly T[] _values = new T[_size]; + private readonly bool[] _valid = new bool[_size]; - int index = (int)(tick % Size); + int index = (int)(tick % _size); - int index = (int)(tick % Size); + int index = (int)(tick % _size); - for (int i = 0; i < Size; i++) + for (int i = 0; i < _size; i++) - Array.Clear(_ticks, 0, Size); - Array.Clear(_values, 0, Size); - Array.Clear(_valid, 0, Size); + Array.Clear(_ticks, 0, _size); + Array.Clear(_values, 0, _size); + Array.Clear(_valid, 0, _size);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/AppliedCorrectionRing.cs` around lines 7 - 65, The private ring-size constant in AppliedCorrectionRing does not follow the required _camelCase private-field naming convention. Rename Size to use the private-field prefix and update every reference in Record, TryGetBaseline, and Clear so the class consistently follows the coding guideline for private fields.Source: Coding guidelines
Assets/PredictionTests/Scripts/Scenarios/PolicyBallRig.cs (1)
36-59: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider adding
[MethodImpl(MethodImplOptions.AggressiveInlining)]toSimulate.
Simulateis invoked every prediction tick (confirmed by the downstreamPredictedModuleStatetick loop) and qualifies as a hot path method. Per coding guidelines, hot path methods should use[MethodImpl(MethodImplOptions.AggressiveInlining)]. This is test code so the impact is low, but it aligns with the project convention.As per coding guidelines: "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".
♻️ Optional refactor
+using System.Runtime.CompilerServices; + protected override void Simulate(ref RigState state, sfloat delta) + [MethodImpl(MethodImplOptions.AggressiveInlining)] { if (state.spawned || !ballPrefab) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/PolicyBallRig.cs` around lines 36 - 59, Add the missing MethodImplAttribute to PolicyBallRig.Simulate so it follows the hot-path convention. Update the Simulate override in PolicyBallRig to include [MethodImpl(MethodImplOptions.AggressiveInlining)] and ensure the needed MethodImplOptions/MethodImpl namespace is available, keeping the behavior unchanged.Source: Coding guidelines
Assets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs (1)
53-67: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider adding
[MethodImpl(MethodImplOptions.AggressiveInlining)]toSimulate.
Simulateis called every prediction tick and qualifies as a hot path method. Per coding guidelines, it should use[MethodImpl(MethodImplOptions.AggressiveInlining)]. Low priority for test code.As per coding guidelines: "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute".
♻️ Optional refactor
+using System.Runtime.CompilerServices; + protected override void Simulate(ref ProbeState state, float delta) + [MethodImpl(MethodImplOptions.AggressiveInlining)] { if (predictionManager.cachedIsServer) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs` around lines 53 - 67, Add [MethodImpl(MethodImplOptions.AggressiveInlining)] to the Simulate override in OwnedRelayProbe so it follows the hot-path coding guideline; make sure the method remains the same otherwise, and add the required using or fully qualified reference if needed to resolve MethodImplOptions.Source: Coding guidelines
Assets/PredictionTests/Scripts/Scenarios/RelayProbe.cs (1)
60-80: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Simulateis a hot-path method missing[MethodImpl(MethodImplOptions.AggressiveInlining)].Per coding guidelines, hot-path methods must use
[MethodImpl(MethodImplOptions.AggressiveInlining)].Simulateis invoked every predicted tick via the simulation loop and qualifies as a hot path.As per coding guidelines: "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute."
♻️ Proposed refactor
+using System.Runtime.CompilerServices; + public class RelayProbe : PredictedIdentity<RelayProbe.ProbeState> { // ... + [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override void Simulate(ref ProbeState state, float delta) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/RelayProbe.cs` around lines 60 - 80, Add the required [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute to the RelayProbe.Simulate override so it follows the hot-path coding guideline. Locate the Simulate method in RelayProbe and update its declaration to include the inlining attribute, keeping the existing simulation logic unchanged.Source: Coding guidelines
Assets/PredictionTests/Scripts/Scenarios/SoftProbe.cs (1)
69-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Simulateis a hot-path method missing[MethodImpl(MethodImplOptions.AggressiveInlining)].Same as
RelayProbe.Simulate— this method runs every predicted tick and should carry the inlining attribute per coding guidelines.As per coding guidelines: "Hot path methods must use [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute."
♻️ Proposed refactor
+using System.Runtime.CompilerServices; + public class SoftProbe : PredictedIdentity<SoftProbe.ProbeState> { // ... + [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override void Simulate(ref ProbeState state, float delta) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/SoftProbe.cs` around lines 69 - 91, Add the [MethodImpl(MethodImplOptions.AggressiveInlining)] attribute to SoftProbe.Simulate so it matches the hot-path handling used by RelayProbe.Simulate. Update the method declaration in SoftProbe to include the inlining hint and ensure the required MethodImpl namespace is available if it is not already.Source: Coding guidelines
Assets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs (1)
39-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared scenario logic to reduce duplication.
RunScenariois nearly line-for-line identical toSoftCorrectionScenario.RunScenario, differing only in probe type (SoftProbe2DvsSoftProbe) and error message text. TheSetupmethods also share the same structure with different component types. A generic base class (e.g.,SoftCorrectionScenarioBase<TProbe>) or a shared helper could eliminate ~60 lines of duplicated async flow logic.♻️ Optional refactor: generic base class
// Shared base for both 3D and 2D soft-correction scenarios public abstract class SoftCorrectionScenarioBase<TProbe> : Scenario where TProbe : Component { [SerializeField] protected PolicyBallRig _rig; [SerializeField] protected float _minDivergence = 0.3f; [SerializeField] protected float _convergedDistance = 0.15f; [SerializeField] protected float _settleSeconds = 2f; [SerializeField] protected float _timeout = 90f; protected abstract List<TProbe> Instances { get; } protected abstract bool ImpulseApplied { get; } protected abstract float MaxObservedDivergence { get; } protected abstract int ReplayViolations { get; } protected abstract void ResetCounters(); protected abstract float GetDivergence(TProbe probe); protected abstract string ProbeName { get; } public override async UniTask<ScenarioResult> RunScenario(ScenarioContext ctx) { try { await UniTaskUtils.WaitWithTimeout( () => _rig.hasSpawned && Instances.Count > 0, _timeout, ctx.cancellationToken); } catch (TimeoutException) { return ScenarioResult.Fail($"{ProbeName} never spawned: spawned={_rig.hasSpawned}"); } if (ctx.role != NetworkRole.Client) return ScenarioResult.Ok(); var probe = Instances[0]; try { await UniTaskUtils.WaitWithTimeout(() => ImpulseApplied, _timeout, ctx.cancellationToken); } catch (TimeoutException) { return ScenarioResult.Fail($"client-side {ProbeName} impulse was never applied"); } try { await UniTaskUtils.WaitWithTimeout( () => MaxObservedDivergence >= _minDivergence, 20f, ctx.cancellationToken); } catch (TimeoutException) { return ScenarioResult.Fail($"{ProbeName} impulse produced no divergence (max={MaxObservedDivergence:F3}, expected >= {_minDivergence})"); } double convergedSince = Time.realtimeSinceStartupAsDouble; try { await UniTaskUtils.WaitWithTimeout( () => { var now = Time.realtimeSinceStartupAsDouble; if (GetDivergence(probe) > _convergedDistance) { convergedSince = now; return false; } return now - convergedSince >= _settleSeconds; }, _timeout, ctx.cancellationToken); } catch (TimeoutException) { return ScenarioResult.Fail($"{ProbeName} never converged (divergence={GetDivergence(probe):F3})"); } if (ReplayViolations > 0) return ScenarioResult.Fail($"{ProbeName} simulated {ReplayViolations} times during replay/verified frames"); return ScenarioResult.Ok(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs` around lines 39 - 106, Extract the duplicated soft-correction scenario flow shared by SoftCorrection2DScenario.RunScenario and SoftCorrectionScenario.RunScenario into a common base class or helper, keeping only probe-specific differences (SoftProbe2D vs SoftProbe, messages, and divergence accessors) in the derived classes. Also fold the matching Setup structure into the same shared abstraction so both scenarios reuse one async/spawn-check path instead of maintaining parallel implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/PredictionTests/Scripts/Scenarios/RelayProbe.cs`:
- Line 18: The static RelayProbe.instances list is not being reset when
ResetCounters runs, so stale probe references can leak into later scenario runs.
Update RelayProbe.ResetCounters to also clear instances alongside the other
static counters, and make sure ServerRelayScenario continues to rely on
RelayProbe.instances only after that reset path so it won’t read destroyed
probes.
In `@Assets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs`:
- Around line 17-40: The ServerRelayScenario.Setup method creates temporary
GameObjects for the relay floor and ball, but neither has a teardown path, and
the ball is also marked DontDestroyOnLoad. Add scenario cleanup so these objects
are destroyed when the scenario completes, preferably by storing references in
ServerRelayScenario and disposing them in the scenario’s end/teardown hook after
Setup.
In `@Assets/PredictionTests/Scripts/Scenarios/SoftProbe.cs`:
- Around line 23-28: ResetCounters in SoftProbe only resets numeric state and
leaves the static instances list intact, so stale probe references can survive
across scenario runs. Update ResetCounters to also clear instances, matching the
behavior expected for RelayProbe, and keep the reset logic grouped with the
existing replayViolations, impulseApplied, and maxObservedDivergence resets.
In `@Assets/PurrDiction/Editor/PredictionPolicyDrawer.cs`:
- Around line 9-21: The private static fields in PredictionPolicyDrawer violate
the _camelCase naming convention. Rename DeterministicPolicies and
DeterministicPolicyLabels to _deterministicPolicies and
_deterministicPolicyLabels, and update any references within
PredictionPolicyDrawer so the editor drawer still uses the renamed arrays
consistently.
In `@Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs`:
- Around line 7-12: `ModulePredictedState` is missing the auto-packing marker
required for predicted-state serialization. Update the `ModulePredictedState`
struct so it implements `IPackedAuto` alongside
`IPredictedData<ModulePredictedState>` and `IDisposable`, keeping the existing
`Dispose` member intact. Use the `ModulePredictedState` type in
`Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs` as the place to make
this interface change so `Packer<ModulePredictedState>` can generate
auto-serialization correctly.
In `@Assets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.cs`:
- Around line 135-154: The setup flow in PredictedIdentityStatefull leaves stale
interpolation samples for non-SoftCorrection policies because
preserveInterpolation can be true while the interpolation buffer is not actually
retained. Update the setup logic around ResetStateToInitialState,
GetLatestUnityState, and _interpolatedState.Teleport so that whenever the
existing _interpolatedState is kept but preservation is not being used for
SoftCorrection, it is teleported to fullPredictedState.DeepCopy() before
continuing; keep the early return only for the true preserved SoftCorrection
case.
In `@Assets/PurrDiction/Runtime/Core/PredictedModuleState.cs`:
- Around line 66-77: Reset the interpolation buffer whenever
PredictedModuleState::SetUp (or the surrounding setup/reset path) is not
preserving soft-correction state. The current preserveInterpolation check only
protects the soft-correction timeline, but when preservesStateOnSetup is true
under other policies the state/history are cleared while _interpolatedState
still contains stale samples. Update the non-preserving branch so
_interpolatedState is also cleared or reinitialized alongside
ResetStateToInitialState, _history.Clear, and _viewState disposal, while keeping
the existing early return for UsesSoftCorrectionTimeline().
In `@Assets/PurrDiction/Runtime/Core/PredictionManager.cs`:
- Line 317: PredictionManager.CleanupAllSystems currently clears only the
replay-state collection, but _speculativeRelayLocks can still keep stale
PredictedIdentity entries after despawn/respawn. Update the cleanup logic so
CleanupAllSystems clears both _replayFrozenSystems and _speculativeRelayLocks
together, keeping the replay/speculative state fully reset during system
cleanup.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs`:
- Around line 13-14: The soft correction rate in PredictedProjectile3D is
allowed to go negative, which makes the SoftCorrection blend amplify velocity
error instead of reducing it. Update the _softVelocityCorrectionRate usage in
the soft-correction path of PredictedProjectile3D (including the other affected
spot mentioned in the comment) to clamp or validate it to a non-negative value
before computing blend, and keep the correction logic in the same named code
paths so negative inputs cannot invert the decay behavior.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs`:
- Around line 38-39: The soft-correction rate used by PredictedRigidbody can go
negative, which makes the blend reverse direction and move velocity farther from
the verified value. Clamp or validate _softVelocityCorrectionRate to a
non-negative range at the point it is configured and wherever it is used in the
soft-correction logic, so the correction always moves toward the verified
velocity. Apply the same guard in the other soft-correction site referenced by
the review as well.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.cs`:
- Around line 14-15: The soft-correction rate in PredictedRigidbody2D can go
negative and make the SoftCorrection blend amplify velocity error instead of
reducing it. Update the _softVelocityCorrectionRate field usage and the
soft-correction logic near the linear/angular blend calculation to clamp or
validate the rate to a non-negative value before it is used, so both velocity
and angular error decay correctly.
---
Outside diff comments:
In `@Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs`:
- Around line 446-450: In PredictedTransform’s correction clamping logic, the
comparison mixes a magnitude value with a squared threshold, so the minimum
correction check triggers at the wrong point. Update the condition around the
`positionError`, `minThreshold`, and `correction.sqrMagnitude` check to compare
like-for-like values, using either magnitudes on both sides or squared values on
both sides, and keep the clamp behavior in the same `PredictedTransform` method.
---
Nitpick comments:
In `@Assets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs`:
- Around line 53-67: Add [MethodImpl(MethodImplOptions.AggressiveInlining)] to
the Simulate override in OwnedRelayProbe so it follows the hot-path coding
guideline; make sure the method remains the same otherwise, and add the required
using or fully qualified reference if needed to resolve MethodImplOptions.
In `@Assets/PredictionTests/Scripts/Scenarios/PolicyBallRig.cs`:
- Around line 36-59: Add the missing MethodImplAttribute to
PolicyBallRig.Simulate so it follows the hot-path convention. Update the
Simulate override in PolicyBallRig to include
[MethodImpl(MethodImplOptions.AggressiveInlining)] and ensure the needed
MethodImplOptions/MethodImpl namespace is available, keeping the behavior
unchanged.
In `@Assets/PredictionTests/Scripts/Scenarios/RelayProbe.cs`:
- Around line 60-80: Add the required
[MethodImpl(MethodImplOptions.AggressiveInlining)] attribute to the
RelayProbe.Simulate override so it follows the hot-path coding guideline. Locate
the Simulate method in RelayProbe and update its declaration to include the
inlining attribute, keeping the existing simulation logic unchanged.
In `@Assets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs`:
- Around line 39-106: Extract the duplicated soft-correction scenario flow
shared by SoftCorrection2DScenario.RunScenario and
SoftCorrectionScenario.RunScenario into a common base class or helper, keeping
only probe-specific differences (SoftProbe2D vs SoftProbe, messages, and
divergence accessors) in the derived classes. Also fold the matching Setup
structure into the same shared abstraction so both scenarios reuse one
async/spawn-check path instead of maintaining parallel implementations.
In `@Assets/PredictionTests/Scripts/Scenarios/SoftProbe.cs`:
- Around line 69-91: Add the [MethodImpl(MethodImplOptions.AggressiveInlining)]
attribute to SoftProbe.Simulate so it matches the hot-path handling used by
RelayProbe.Simulate. Update the method declaration in SoftProbe to include the
inlining hint and ensure the required MethodImpl namespace is available if it is
not already.
In `@Assets/PurrDiction/Runtime/Core/AppliedCorrectionRing.cs`:
- Around line 7-65: The private ring-size constant in AppliedCorrectionRing does
not follow the required _camelCase private-field naming convention. Rename Size
to use the private-field prefix and update every reference in Record,
TryGetBaseline, and Clear so the class consistently follows the coding guideline
for private fields.
In `@Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs`:
- Around line 20-24: The private ProfilerMarker field in ModulePredictedState<T>
does not follow the required private field naming convention. Rename
SimulateMarker to _camelCase style in the ModulePredictedState<T> type and
update its use inside DeepCopy() so the private static marker field matches the
project guideline for private fields.
In `@Assets/PurrDiction/Runtime/Core/PredictedIdentity.cs`:
- Around line 161-165: Add MethodImpl(MethodImplOptions.AggressiveInlining) to
the replay-state accessor helpers in PredictedIdentity, matching the neighboring
policy helpers. Update IsSoftCorrectionReplaySimulating() and
SkipsReplaySpawnInitialization() so these hot-path methods are explicitly marked
for aggressive inlining, keeping the existing return behavior unchanged.
In `@Assets/PurrDiction/Runtime/Core/PredictedIdentity.Lifecycle.cs`:
- Around line 9-49: Add MethodImpl(MethodImplOptions.AggressiveInlining)
consistently to the hot-path lifecycle wrapper methods in
PredictedIdentity.Lifecycle, including RunSimulateTick, RunLateSimulateTick,
RunPrepareSimulationInputs, RunPostSimulate, and RunGetLatestUnityState (and the
related wrappers in the 70-90 range). Keep the existing guard-and-forward
behavior unchanged, but ensure each wrapper is marked for aggressive inlining
alongside the other lifecycle methods in this class.
In `@Assets/PurrDiction/Runtime/Core/PredictionManager.cs`:
- Around line 1138-1311: The replay and speculative-relay orchestration has
grown large inside PredictionManager, so split these helpers into a dedicated
partial to keep the class organized. Move the replay-related methods and state
such as OnPostTick, LockSpeculativeRelayStates, HasSpeculativeRelayLock,
RestoreSpeculativeRelayStates, ClearSpeculativeRelayLocks, NotifyReplayStart,
FreezeForReplay, and NotifyReplayEnd into a PredictionManager.Replay.cs partial,
preserving their current behavior and references to shared fields and methods.
- Around line 1219-1276: Mark the speculative-relay hot-path helpers in
PredictionManager for aggressive inlining: add
[MethodImpl(MethodImplOptions.AggressiveInlining)] to
LockSpeculativeRelayStates, HasSpeculativeRelayLock,
RestoreSpeculativeRelayStates, and ClearSpeculativeRelayLocks. Keep the
implementations unchanged and ensure the MethodImpl attribute is applied
consistently to these replay/tick loop helpers so the runtime can inline them on
the prediction path.
In `@Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs`:
- Line 299: The Simulate method in PredictedTransform is part of the prediction
tick hot path, so add the required
MethodImpl(MethodImplOptions.AggressiveInlining) attribute to this override.
Update the Simulate(ref PredictedTransformState state, float delta) declaration
to include the inlining attribute consistent with the coding guidelines for
hot-path methods.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs`:
- Line 160: The Simulate method in PredictedProjectile3D is a hot-path override
and must be marked for aggressive inlining. Add the required
MethodImpl(MethodImplOptions.AggressiveInlining) attribute to Simulate, keeping
the change localized to this hot-path method so it follows the coding guideline.
- Line 34: The private constants in PredictedProjectile3D, including
SafetyMarginFactor and the other flagged constant, do not follow the required
_camelCase prefix convention. Rename them to private field-style names with a
leading underscore and update any references within the class so the naming
matches the coding guidelines consistently.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs`:
- Line 313: The Simulate method in PredictedRigidbody is a hot-path override and
is missing the required inlining hint. Add the MethodImplAttribute with
MethodImplOptions.AggressiveInlining to Simulate(ref UnityRigidbodyState state,
float delta), matching the project’s hot-path guideline and keeping the
attribute on the method declaration so it remains easy to locate if signatures
change.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.cs`:
- Line 250: The Simulate method in PredictedRigidbody2D is a hot-path override
and needs the required inlining annotation. Add
[MethodImpl(MethodImplOptions.AggressiveInlining)] to Simulate, and make sure
the needed System.Runtime.CompilerServices import is available so the attribute
resolves correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b987e8f6-d2b8-4a58-b616-c0f268801770
⛔ Files ignored due to path filters (10)
Assets/PredictionTests/Bootstrap.unityis excluded by!**/*.unityAssets/PurrDiction/Examples/PlayerTest.prefabis excluded by!**/*.prefabAssets/PurrDiction/Examples/Projectile.prefabis excluded by!**/*.prefabAssets/PurrDictionTests/FallingSand.unityis excluded by!**/*.unityAssets/PurrDictionTests/Prefabs/SandboxPlayer.prefabis excluded by!**/*.prefabAssets/PurrDictionTests/SampleSceneUnity.unityis excluded by!**/*.unityAssets/Settings/PC_RPAsset.assetis excluded by!**/*.assetAssets/Settings/UniversalRenderPipelineGlobalSettings.assetis excluded by!**/*.assetProjectSettings/GraphicsSettings.assetis excluded by!**/*.assetProjectSettings/ProjectSettings.assetis excluded by!**/*.asset
📒 Files selected for processing (43)
Assets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.csAssets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs.metaAssets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.csAssets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/PolicyBallRig.csAssets/PredictionTests/Scripts/Scenarios/PolicyBallRig.cs.metaAssets/PredictionTests/Scripts/Scenarios/RelayProbe.csAssets/PredictionTests/Scripts/Scenarios/RelayProbe.cs.metaAssets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.csAssets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.csAssets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.csAssets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftProbe.csAssets/PredictionTests/Scripts/Scenarios/SoftProbe.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftProbe2D.csAssets/PredictionTests/Scripts/Scenarios/SoftProbe2D.cs.metaAssets/PurrDiction/Editor/PredictionPolicyDrawer.csAssets/PurrDiction/Editor/PredictionPolicyDrawer.cs.metaAssets/PurrDiction/Examples/ProjectileShooter.csAssets/PurrDiction/Examples/TestStateNode.csAssets/PurrDiction/Runtime/Core/AppliedCorrectionRing.csAssets/PurrDiction/Runtime/Core/AppliedCorrectionRing.cs.metaAssets/PurrDiction/Runtime/Core/DeterministicIdentity.csAssets/PurrDiction/Runtime/Core/ModulePredictedState.csAssets/PurrDiction/Runtime/Core/ModulePredictedState.cs.metaAssets/PurrDiction/Runtime/Core/PredictedIdentity.Lifecycle.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.Module.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.csAssets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.csAssets/PurrDiction/Runtime/Core/PredictedModuleState.csAssets/PurrDiction/Runtime/Core/PredictionManager.csAssets/PurrDiction/Runtime/Core/PredictionPolicy.csAssets/PurrDiction/Runtime/Core/PredictionPolicy.cs.metaAssets/PurrDiction/Runtime/Hierarchy/PredictedHierarchy.csAssets/PurrDiction/Runtime/Prebuilt/Rigidbody/RigidbodyShooter.csAssets/PurrDiction/Runtime/Transform/PredictedTransform.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.csPackages/packages-lock.jsonProjectSettings/ApplicationConstants.json
💤 Files with no reviewable changes (1)
- Packages/packages-lock.json
| public override void Setup(ScenarioContext ctx, NetworkManager manager) | ||
| { | ||
| var floor = new GameObject("RelayFloor"); | ||
| var floorCollider = floor.AddComponent<BoxCollider>(); | ||
| floorCollider.size = new Vector3(12f, 1f, 12f); | ||
| floor.transform.position = new Vector3(_rig.spawnPosition.x, -0.5f, _rig.spawnPosition.z); | ||
|
|
||
| var ball = new GameObject("RelayBall"); | ||
| ball.SetActive(false); | ||
| var rb = ball.AddComponent<Rigidbody>(); | ||
| rb.collisionDetectionMode = CollisionDetectionMode.ContinuousDynamic; | ||
| ball.AddComponent<SphereCollider>(); | ||
| ball.AddComponent<PredictedTransform>(); | ||
| var predictedRb = ball.AddComponent<PredictedRigidbody>(); | ||
| predictedRb.configuredPredictionPolicy = PredictionPolicy.ServerRelay; | ||
| ball.AddComponent<RelayProbe>(); | ||
| DontDestroyOnLoad(ball); | ||
|
|
||
| _prefabId = ctx.predictionManager.predictedPrefabs.prefabs.Count; | ||
| PredictionTestUtils.RegisterPrefab(ctx, ball); | ||
| _rig.ballPrefab = ball; | ||
| _rig.requiredPlayers = ctx.expectedConnections; | ||
| RelayProbe.ResetCounters(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the reviewed file and related scenario/base types
git ls-files 'Assets/PredictionTests/Scripts/Scenarios/*.cs' | sed -n '1,200p'
echo '--- ServerRelayScenario.cs ---'
sed -n '1,220p' Assets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs
echo '--- search for cleanup/teardown hooks ---'
rg -n "Teardown|Cleanup|Dispose|OnDestroy|DontDestroyOnLoad|Setup\(ScenarioContext|class .*Scenario" Assets/PredictionTests/Scripts -g '*.cs'
echo '--- scenario base types ---'
rg -n "abstract class .*Scenario|class .*ScenarioBase|virtual void Teardown|virtual void Cleanup|IDisposable" Assets/PredictionTests/Scripts -g '*.cs'Repository: PurrNet/PurrDiction
Length of output: 19994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Scenario.cs ---'
sed -n '1,220p' Assets/PredictionTests/Scripts/Scenario.cs
echo '--- ScenarioSequencer.cs ---'
sed -n '1,220p' Assets/PredictionTests/Scripts/ScenarioSequencer.cs
echo '--- PredictionBootstrap.cs (relevant lifecycle sections) ---'
sed -n '1,220p' Assets/PredictionTests/Scripts/PredictionBootstrap.cs
echo '--- PredictionBootstrap.cs (later lifecycle sections) ---'
sed -n '220,520p' Assets/PredictionTests/Scripts/PredictionBootstrap.csRepository: PurrNet/PurrDiction
Length of output: 23716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- search for teardown / destroy / scene reload paths ---'
rg -n "Destroy\\(|DestroyImmediate\\(|UnloadScene|LoadScene|SceneManager|OnDestroy\\(|Cleanup\\(|Teardown\\(" Assets/PredictionTests/Scripts -g '*.cs'
echo '--- end of PredictionBootstrap.cs ---'
sed -n '520,760p' Assets/PredictionTests/Scripts/PredictionBootstrap.cs
echo '--- related scenario pattern examples ---'
sed -n '1,120p' Assets/PredictionTests/Scripts/Scenarios/BounceScenario.cs
sed -n '1,120p' Assets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.cs
sed -n '1,120p' Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.csRepository: PurrNet/PurrDiction
Length of output: 13516
Destroy the scenario's temporary objects. floor has no teardown path, and ball is marked DontDestroyOnLoad, so both stay alive after this scenario completes. Add a cleanup hook to destroy them when the scenario ends.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs` around lines
17 - 40, The ServerRelayScenario.Setup method creates temporary GameObjects for
the relay floor and ball, but neither has a teardown path, and the ball is also
marked DontDestroyOnLoad. Add scenario cleanup so these objects are destroyed
when the scenario completes, preferably by storing references in
ServerRelayScenario and disposing them in the scenario’s end/teardown hook after
Setup.
| internal struct ModulePredictedState : IPredictedData<ModulePredictedState> | ||
| { | ||
| public bool wasOnSimulationStartCalled; | ||
|
|
||
| public void Dispose() { } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add IPackedAuto to the module prediction metadata state.
ModulePredictedState is serialized via Packer<ModulePredictedState>, but it does not implement the auto-packing marker required for predicted state types.
Proposed fix
- internal struct ModulePredictedState : IPredictedData<ModulePredictedState>
+ internal struct ModulePredictedState : IPredictedData<ModulePredictedState>, IPackedAutoAs per coding guidelines, “State types implementing IPredictedData must also implement IDisposable and IPackedAuto for auto-serialization codegen.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal struct ModulePredictedState : IPredictedData<ModulePredictedState> | |
| { | |
| public bool wasOnSimulationStartCalled; | |
| public void Dispose() { } | |
| } | |
| internal struct ModulePredictedState : IPredictedData<ModulePredictedState>, IPackedAuto | |
| { | |
| public bool wasOnSimulationStartCalled; | |
| public void Dispose() { } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs` around lines 7 - 12,
`ModulePredictedState` is missing the auto-packing marker required for
predicted-state serialization. Update the `ModulePredictedState` struct so it
implements `IPackedAuto` alongside `IPredictedData<ModulePredictedState>` and
`IDisposable`, keeping the existing `Dispose` member intact. Use the
`ModulePredictedState` type in
`Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs` as the place to make
this interface change so `Packer<ModulePredictedState>` can generate
auto-serialization correctly.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/PurrDiction/Runtime/Core/DeterministicIdentity.cs (1)
148-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale interpolation buffer when
preserveInterpolationis true.
ResetStateToInitialState()andGetLatestUnityState()resetfullPredictedState, but whenpreserveInterpolationis true theelse if (!preserveInterpolation)guard skips_interpolatedState.Teleport(...). UnlikePredictedIdentityStatefull,DeterministicIdentityhas noUsesSoftCorrectionTimeline()early-return path, sopreserveInterpolationbeing true always leaves the view buffer on stale pre-reset samples.🔧 Proposed fix: always teleport after state reset
if (_interpolatedState == null) { _interpolatedState = new InterpolatedWithDispose<FULL_STATE<STATE>>( FULLInterpolate, 1f / world.tickRate, fullPredictedState.DeepCopy(), interpolationBuffer); } - else if (!preserveInterpolation) + else _interpolatedState.Teleport(fullPredictedState.DeepCopy());Since
DeterministicIdentityalways resets state inSetup, there is no scenario where preserving old interpolation samples is correct. ThepreserveInterpolationflag can also be removed if it's not used elsewhere in this method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/DeterministicIdentity.cs` around lines 148 - 179, The interpolation buffer in DeterministicIdentity.Setup is left stale when preserveInterpolation is true because _interpolatedState.Teleport(fullPredictedState.DeepCopy()) is skipped after ResetStateToInitialState() and GetLatestUnityState() refresh the state. Update the Setup flow so the interpolated buffer is always teleported to the newly reset fullPredictedState (or remove preserveInterpolation if it is no longer needed), and keep the fix localized around _interpolatedState, ResetStateToInitialState(), and GetLatestUnityState().
♻️ Duplicate comments (1)
Assets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.cs (1)
160-182: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
preserveInterpolationissue still present — stale interpolation buffer for non-SoftCorrection policies.The past review suggested changing
else if (!preserveInterpolation)toelse if (!preserveSoftCorrection), but the current code still usespreserveInterpolation. WhenpreserveInterpolationis true andUsesSoftCorrectionTimeline()is false, the state is reset viaResetStateToInitialState()+GetLatestUnityState()but_interpolatedState.Teleport(...)is skipped, leaving the view buffer on stale pre-setup samples.🔧 Proposed fix
bool preserveInterpolation = preservesStateOnSetup && _interpolatedState != null && _stateHistory != null; + bool preserveSoftCorrection = preserveInterpolation && UsesSoftCorrectionTimeline(); - if (preserveInterpolation && UsesSoftCorrectionTimeline()) + if (preserveSoftCorrection) { fullPredictedState.prediction.owner = owner; return; } @@ - else if (!preserveInterpolation) + else if (!preserveSoftCorrection) _interpolatedState.Teleport(fullPredictedState.DeepCopy());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.cs` around lines 160 - 182, The interpolation reset logic in PredictedIdentityStatefull.Setup still uses preserveInterpolation for the teleport branch, which can skip refreshing _interpolatedState after ResetStateToInitialState() and GetLatestUnityState() when the soft-correction timeline is not active. Update the conditional around _interpolatedState.Teleport(...) to use the soft-correction preservation check instead of preserveInterpolation, and keep the early return in sync with UsesSoftCorrectionTimeline() so the view buffer is always refreshed for non-soft-correction policies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Assets/PurrDiction/Runtime/Core/DeterministicIdentity.cs`:
- Around line 148-179: The interpolation buffer in DeterministicIdentity.Setup
is left stale when preserveInterpolation is true because
_interpolatedState.Teleport(fullPredictedState.DeepCopy()) is skipped after
ResetStateToInitialState() and GetLatestUnityState() refresh the state. Update
the Setup flow so the interpolated buffer is always teleported to the newly
reset fullPredictedState (or remove preserveInterpolation if it is no longer
needed), and keep the fix localized around _interpolatedState,
ResetStateToInitialState(), and GetLatestUnityState().
---
Duplicate comments:
In `@Assets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.cs`:
- Around line 160-182: The interpolation reset logic in
PredictedIdentityStatefull.Setup still uses preserveInterpolation for the
teleport branch, which can skip refreshing _interpolatedState after
ResetStateToInitialState() and GetLatestUnityState() when the soft-correction
timeline is not active. Update the conditional around
_interpolatedState.Teleport(...) to use the soft-correction preservation check
instead of preserveInterpolation, and keep the early return in sync with
UsesSoftCorrectionTimeline() so the view buffer is always refreshed for
non-soft-correction policies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7628c826-b4c5-461e-a5f9-519f23d4f33c
📒 Files selected for processing (26)
Assets/PurrDiction/Editor/PredictionPolicyDrawer.csAssets/PurrDiction/Runtime/AssemblyInfo.csAssets/PurrDiction/Runtime/Core/DeterministicIdentity.csAssets/PurrDiction/Runtime/Core/DeterministicIdentityWithInput.csAssets/PurrDiction/Runtime/Core/FULL_STATE.csAssets/PurrDiction/Runtime/Core/History.csAssets/PurrDiction/Runtime/Core/ModulePredictedState.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.Module.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.csAssets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.csAssets/PurrDiction/Runtime/Core/PredictedIdentityWithInput.csAssets/PurrDiction/Runtime/Core/PredictedModule.csAssets/PurrDiction/Runtime/Core/PredictedModuleState.csAssets/PurrDiction/Runtime/Core/PredictionPolicyScope.csAssets/PurrDiction/Runtime/Core/PredictionPolicyScope.cs.metaAssets/PurrDiction/Runtime/Core/PredictionPolicySource.csAssets/PurrDiction/Runtime/Core/PredictionPolicySource.cs.metaAssets/PurrDiction/Runtime/Transform/PredictedTransform.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.csAssets/PurrDictionTests/EditorTests.metaAssets/PurrDictionTests/EditorTests/PredictionOwnershipTests.csAssets/PurrDictionTests/EditorTests/PredictionOwnershipTests.cs.metaAssets/PurrDictionTests/EditorTests/PurrNet.Prediction.EditorTests.asmdefAssets/PurrDictionTests/EditorTests/PurrNet.Prediction.EditorTests.asmdef.meta
✅ Files skipped from review due to trivial changes (6)
- Assets/PurrDictionTests/EditorTests/PurrNet.Prediction.EditorTests.asmdef.meta
- Assets/PurrDiction/Runtime/Core/PredictionPolicySource.cs
- Assets/PurrDiction/Runtime/Core/PredictionPolicyScope.cs.meta
- Assets/PurrDictionTests/EditorTests/PredictionOwnershipTests.cs.meta
- Assets/PurrDictionTests/EditorTests.meta
- Assets/PurrDiction/Runtime/Core/PredictionPolicySource.cs.meta
🚧 Files skipped from review as they are similar to previous changes (8)
- Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs
- Assets/PurrDiction/Runtime/Core/PredictedIdentity.Module.cs
- Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs
- Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs
- Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.cs
- Assets/PurrDiction/Runtime/Core/PredictedModuleState.cs
- Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs
- Assets/PurrDiction/Runtime/Core/PredictedIdentity.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32a316bc6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| } | ||
|
|
||
| system.RunRollback(locked.tick); |
There was a problem hiding this comment.
When a client has any ServerRelay (or remote PredictedIfOwned) system, this restores the saved lock but leaves it in _speculativeRelayLocks; the next tick HasSpeculativeRelayLock then prevents taking a fresh snapshot, so every later RestoreSpeculativeRelayStates() rolls that object back to the first locked tick and can overwrite newer server-relayed state. Remove/clear each lock after it has been restored so the next tick is locked against the current authoritative pose.
Useful? React with 👍 / 👎.
| else if (!preserveInterpolation) | ||
| _interpolatedState.Teleport(fullPredictedState.DeepCopy()); |
There was a problem hiding this comment.
For deterministic identities, SoftCorrection is normalized away, so a pooled same-ID reuse that sets preservesStateOnSetup has just reset fullPredictedState and cleared history above but this branch skips teleporting the interpolation buffer. That leaves the reused deterministic object rendering from the previous pooled lifetime after rollback delete/recreate; only the soft-correction preservation path should skip the teleport.
Useful? React with 👍 / 👎.
| if (!PreservesSoftCorrectionInstance(instance, instanceId)) | ||
| go.transform.SetPositionAndRotation(position, rotation); |
There was a problem hiding this comment.
When a pooled prefab has mixed policies under the same PredictedObjectID (for example a fully predicted root plus a soft-corrected child), PreservesSoftCorrectionInstance returns true for any soft child and this skips resetting the root transform before re-registration. During rollback-created spawns that do not immediately read a fresh transform state, the full-prediction root is initialized from the stale pooled pose instead of the requested spawn pose; only skip this root reposition when the root/pose-owning identity being preserved is actually soft-corrected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/PurrDiction/Runtime/Hierarchy/PredictedHierarchy.cs (1)
192-200: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider adding
[MethodImpl(MethodImplOptions.AggressiveInlining)]toPreservesSoftCorrectionRootPose.This is a small method called during object creation in simulation/rollback paths. Per coding guidelines, hot path methods should use
AggressiveInlining, which is especially beneficial in IL2CPP builds where JIT inlining is unavailable.♻️ Proposed refactor
+using System.Runtime.CompilerServices; + private bool PreservesSoftCorrectionRootPose(GameObject instance, PredictedObjectID instanceId) { if (!predictionManager.isReplaying || !instance)+[MethodImpl(MethodImplOptions.AggressiveInlining)] private bool PreservesSoftCorrectionRootPose(GameObject instance, PredictedObjectID instanceId) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PurrDiction/Runtime/Hierarchy/PredictedHierarchy.cs` around lines 192 - 200, Add MethodImplOptions.AggressiveInlining to the PreservesSoftCorrectionRootPose method, including the necessary System.Runtime.CompilerServices import if absent, while preserving its existing logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Assets/PurrDiction/Runtime/Hierarchy/PredictedHierarchy.cs`:
- Around line 192-200: Add MethodImplOptions.AggressiveInlining to the
PreservesSoftCorrectionRootPose method, including the necessary
System.Runtime.CompilerServices import if absent, while preserving its existing
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c14cd0ae-3306-4bd2-98e8-ce4ea3c108dc
⛔ Files ignored due to path filters (4)
Assets/PurrDiction/Examples/PlayerTest.prefabis excluded by!**/*.prefabAssets/PurrDiction/Examples/Projectile.prefabis excluded by!**/*.prefabAssets/PurrDictionTests/FallingSand.unityis excluded by!**/*.unityAssets/PurrDictionTests/SampleSceneUnity.unityis excluded by!**/*.unity
📒 Files selected for processing (37)
Assets/PredictionTests/README.mdAssets/PredictionTests/Scripts/Scenarios/BounceProbe.csAssets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.csAssets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs.metaAssets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.csAssets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/PolicyBallRig.cs.metaAssets/PredictionTests/Scripts/Scenarios/RelayProbe.csAssets/PredictionTests/Scripts/Scenarios/RelayProbe.cs.metaAssets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.csAssets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.csAssets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.csAssets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftProbe.csAssets/PredictionTests/Scripts/Scenarios/SoftProbe.cs.metaAssets/PredictionTests/Scripts/Scenarios/SoftProbe2D.csAssets/PredictionTests/Scripts/Scenarios/SoftProbe2D.cs.metaAssets/PurrDiction/Editor/PredictionPolicyDrawer.csAssets/PurrDiction/Runtime/Core/DeterministicIdentity.csAssets/PurrDiction/Runtime/Core/ModulePredictedState.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.Module.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.csAssets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.csAssets/PurrDiction/Runtime/Core/PredictedModuleState.csAssets/PurrDiction/Runtime/Core/PredictionManager.csAssets/PurrDiction/Runtime/Core/PredictionPolicy.csAssets/PurrDiction/Runtime/Core/PredictionPolicyScope.csAssets/PurrDiction/Runtime/Hierarchy/PredictedHierarchy.csAssets/PurrDiction/Runtime/Transform/PredictedTransform.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.csAssets/PurrDictionTests/EditorTests.metaAssets/PurrDictionTests/EditorTests/PredictionOwnershipTests.csAssets/PurrDictionTests/EditorTests/PurrNet.Prediction.EditorTests.asmdef.meta
💤 Files with no reviewable changes (4)
- Assets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs
- Assets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.cs
- Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs
- Assets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs
✅ Files skipped from review due to trivial changes (12)
- Assets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.cs.meta
- Assets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs.meta
- Assets/PredictionTests/README.md
- Assets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.cs.meta
- Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs.meta
- Assets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.cs.meta
- Assets/PredictionTests/Scripts/Scenarios/SoftProbe2D.cs.meta
- Assets/PredictionTests/Scripts/Scenarios/PolicyBallRig.cs.meta
- Assets/PurrDictionTests/EditorTests/PurrNet.Prediction.EditorTests.asmdef.meta
- Assets/PredictionTests/Scripts/Scenarios/RelayProbe.cs.meta
- Assets/PurrDictionTests/EditorTests.meta
- Assets/PredictionTests/Scripts/Scenarios/SoftProbe.cs.meta
🚧 Files skipped from review as they are similar to previous changes (17)
- Assets/PurrDiction/Runtime/Core/ModulePredictedState.cs
- Assets/PurrDiction/Runtime/Core/PredictionPolicy.cs
- Assets/PredictionTests/Scripts/Scenarios/RelayProbe.cs
- Assets/PredictionTests/Scripts/Scenarios/SoftProbe.cs
- Assets/PurrDiction/Runtime/Core/PredictionPolicyScope.cs
- Assets/PredictionTests/Scripts/Scenarios/OwnedRelayProbe.cs
- Assets/PredictionTests/Scripts/Scenarios/SoftProbe2D.cs
- Assets/PurrDiction/Runtime/Core/PredictedIdentity.Module.cs
- Assets/PurrDiction/Runtime/Transform/PredictedTransform.cs
- Assets/PurrDiction/Runtime/Core/PredictedIdentityStatefull.cs
- Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs
- Assets/PurrDiction/Runtime/UnityPhysics/PredictedProjectile3D.cs
- Assets/PurrDiction/Runtime/Core/PredictedIdentity.cs
- Assets/PurrDiction/Runtime/Core/DeterministicIdentity.cs
- Assets/PurrDiction/Runtime/Core/PredictionManager.cs
- Assets/PurrDiction/Runtime/Core/PredictedModuleState.cs
- Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody2D.cs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs (1)
15-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTemporary scenario objects are never destroyed.
SoftFloor(active scene object) andSoftBallhave no teardown path. If later scenarios spawn their own floors/balls, leftover colliders from this scenario can perturb their physics and lead to flaky results. Consider storing references and destroying them in a scenario end/teardown hook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs` around lines 15 - 34, The SoftCorrectionScenario creates persistent SoftFloor and SoftBall objects without cleanup. Store references to both objects in Setup, then destroy them in the scenario’s teardown/end hook, ensuring the registered ball prefab and associated components are removed before later scenarios run.
🧹 Nitpick comments (1)
Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs (1)
69-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
20fdivergence timeout is inconsistent with_timeout. Every other wait in this scenario uses the serialized_timeout; this one uses a magic literal. Extract it to a serialized field/const so it can be tuned alongside the others.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs` around lines 69 - 79, The divergence wait in the scenario currently hardcodes a 20-second timeout instead of using configurable timing. Update the wait inside the scenario method containing the SoftProbe divergence check to use the scenario’s serialized _timeout field, or introduce a dedicated serialized timeout field/constant if this wait needs independent tuning, and remove the 20f literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs`:
- Around line 15-34: The SoftCorrectionScenario creates persistent SoftFloor and
SoftBall objects without cleanup. Store references to both objects in Setup,
then destroy them in the scenario’s teardown/end hook, ensuring the registered
ball prefab and associated components are removed before later scenarios run.
---
Nitpick comments:
In `@Assets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.cs`:
- Around line 69-79: The divergence wait in the scenario currently hardcodes a
20-second timeout instead of using configurable timing. Update the wait inside
the scenario method containing the SoftProbe divergence check to use the
scenario’s serialized _timeout field, or introduce a dedicated serialized
timeout field/constant if this wait needs independent tuning, and remove the 20f
literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ecd188c-015d-4624-87c6-b72566b26c87
⛔ Files ignored due to path filters (1)
Assets/PredictionTests/Bootstrap.unityis excluded by!**/*.unity
📒 Files selected for processing (13)
.github/workflows/prediction-tests.ymlAssets/PredictionTests/README.mdAssets/PredictionTests/Scripts/PredictionBootstrap.csAssets/PredictionTests/Scripts/Scenario.csAssets/PredictionTests/Scripts/ScenarioSequencer.csAssets/PredictionTests/Scripts/Scenarios/BounceRig.csAssets/PredictionTests/Scripts/Scenarios/BounceScenario.csAssets/PredictionTests/Scripts/Scenarios/OwnedRelayScenario.csAssets/PredictionTests/Scripts/Scenarios/PolicyBallRig.csAssets/PredictionTests/Scripts/Scenarios/ServerRelayScenario.csAssets/PredictionTests/Scripts/Scenarios/SoftCorrection2DScenario.csAssets/PredictionTests/Scripts/Scenarios/SoftCorrectionScenario.csAssets/PurrDictionTests/Benchmarks/Editor/PredictionScenarioBenchmarkRunner.cs
✅ Files skipped from review due to trivial changes (1)
- Assets/PredictionTests/README.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f475ad485e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| internal override void Setup(NetworkManager manager, PredictionManager world, PredictedComponentID id, PlayerID? owner) | ||
| { | ||
| base.Setup(manager, world, id, owner); |
There was a problem hiding this comment.
When a pooled 3D body that previously used ServerRelay is re-registered without reset (for example a rollback reuse through PredictedHierarchy), it can still be kinematic from ForceRelayKinematic(). base.Setup() reaches GetLatestUnityState() before RestoreDefaultPhysicsMode() runs below, so the new lifetime's currentState/initial history can record the stale kinematic mode and zero velocities, and a later rollback can turn a fully-predicted respawn back into a relay/kinematic body. Restore/clear the relay physics mode before calling the base state setup.
Useful? React with 👍 / 👎.
|
|
||
| internal override void Setup(NetworkManager manager, PredictionManager world, PredictedComponentID id, PlayerID? owner) | ||
| { | ||
| base.Setup(manager, world, id, owner); |
There was a problem hiding this comment.
When a pooled 2D body is reused without reset after a ServerRelay lifetime, it may still have bodyType = Kinematic. Because base.Setup() reads the Unity state before RestoreDefaultPhysicsMode() below, the new spawn's currentState/history can capture that stale kinematic body type and zero velocities, so subsequent rollback can incorrectly keep a full-prediction respawn kinematic. Restore the default body type before the base setup reads state.
Useful? React with 👍 / 👎.
| _probePrefab.GetComponent<GenericSoftCorrectionProbe>().configuredPredictionPolicy = | ||
| PredictionPolicy.SoftCorrection; |
There was a problem hiding this comment.
This scenario never actually exercises the generic SoftCorrection path: GenericSoftCorrectionProbe does not override supportsSoftCorrection, so assigning configuredPredictionPolicy = PredictionPolicy.SoftCorrection is normalized to FullPrediction immediately by the setter before the prefab is registered. The client then converges via normal rollback and the regression can pass even if generic soft-correction state still never applies verified snapshots; have the probe opt in explicitly (or assert the resolved policy) so the test covers the intended mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01c7e39c3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| predictionManager = world; | ||
| sceneId = world.sceneId; | ||
| SetOwner(owner, false); | ||
| SetPredictionPolicy(ResolvePredictionPolicy()); |
There was a problem hiding this comment.
When a pooled identity keeps predictionPolicy == ServerRelay from its previous lifetime but is reset and re-registered under FullPrediction or SoftCorrection, this call fires OnPredictionPolicyChanged before derived setup has rebuilt currentState. For PredictedRigidbody/PredictedRigidbody2D, the ServerRelay→non-relay side effect restores physics from the disposed/default currentState, so the new spawn can capture zero velocities and, for 3D bodies, useGravity = false as its initial state. Reset the applied policy during ResetState or defer policy side effects until after derived setup refreshes state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23aadf0d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| if (predictionPolicy != PredictionPolicy.PredictedIfOwned) | ||
| return predictionPolicy; | ||
| return IsOwner() ? PredictionPolicy.FullPrediction : PredictionPolicy.ServerRelay; |
There was a problem hiding this comment.
When PredictedIfOwned is applied to deterministic/stateless identities and ownership changes after spawn (for example through PredictionManager.SetOwnership), clients keep the old owner because deterministic WriteCurrentState only sends validation data and never serializes PredictedIdentityState.owner. This branch then resolves the effective policy from stale local ownership, so the old owner can keep full-predicting while the new owner is relayed; either send owner metadata for these identities or disallow this policy where owner changes are not synchronized.
Useful? React with 👍 / 👎.
| { | ||
| internal sealed class AppliedCorrectionRing<T> where T : struct | ||
| { | ||
| private const int Size = 128; |
There was a problem hiding this comment.
With SoftCorrection under high latency or a long client stall, authoritative ticks can arrive more than 128 ticks after their predicted baseline while the state history still has that tick. The fixed ring then falls back to a later baseline (or none), so corrections already applied between the verified tick and the retained baseline are not subtracted and the same pose/velocity error is applied again, producing overshoot instead of convergence; keep these totals for at least the same tick window as prediction history.
Useful? React with 👍 / 👎.
| bool preserveSoftState = preserveState && component.UsesSoftCorrectionTimeline(); | ||
|
|
||
| if (!preserveSoftState) |
There was a problem hiding this comment.
When a same-ID pooled object was SoftCorrection in its previous lifetime but is re-registered after its scope/config changed to FullPrediction or ServerRelay, this uses the stale old predictionPolicy to set preserveSoftState before Setup() resolves the new policy. That skips OnPreSetup() even though the object will be initialized as non-soft, so components such as PredictedRigidbody can read stale pooled velocities/modes into the new lifetime; base the preservation decision on the policy that will actually be applied for this registration or still run pre-setup when it changes away from soft.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2badeef22a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return instance.TryGetComponent(out PredictedTransform predictedTransform) && | ||
| predictedTransform.id.objectId.Equals(instanceId) && | ||
| predictedTransform.predictionPolicy == PredictionPolicy.SoftCorrection; |
There was a problem hiding this comment.
Fresh evidence after the setup-preservation change: this predicate still reads the pooled transform's stale applied predictionPolicy before RegisterInstance resolves the incoming policy. When a same-ID object was SoftCorrection last lifetime but its scope/override now resolves to FullPrediction or ServerRelay during replay, this returns true and skips SetPositionAndRotation, so setup captures the old pooled pose instead of the requested spawn pose. Use ResolvePredictionPolicyForSetup() for this decision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9c3d44c22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bool preserveSoftState = preserveState && | ||
| component.ResolvePredictionPolicyForSetup() == PredictionPolicy.SoftCorrection; |
There was a problem hiding this comment.
When a same-ID tick-pooled object was relayed or fully predicted in its previous lifetime but its scope/config now resolves to SoftCorrection, this sets preserveSoftState based only on the incoming policy. That passes preservesStateOnSetup into setup, so the soft-preserve branch skips reinitializing state/history/modules from the new spawn and the new soft object can inherit the previous lifetime's runtime state. Gate preservation on the previously applied policy being SoftCorrection as well.
Useful? React with 👍 / 👎.
| return instance.TryGetComponent(out PredictedTransform predictedTransform) && | ||
| predictedTransform.id.objectId.Equals(instanceId) && | ||
| predictedTransform.ResolvePredictionPolicyForSetup() == PredictionPolicy.SoftCorrection; |
There was a problem hiding this comment.
When a same-ID pooled object switches into SoftCorrection during replay after a non-soft lifetime, this returns true even though there is no soft live pose to preserve. The caller then skips SetPositionAndRotation before registration, so setup reads the old pooled transform instead of the requested spawn pose. Require the previous applied policy to already be SoftCorrection before skipping the pose reset.
Useful? React with 👍 / 👎.
| if (!gameObject.activeInHierarchy) | ||
| return; |
There was a problem hiding this comment.
When a registered scoped subtree is deactivated at runtime without being unregistered (for example via PredictedGameObject.SetActive(false)), this early return leaves descendants on the old scoped policy even though TryGetPredictionPolicyScope will no longer consider the inactive scope active. PredictionManager continues to drive registered identities from its _systems list, so relayed/owned/soft side effects can remain stale until the scope is re-enabled; only the pooled-unregister path should suppress the fallback refresh.
Useful? React with 👍 / 👎.
| RestoreDefaultPhysicsMode(); | ||
| _appliedKinematicPolicy = PredictionPolicy.FullPrediction; | ||
| ApplyEffectiveKinematic(); |
There was a problem hiding this comment.
When a same-ID soft-correction Rigidbody is reused from the tick pool, base.Setup() can take the preserve-state path and keep the previous live currentState/history, but this still restores the prefab default physics mode afterward. If the preserved soft body had been verified with a non-default isKinematic state, the Unity Rigidbody is reset away from the preserved state before the next tick; skip this default restore on the soft-preserve path or restore from currentState instead.
Useful? React with 👍 / 👎.
| RestoreDefaultPhysicsMode(); | ||
| _appliedKinematicPolicy = PredictionPolicy.FullPrediction; | ||
| ApplyEffectiveKinematic(); |
There was a problem hiding this comment.
When a same-ID soft-correction 2D Rigidbody is reused from the tick pool, the generic setup can preserve the old soft state/history, but this unconditionally restores the prefab bodyType afterward. If the previous soft lifetime had already accepted an authoritative non-default body type, the live Rigidbody2D no longer matches the preserved currentState for the reused lifetime; keep the preserved body type or restore from currentState when preservesStateOnSetup is active.
Useful? React with 👍 / 👎.
|
🎉 This PR is included in version 1.3.0-beta.44 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit