[Communication] PR3: Added Central Mailbox Component.#214
[Communication] PR3: Added Central Mailbox Component.#214Joseph0120 wants to merge 1 commit intojoseph/communication_delay_PR2from
Conversation
|
🧱 Stack PR · Part of stack (9 PRs total) Stack Structure:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized Mailbox component for handling delayed message delivery between agents, featuring configurable latency and Gaussian jitter. The review feedback identifies several areas for improvement, including optimizing the delivery loop to avoid per-frame allocations, addressing potential null reference exceptions due to initialization order, ensuring singleton persistence for manually placed components, and resolving a discrepancy regarding missing packet delivery ratio logic.
| var dueMessages = new System.Collections.Generic.List<PendingMessage>(); | ||
| while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { | ||
| dueMessages.Add(_messageQueue.Dequeue()); | ||
| } | ||
| foreach (PendingMessage pending in dueMessages) { | ||
| if (!IsReceiverValid(pending.Receiver)) { continue; } | ||
| OnMessageDelivered?.Invoke(pending.Receiver, pending.Message); | ||
| } |
There was a problem hiding this comment.
Allocating a new List<PendingMessage> inside DeliverDueMessages every frame (via Update) creates unnecessary garbage collection pressure. Since you are already dequeuing messages from the priority queue, you can process them directly within the while loop. This avoids the intermediate list allocation and the second iteration pass.
while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) {
PendingMessage pending = _messageQueue.Dequeue();
if (IsReceiverValid(pending.Receiver)) {
OnMessageDelivered?.Invoke(pending.Receiver, pending.Message);
}
}| public void Send(Message message) { | ||
| if (message == null || message.Sender == null || message.Receiver == null) { return; } | ||
| float baseLatency = _latencyTable.Get(message.Sender.NodeType, message.Receiver.NodeType); | ||
| float jitter = _latencyJitterStdSeconds > 0f ? SampleGaussian(mean: 0f, stdDev: _latencyJitterStdSeconds) : 0f; | ||
| float totalLatency = Mathf.Max(0f, baseLatency + jitter); | ||
| float deliverAt = GetCurrentTime() + totalLatency; | ||
| _messageQueue.Enqueue(new PendingMessage(deliverAt, message), deliverAt); | ||
| } |
There was a problem hiding this comment.
The Send method is susceptible to a NullReferenceException if called before the Mailbox component has finished its Awake cycle (e.g., if another script calls Mailbox.Instance.Send in its own Awake and has a higher execution priority). Both _latencyTable and _messageQueue are only initialized during Awake. Consider ensuring these fields are initialized in GetOrCreateInstance or adding a safety check/lazy initialization in Send.
| } | ||
| } | ||
|
|
||
| // Repeadly pop due messages off the queue. Apply PDR so only certain amount of messages pass through. |
There was a problem hiding this comment.
The method comment and PR description mention applying PDR (Packet Delivery Ratio), but the implementation currently delivers all due messages without any probabilistic dropping logic. If PDR is intended to be part of this component, the logic should be added to the delivery loop.
| DontDestroyOnLoad(mailboxObject); | ||
| Instance = mailboxObject.AddComponent<Mailbox>(); |
There was a problem hiding this comment.
DontDestroyOnLoad is currently only called when the Mailbox is created via GetOrCreateInstance. If the component is placed manually in a scene, it will be destroyed on scene load, which breaks the singleton persistence. It is recommended to move DontDestroyOnLoad(gameObject) into the Awake method to ensure persistence regardless of how the instance was created.
📝 WalkthroughWalkthroughA new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🤖 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/Scripts/Agents/Messaging/Mailbox.cs`:
- Around line 39-47: The IndividualLatency/default latency arrays are missing
two directed CommsNode pairs (IADS -> Interceptor and Interceptor -> IADS),
causing those routes to default to 0f; update the DefaultLatencyOverrides array
(and the second similar array around the other occurrence) to include
LatencyOverride entries for From = CommsNode.IADS, To = CommsNode.Interceptor,
Seconds = 0.2f and From = CommsNode.Interceptor, To = CommsNode.IADS, Seconds =
0.2f so all 9 directed pairs between CommsNode.IADS, CommsNode.Carrier, and
CommsNode.Interceptor are explicitly specified.
- Around line 87-94: Send() dereferences _latencyTable and _messageQueue which
can be null if ApplyLatencyOverrides() hasn't run; update Send(Message) to check
for null (e.g., if _latencyTable == null || _messageQueue == null) and return
early (or log a warning) before accessing them, so PendingMessage creation and
_latencyTable.Get(...) are protected; reference the Send method and
ApplyLatencyOverrides, and ensure any unit tests that call Send before
initialization still pass by handling the uninitialized mailbox state
gracefully.
- Around line 122-135: DeliverDueMessages is dequeuing all due PendingMessage
items and immediately invoking OnMessageDelivered for each, but it never applies
the Packet Delivery Rate (PDR) probabilistic filter; update DeliverDueMessages
to apply PDR per message before delivery: when iterating dueMessages, for each
PendingMessage use the configured PDR value (e.g., a float field or getter) to
perform a random check (System.Random or UnityEngine.Random.value) and only call
OnMessageDelivered(pending.Receiver, pending.Message) if the random check passes
and IsReceiverValid(pending.Receiver) returns true; ensure dropped messages are
simply not delivered (no further action) and preserve current receiver validity
check and any logging if desired so the rest of the queue logic (_messageQueue,
Peek/Dequeue, PendingMessage.DeliverAt, GetCurrentTime) remains unchanged.
- Around line 128-132: The code in Mailbox.Update is allocating a new
List<PendingMessage> (dueMessages) every frame; replace that per-frame
allocation by either processing messages inline as you Dequeue from
_messageQueue (iterate while !_messageQueue.IsEmpty() and handle each
PendingMessage immediately) or reuse a single preallocated List<PendingMessage>
field (e.g., _reusableDueMessages) that you Clear() each frame before adding;
reference the existing _messageQueue, PendingMessage, DeliverAt and the Update()
delivery loop when applying the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4393003b-061f-4451-8a17-92ad61c4b9ac
📒 Files selected for processing (1)
Assets/Scripts/Agents/Messaging/Mailbox.cs
| private static readonly LatencyOverride[] DefaultLatencyOverrides = { | ||
| new LatencyOverride { From = CommsNode.IADS, To = CommsNode.IADS, Seconds = 0.2f }, | ||
| new LatencyOverride { From = CommsNode.IADS, To = CommsNode.Carrier, Seconds = 0.2f }, | ||
| new LatencyOverride { From = CommsNode.Carrier, To = CommsNode.IADS, Seconds = 0.2f }, | ||
| new LatencyOverride { From = CommsNode.Carrier, To = CommsNode.Carrier, Seconds = 0.2f }, | ||
| new LatencyOverride { From = CommsNode.Carrier, To = CommsNode.Interceptor, Seconds = 0.2f }, | ||
| new LatencyOverride { From = CommsNode.Interceptor, To = CommsNode.Carrier, Seconds = 0.2f }, | ||
| new LatencyOverride { From = CommsNode.Interceptor, To = CommsNode.Interceptor, Seconds = 0.2f }, | ||
| }; |
There was a problem hiding this comment.
IndividualLatency table is incomplete (missing 2 node pairs)
For 3 CommsNode values, individual mode should define 9 directed pairs, but only 7 are set. Missing pairs fall back to 0f latency, which creates unintended zero-latency routes.
🐛 Minimum fix
private static readonly LatencyOverride[] DefaultLatencyOverrides = {
new LatencyOverride { From = CommsNode.IADS, To = CommsNode.IADS, Seconds = 0.2f },
new LatencyOverride { From = CommsNode.IADS, To = CommsNode.Carrier, Seconds = 0.2f },
+ new LatencyOverride { From = CommsNode.IADS, To = CommsNode.Interceptor, Seconds = 0.2f },
new LatencyOverride { From = CommsNode.Carrier, To = CommsNode.IADS, Seconds = 0.2f },
new LatencyOverride { From = CommsNode.Carrier, To = CommsNode.Carrier, Seconds = 0.2f },
new LatencyOverride { From = CommsNode.Carrier, To = CommsNode.Interceptor, Seconds = 0.2f },
+ new LatencyOverride { From = CommsNode.Interceptor, To = CommsNode.IADS, Seconds = 0.2f },
new LatencyOverride { From = CommsNode.Interceptor, To = CommsNode.Carrier, Seconds = 0.2f },
new LatencyOverride { From = CommsNode.Interceptor, To = CommsNode.Interceptor, Seconds = 0.2f },
};Also applies to: 108-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Agents/Messaging/Mailbox.cs` around lines 39 - 47, The
IndividualLatency/default latency arrays are missing two directed CommsNode
pairs (IADS -> Interceptor and Interceptor -> IADS), causing those routes to
default to 0f; update the DefaultLatencyOverrides array (and the second similar
array around the other occurrence) to include LatencyOverride entries for From =
CommsNode.IADS, To = CommsNode.Interceptor, Seconds = 0.2f and From =
CommsNode.Interceptor, To = CommsNode.IADS, Seconds = 0.2f so all 9 directed
pairs between CommsNode.IADS, CommsNode.Carrier, and CommsNode.Interceptor are
explicitly specified.
| private void OnValidate() { | ||
| _latencyJitterStdSeconds = Mathf.Max(0f, _latencyJitterStdSeconds); | ||
| _uniformLatency = Mathf.Max(0f, _uniformLatency); | ||
| } |
There was a problem hiding this comment.
Inspector latency edits won’t take effect after initialization
Line 77–80 sanitizes values but does not rebuild _latencyTable, so runtime inspector changes can leave the active table stale.
🔧 Proposed fix
private void OnValidate() {
_latencyJitterStdSeconds = Mathf.Max(0f, _latencyJitterStdSeconds);
_uniformLatency = Mathf.Max(0f, _uniformLatency);
+ if (Application.isPlaying) {
+ ApplyLatencyOverrides();
+ }
}| public void Send(Message message) { | ||
| if (message == null || message.Sender == null || message.Receiver == null) { return; } | ||
| float baseLatency = _latencyTable.Get(message.Sender.NodeType, message.Receiver.NodeType); | ||
| float jitter = _latencyJitterStdSeconds > 0f ? SampleGaussian(mean: 0f, stdDev: _latencyJitterStdSeconds) : 0f; | ||
| float totalLatency = Mathf.Max(0f, baseLatency + jitter); | ||
| float deliverAt = GetCurrentTime() + totalLatency; | ||
| _messageQueue.Enqueue(new PendingMessage(deliverAt, message), deliverAt); | ||
| } |
There was a problem hiding this comment.
Guard Send() against uninitialized mailbox state
At Line 89 and Line 93, Send() dereferences _latencyTable and _messageQueue without ensuring they are initialized. If Send() is reached before ApplyLatencyOverrides(), this will throw a NullReferenceException.
🐛 Proposed fix
public void Send(Message message) {
+ if (_latencyTable == null || _messageQueue == null) {
+ ApplyLatencyOverrides();
+ }
if (message == null || message.Sender == null || message.Receiver == null) { return; }
float baseLatency = _latencyTable.Get(message.Sender.NodeType, message.Receiver.NodeType);
float jitter = _latencyJitterStdSeconds > 0f ? SampleGaussian(mean: 0f, stdDev: _latencyJitterStdSeconds) : 0f;
float totalLatency = Mathf.Max(0f, baseLatency + jitter);
float deliverAt = GetCurrentTime() + totalLatency;
_messageQueue.Enqueue(new PendingMessage(deliverAt, message), deliverAt);
}📝 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.
| public void Send(Message message) { | |
| if (message == null || message.Sender == null || message.Receiver == null) { return; } | |
| float baseLatency = _latencyTable.Get(message.Sender.NodeType, message.Receiver.NodeType); | |
| float jitter = _latencyJitterStdSeconds > 0f ? SampleGaussian(mean: 0f, stdDev: _latencyJitterStdSeconds) : 0f; | |
| float totalLatency = Mathf.Max(0f, baseLatency + jitter); | |
| float deliverAt = GetCurrentTime() + totalLatency; | |
| _messageQueue.Enqueue(new PendingMessage(deliverAt, message), deliverAt); | |
| } | |
| public void Send(Message message) { | |
| if (_latencyTable == null || _messageQueue == null) { | |
| ApplyLatencyOverrides(); | |
| } | |
| if (message == null || message.Sender == null || message.Receiver == null) { return; } | |
| float baseLatency = _latencyTable.Get(message.Sender.NodeType, message.Receiver.NodeType); | |
| float jitter = _latencyJitterStdSeconds > 0f ? SampleGaussian(mean: 0f, stdDev: _latencyJitterStdSeconds) : 0f; | |
| float totalLatency = Mathf.Max(0f, baseLatency + jitter); | |
| float deliverAt = GetCurrentTime() + totalLatency; | |
| _messageQueue.Enqueue(new PendingMessage(deliverAt, message), deliverAt); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Agents/Messaging/Mailbox.cs` around lines 87 - 94, Send()
dereferences _latencyTable and _messageQueue which can be null if
ApplyLatencyOverrides() hasn't run; update Send(Message) to check for null
(e.g., if _latencyTable == null || _messageQueue == null) and return early (or
log a warning) before accessing them, so PendingMessage creation and
_latencyTable.Get(...) are protected; reference the Send method and
ApplyLatencyOverrides, and ensure any unit tests that call Send before
initialization still pass by handling the uninitialized mailbox state
gracefully.
| // Repeadly pop due messages off the queue. Apply PDR so only certain amount of messages pass through. | ||
| private void DeliverDueMessages() { | ||
| if (_messageQueue == null) { | ||
| return; | ||
| } | ||
| float currentTime = GetCurrentTime(); | ||
| var dueMessages = new System.Collections.Generic.List<PendingMessage>(); | ||
| while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { | ||
| dueMessages.Add(_messageQueue.Dequeue()); | ||
| } | ||
| foreach (PendingMessage pending in dueMessages) { | ||
| if (!IsReceiverValid(pending.Receiver)) { continue; } | ||
| OnMessageDelivered?.Invoke(pending.Receiver, pending.Message); | ||
| } |
There was a problem hiding this comment.
PDR filtering is not implemented in delivery path
Line 132–135 currently invokes delivery for all due messages (subject only to receiver validity). The PR objective says delivery should be probabilistically filtered by packet delivery rate; that logic is missing.
🐛 Proposed direction
+ [SerializeField, Range(0f, 1f)]
+ private float _packetDeliveryRate = 1f;
foreach (PendingMessage pending in dueMessages) {
if (!IsReceiverValid(pending.Receiver)) { continue; }
+ if (UnityEngine.Random.value > _packetDeliveryRate) { continue; }
OnMessageDelivered?.Invoke(pending.Receiver, pending.Message);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Agents/Messaging/Mailbox.cs` around lines 122 - 135,
DeliverDueMessages is dequeuing all due PendingMessage items and immediately
invoking OnMessageDelivered for each, but it never applies the Packet Delivery
Rate (PDR) probabilistic filter; update DeliverDueMessages to apply PDR per
message before delivery: when iterating dueMessages, for each PendingMessage use
the configured PDR value (e.g., a float field or getter) to perform a random
check (System.Random or UnityEngine.Random.value) and only call
OnMessageDelivered(pending.Receiver, pending.Message) if the random check passes
and IsReceiverValid(pending.Receiver) returns true; ensure dropped messages are
simply not delivered (no further action) and preserve current receiver validity
check and any logging if desired so the rest of the queue logic (_messageQueue,
Peek/Dequeue, PendingMessage.DeliverAt, GetCurrentTime) remains unchanged.
| var dueMessages = new System.Collections.Generic.List<PendingMessage>(); | ||
| while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) { | ||
| dueMessages.Add(_messageQueue.Dequeue()); | ||
| } | ||
| foreach (PendingMessage pending in dueMessages) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid per-frame allocation in message delivery loop
Line 128 allocates a new list every Update(). This hot-path allocation can cause unnecessary GC pressure.
♻️ Proposed refactor
float currentTime = GetCurrentTime();
-var dueMessages = new System.Collections.Generic.List<PendingMessage>();
while (!_messageQueue.IsEmpty() && currentTime >= _messageQueue.Peek().DeliverAt) {
- dueMessages.Add(_messageQueue.Dequeue());
-}
-foreach (PendingMessage pending in dueMessages) {
+ PendingMessage pending = _messageQueue.Dequeue();
if (!IsReceiverValid(pending.Receiver)) { continue; }
OnMessageDelivered?.Invoke(pending.Receiver, pending.Message);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Agents/Messaging/Mailbox.cs` around lines 128 - 132, The code
in Mailbox.Update is allocating a new List<PendingMessage> (dueMessages) every
frame; replace that per-frame allocation by either processing messages inline as
you Dequeue from _messageQueue (iterate while !_messageQueue.IsEmpty() and
handle each PendingMessage immediately) or reuse a single preallocated
List<PendingMessage> field (e.g., _reusableDueMessages) that you Clear() each
frame before adding; reference the existing _messageQueue, PendingMessage,
DeliverAt and the Update() delivery loop when applying the change.
Joseph0120
changed the title
Joseph0120
changed the title
1 participant
Files Created:
Mailbox.cs -> The centralized mailbox is what all agents use to send and receive delayed messages.
Descriptions:
There are 3 modes under "LatencyMode" for constructing the latency table. NoLatency sets mailbox to Ideal communication with zero latency (assume no latency); UniformLatency set all latency entries to one singular "_uniformLatency" value; IndividualLatency allows you to configure each "from->to" pair independently. "_latencyJitterStdSeconds" sets Latency jitter standard deviation in seconds.
Send(Message message) enqueue a message for delayed delivery. Message will be released when DeliverTime has reached.
DeliverDueMessages() repeatedly pop due messages off the queue. Apply PDR so only certain amount of messages pass through.