Added Unreliable mode to Prediction#48
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThreads a ChangesReliability-aware serialization
Editor & Config
Sequence DiagramsequenceDiagram
participant PM as PredictionManager
participant RPC as SendFrameToRemote (dispatcher)
participant RelCh as SendFrameToRemoteReliable
participant UnrelCh as SendFrameToRemoteUnreliable
participant Identity as PredictedIdentity
participant DeltaMod as DeltaModule
PM->>RPC: SendFrame(isReliable)
alt isReliable == true
RPC->>RelCh: ReliableOrdered RPC
RelCh->>PM: Enqueue delta (clientTick)
else isReliable == false
RPC->>UnrelCh: UnreliableSequenced RPC
UnrelCh->>PM: Enqueue delta (clientTick)
end
Identity->>DeltaMod: ReadState(..., reliable)
alt reliable == true
DeltaMod->>DeltaMod: ReadReliable() with changed-flag
else
DeltaMod->>DeltaMod: Read() with changed semantics
end
DeltaMod-->>Identity: Deserialized state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Assets/PurrDiction/Runtime/Core/DeltaKey.cs (1)
8-9: Rename the new private fields to_camelCase.
idandscenein both structs don't follow the repo's C# private-field convention. Renaming them to_id/_scenewill keep these new core types consistent with the rest of the runtime code.As per coding guidelines "Private fields must use _camelCase prefix naming convention".
Also applies to: 33-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/PurrDiction/Runtime/Core/DeltaKey.cs` around lines 8 - 9, The private readonly fields named id and scene in the structs (types PredictedComponentID and SceneID defined in DeltaKey.cs) must be renamed to follow the repo convention: change id -> _id and scene -> _scene; update all references (constructors, property getters, Equals/GetHashCode/ToString implementations, and any usage within the same file) to use the new names so compilation and behavior remain identical. Ensure the fields remain readonly and visibility unchanged, and run a quick compile to catch any missed references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/PurrDiction/Runtime/Core/PredictedModule.cs`:
- Around line 100-112: The new bool reliable parameter on the protected abstract
methods breaks external subclasses; restore the original protected abstract
signatures WriteState(PlayerID, BitPacker, DeltaModule) and ReadState(ulong,
BitPacker, DeltaModule) so existing implementations keep compiling, then add new
protected virtual overloads WriteState(PlayerID, BitPacker, DeltaModule, bool
reliable) and ReadState(ulong, BitPacker, DeltaModule, bool reliable) that by
default call the original abstract methods (ignoring reliable) to provide the
extended behavior; update the internal entry points WriteStateInternal and
ReadStateInternal to call the new overloads so internal code can pass reliable
without forcing a breaking change on subclasses.
In `@Assets/PurrDiction/Runtime/Core/PredictionManager.cs`:
- Around line 913-917: The queued frame payloads are being serialized with the
sender's isReliable but deserialized later using the receiver's current
isReliable, which can corrupt parsing; fix this by adding a reliability flag to
the FrameDelta payload (e.g., add a reliable boolean/member to FrameDelta when
created in
SendFrameToRemote/SendFrameToRemoteReliable/SendFrameToRemoteUnreliable) and
propagate that flag through RollbackToFrame, RunReadState, and ReadInput (use
the passed-in reliable value instead of reading PredictionManager.isReliable) so
the same FrameChannelMode used when writing is used when reading; update the
dequeue/parse code paths that currently access FrameDelta.packer and clientTick
to also read FrameDelta.reliable and use it to select the correct delta read
path.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs`:
- Around line 136-145: The delta key is constructed with
predictionManager.sceneId when writing (in the paths creating
DeltaKey<UnityRigidbodyCompressedState> and DeltaKey<UnityRigidbodyHalfState>)
but read back using sceneId elsewhere, causing mismatched hashes; update the
write and/or read paths so both use the same source (either always
predictionManager.sceneId or always the local sceneId variable) for DeltaKey<T>
construction (affecting DeltaKey<UnityRigidbodyCompressedState>,
DeltaKey<UnityRigidbodyHalfState> and their corresponding read paths) so the
keys match across Write/WriteReliable and Read.
---
Nitpick comments:
In `@Assets/PurrDiction/Runtime/Core/DeltaKey.cs`:
- Around line 8-9: The private readonly fields named id and scene in the structs
(types PredictedComponentID and SceneID defined in DeltaKey.cs) must be renamed
to follow the repo convention: change id -> _id and scene -> _scene; update all
references (constructors, property getters, Equals/GetHashCode/ToString
implementations, and any usage within the same file) to use the new names so
compilation and behavior remain identical. Ensure the fields remain readonly and
visibility unchanged, and run a quick compile to catch any missed references.
🪄 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: df6bd57a-4f2d-419f-89f1-81d080807c94
📒 Files selected for processing (17)
Assets/PurrDiction/Editor/PredictionManagerEditor.csAssets/PurrDiction/Editor/PredictionManagerEditor.cs.metaAssets/PurrDiction/Runtime/Core/DeltaKey.csAssets/PurrDiction/Runtime/Core/DeltaKey.cs.metaAssets/PurrDiction/Runtime/Core/DeterministicIdentity.csAssets/PurrDiction/Runtime/Core/DeterministicIdentityWithInput.csAssets/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/PredictedModule.csAssets/PurrDiction/Runtime/Core/PredictedModuleState.csAssets/PurrDiction/Runtime/Core/PredictionManager.csAssets/PurrDiction/Runtime/Core/StatelessPredictedIdentity.csAssets/PurrDiction/Runtime/Core/StaticPredictedIdentity.csAssets/PurrDiction/Runtime/Transform/PredictedTransform.csAssets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs
💤 Files with no reviewable changes (1)
- Assets/PurrDiction/Runtime/Core/DeterministicIdentityWithInput.cs
| internal bool WriteStateInternal(PlayerID receiver, BitPacker packer, DeltaModule deltaModule, bool reliable) => WriteState(receiver, packer, deltaModule, reliable); | ||
|
|
||
| /// <summary> | ||
| /// Serializes the current state of the module for network transmission. | ||
| /// </summary> | ||
| /// <returns>True if any data was written (i.e., state changed), otherwise False.</returns> | ||
| protected abstract bool WriteState(PlayerID receiver, BitPacker packer, DeltaModule deltaModule); | ||
| internal void ReadStateInternal(ulong tick, BitPacker packer, DeltaModule deltaModule) => ReadState(tick, packer, deltaModule); | ||
| protected abstract bool WriteState(PlayerID receiver, BitPacker packer, DeltaModule deltaModule, bool reliable); | ||
| internal void ReadStateInternal(ulong tick, BitPacker packer, DeltaModule deltaModule, bool reliable) => ReadState(tick, packer, deltaModule, reliable); | ||
|
|
||
| /// <summary> | ||
| /// Deserializes incoming network data and applies it to the module's state. | ||
| /// </summary> | ||
| protected abstract void ReadState(ulong tick, BitPacker packer, DeltaModule deltaModule); | ||
| protected abstract void ReadState(ulong tick, BitPacker packer, DeltaModule deltaModule, bool reliable); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the full PredictedModule.cs file
fd -t f "PredictedModule\.cs$" -x wc -l {}Repository: PurrNet/PurrDiction
Length of output: 120
🏁 Script executed:
# Get the actual file to see the class declaration and method signatures
fd -t f "PredictedModule\.cs$" -x cat -n {}Repository: PurrNet/PurrDiction
Length of output: 8387
🏁 Script executed:
# Check for any version or changelog files that might document breaking changes
fd -t f "CHANGELOG\|VERSION\|ReleaseNotes" -iRepository: PurrNet/PurrDiction
Length of output: 45
🏁 Script executed:
# Search for references to breaking changes or migration guides
rg -i "breaking|migration|version" --max-count 20 -t csRepository: PurrNet/PurrDiction
Length of output: 27936
🏁 Script executed:
# Check if PredictedModule is sealed or if there are other constraints
rg "class PredictedModule|abstract.*PredictedModule" -A 5 -B 2 -t csRepository: PurrNet/PurrDiction
Length of output: 1545
This breaks the public PredictedModule extension contract.
Adding bool reliable to the protected abstract WriteState and ReadState method signatures will cause any external PredictedModule subclass to stop compiling. Since PredictedModule is explicitly public abstract, custom module implementations are a supported extension point and this source-breaking change requires an explicit migration path or release-note callout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/PurrDiction/Runtime/Core/PredictedModule.cs` around lines 100 - 112,
The new bool reliable parameter on the protected abstract methods breaks
external subclasses; restore the original protected abstract signatures
WriteState(PlayerID, BitPacker, DeltaModule) and ReadState(ulong, BitPacker,
DeltaModule) so existing implementations keep compiling, then add new protected
virtual overloads WriteState(PlayerID, BitPacker, DeltaModule, bool reliable)
and ReadState(ulong, BitPacker, DeltaModule, bool reliable) that by default call
the original abstract methods (ignoring reliable) to provide the extended
behavior; update the internal entry points WriteStateInternal and
ReadStateInternal to call the new overloads so internal code can pass reliable
without forcing a breaking change on subclasses.
| private void SendFrameToRemote(PlayerID player, ulong localTick, BitPackerWithLength delta) | ||
| { | ||
| if (isReliable) SendFrameToRemoteReliable(player, localTick, delta); | ||
| else SendFrameToRemoteUnreliable(player, localTick, delta); | ||
| } |
There was a problem hiding this comment.
Store the frame's reliability mode with the queued payload.
These RPCs choose the wire format from the sender's isReliable, but RollbackToFrame(...) later parses the queued bytes using the receiver's current isReliable. FrameDelta only keeps packer and clientTick, so a server/client FrameChannelMode mismatch — or any mode change before dequeue — will deserialize the same payload with the wrong delta path and corrupt state/input parsing.
Suggested fix
struct FrameDelta : IDisposable
{
public BitPacker packer;
public ulong clientTick;
+ public bool reliable;
public void Dispose()
{
packer?.Dispose();
}
} private void SendFrameToRemoteReliable([UsedImplicitly] PlayerID player, ulong localTick, BitPackerWithLength delta)
{
delta.packer.SkipBytes(delta.originalLength);
_deltas.Enqueue(new FrameDelta
{
packer = delta.packer,
- clientTick = localTick
+ clientTick = localTick,
+ reliable = true
});
}
private void SendFrameToRemoteUnreliable([UsedImplicitly] PlayerID player, ulong localTick, BitPackerWithLength delta)
{
delta.packer.SkipBytes(delta.originalLength);
_deltas.Enqueue(new FrameDelta
{
packer = delta.packer,
- clientTick = localTick
+ clientTick = localTick,
+ reliable = false
});
}Then thread previousFrame.reliable through RollbackToFrame(...), RunReadState(...), and ReadInput(...) instead of re-reading PredictionManager.isReliable.
Also applies to: 919-939, 964-978
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/PurrDiction/Runtime/Core/PredictionManager.cs` around lines 913 - 917,
The queued frame payloads are being serialized with the sender's isReliable but
deserialized later using the receiver's current isReliable, which can corrupt
parsing; fix this by adding a reliability flag to the FrameDelta payload (e.g.,
add a reliable boolean/member to FrameDelta when created in
SendFrameToRemote/SendFrameToRemoteReliable/SendFrameToRemoteUnreliable) and
propagate that flag through RollbackToFrame, RunReadState, and ReadInput (use
the passed-in reliable value instead of reading PredictionManager.isReliable) so
the same FrameChannelMode used when writing is used when reading; update the
dequeue/parse code paths that currently access FrameDelta.packer and clientTick
to also read FrameDelta.reliable and use it to select the correct delta read
path.
There was a problem hiding this comment.
isReliable is set at build time and will not be changed at runtime.
There was a problem hiding this comment.
@feiyuren233, understood — since _frameChannelMode is marked [PurrLock] and fixed at configuration time, both the write path and the read path will always observe the same isReliable value for the lifetime of the session. The concern about a mid-flight mismatch doesn't apply here. Thanks for the clarification!
✏️ Learnings added
Learnt from: feiyuren233
Repo: PurrNet/PurrDiction PR: 48
File: Assets/PurrDiction/Runtime/Core/PredictionManager.cs:913-917
Timestamp: 2026-04-29T04:57:43.438Z
Learning: In PurrDiction (`Assets/PurrDiction/Runtime/Core/PredictionManager.cs`), `_frameChannelMode` (and thus `isReliable`) is a `[SerializeField, PurrLock]` field set at build/configuration time and is immutable at runtime. It is safe to read `isReliable` at both enqueue and dequeue sites without storing it per-frame.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| var key = new DeltaKey<UnityRigidbodyCompressedState>(predictionManager.sceneId, id); | ||
| return deltaModule.WriteReliable(packer, target, key, new UnityRigidbodyCompressedState(currentState)); | ||
| var value = new UnityRigidbodyCompressedState(currentState); | ||
| return reliable | ||
| ? deltaModule.WriteReliable(packer, target, key, value) | ||
| : deltaModule.Write(packer, target, key, value); | ||
| } | ||
| case FloatAccuracy.Low: | ||
| { | ||
| var key = new DeltaKey<UnityRigidbodyHalfState>(predictionManager.sceneId, id); | ||
| var res = deltaModule.WriteReliable(packer, target, key, new UnityRigidbodyHalfState(currentState)); | ||
| return res; | ||
| var value = new UnityRigidbodyHalfState(currentState); |
There was a problem hiding this comment.
Use the same scene source for the delta key on both paths.
Lines 136 and 145 build the key with predictionManager.sceneId, but Lines 163 and 177 read it back with sceneId. Those hashes need to stay identical across write/read; if those values ever diverge during setup, pooling, or scene moves, rigidbody deltas will stop matching.
Suggested fix
- var key = new DeltaKey<UnityRigidbodyCompressedState>(predictionManager.sceneId, id);
+ var key = new DeltaKey<UnityRigidbodyCompressedState>(sceneId, id);
...
- var key = new DeltaKey<UnityRigidbodyHalfState>(predictionManager.sceneId, id);
+ var key = new DeltaKey<UnityRigidbodyHalfState>(sceneId, id);Also applies to: 163-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/PurrDiction/Runtime/UnityPhysics/PredictedRigidbody.cs` around lines
136 - 145, The delta key is constructed with predictionManager.sceneId when
writing (in the paths creating DeltaKey<UnityRigidbodyCompressedState> and
DeltaKey<UnityRigidbodyHalfState>) but read back using sceneId elsewhere,
causing mismatched hashes; update the write and/or read paths so both use the
same source (either always predictionManager.sceneId or always the local sceneId
variable) for DeltaKey<T> construction (affecting
DeltaKey<UnityRigidbodyCompressedState>, DeltaKey<UnityRigidbodyHalfState> and
their corresponding read paths) so the keys match across Write/WriteReliable and
Read.
There was a problem hiding this comment.
Not introduced by this PR but this is indeed strange... I guess in practice the PRedictedIdentity's sceneId should be the same as the linked predictionManager's sceneId and we don't cross scene pooling or moving? but this divergence is weird and probably should be fixed?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how sceneId is defined on PredictedIdentity
rg -n "sceneId" --type cs -C3
# Find predictionManager property/field definition on PredictedIdentity
rg -n "predictionManager" --type cs -C2 | grep -A2 -B2 "get\|=>"
Length of output: 20819
@feiyuren233, you're right that in practice they should never diverge. Looking at PredictedIdentity.cs:
// Line 102-103 in PredictedIdentity.cs (Setup method)
predictionManager = world;
sceneId = world.sceneId; // copied from the same PredictionManagersceneId is set from world.sceneId at Setup() time, has a private set, and there's no scene-move or pooling path visible that would update it independently. So sceneId == predictionManager.sceneId is a permanent invariant in normal use.
That said, PredictedTransform uses sceneId consistently on both write and read paths — PredictedRigidbody is the only outlier using predictionManager.sceneId on the write side. The fix is purely cosmetic but brings it in line with the established pattern and makes the contract explicit:
// WriteDeltaState – Medium
- var key = new DeltaKey<UnityRigidbodyCompressedState>(predictionManager.sceneId, id);
+ var key = new DeltaKey<UnityRigidbodyCompressedState>(sceneId, id);
// WriteDeltaState – Low
- var key = new DeltaKey<UnityRigidbodyHalfState>(predictionManager.sceneId, id);
+ var key = new DeltaKey<UnityRigidbodyHalfState>(sceneId, id);Worth a one-liner cleanup even if it's pre-existing, since consistency with PredictedTransform is the right pattern to follow. Would you like to track this as a follow-up issue?
(ᵒ ᵕ ᵒ)ﾉ🐰
🧠 Learnings used
Learnt from: feiyuren233
Repo: PurrNet/PurrDiction PR: 48
File: Assets/PurrDiction/Runtime/Core/PredictionManager.cs:913-917
Timestamp: 2026-04-29T04:57:43.438Z
Learning: In PurrDiction (`Assets/PurrDiction/Runtime/Core/PredictionManager.cs`), `_frameChannelMode` (and thus `isReliable`) is a `[SerializeField, PurrLock]` field set at build/configuration time and is immutable at runtime. It is safe to read `isReliable` at both enqueue and dequeue sites without storing it per-frame.
…verifiedTick); was incorrectly changed
1 participant
Add unreliable frame delivery mode
Problem:
Frames are sent on Channel.ReliableOrdered. A dropped packet stalls the entire stream until retransmission, producing visible freeze-then-jump lag on remotes — even though newer frames have already
arrived.
Solution:
Added UnreliableSequenced frame delivery mode so that packet losses are better tolerated
Changes:
Channel.UnreliableSequenced (Write/Read, no baseline).
determinism guarantees for loss resilience.
Summary by CodeRabbit
New Features
Improvements