[Communication] PR1: Added Base Files for Mailbox#212
Conversation
|
🧱 Stack PR · Base of stack 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 messaging framework for agents, including structures for message payloads, latency tracking, and a pending message queue. I have reviewed the implementation and suggest the following improvements: address the potential fragility of using enum casts as array indices in the LatencyTable, optimize the LatencyTable initialization loop to avoid unnecessary iterations when the default value is zero, refactor the message system to use a generic Message class to reduce boilerplate, and implement IComparable in the PendingMessage struct to better support priority queue operations.
|
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:
📝 WalkthroughWalkthroughThis pull request introduces a new agent messaging infrastructure consisting of four files. It defines an empty Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
🤖 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/LatencyTable.cs`:
- Around line 19-27: The LatencyTable constructor assigns defaultSeconds
directly producing negative values unlike the Set() method; update the
constructor for LatencyTable to clamp defaultSeconds (use Mathf.Max(0f,
defaultSeconds)) before filling the _latency[,] array so all initial entries
match the non-negative enforcement used by Set().
In `@Assets/Scripts/Agents/Messaging/Message.cs`:
- Line 3: Update the class/file comment for Message (the top-line comment above
the Message class) to correct the typos: change "envolope" to "envelope" and
"Reciever" to "Receiver", so the comment reads something like "Message is a base
envelope for message sending. It always carries Sender, Receiver, Type, and
Payload." Ensure you only change the comment text associated with the Message
definition and preserve the original punctuation and casing for identifiers
Sender, Receiver, Type, and Payload.
- Around line 12-15: Fix typos in the class/file comment for Message: replace
"envolope" with "envelope", "Reciever" with "Receiver", "interntionally" with
"intentionally", and "interntionally layers" -> "intentionally layers"; ensure
the sentence reads clearly (e.g., "Message is a base envelope for message
sending. It always carries Sender, Receiver, Type, and Payload. This file wraps
the payload data with transport metadata. It intentionally layers and splits the
payload and the transportation data. Mailbox only focuses on transportation
(enqueue, latency, and delivery timing)"). Update the block comment above the
Message definition accordingly.
- Around line 17-29: Add a short XML doc comment to the Message class describing
how terminated agents are handled: state that Message stores raw IAgent
references (Sender and Receiver) and that consumers or the mailbox are
responsible for checking IAgent.IsTerminated or subscribing to
IAgent.OnTerminated before delivering/processing messages to avoid operating on
terminated agents; mention that Message itself does not automatically remove or
null out references on termination. Update the summary for the Message
constructor and the Sender/Receiver properties (in the Message class) to
reference IAgent.IsTerminated and OnTerminated so maintainers know where to
implement termination checks.
In `@Assets/Scripts/Agents/Messaging/MessagePayload.cs`:
- Around line 3-5: Fix the spelling typo in the class/file comment for
MessagePayload: change "envolopes" to "envelopes" in the top block comment that
describes MessagePayload and payload carriage by Message envelopes so the
comment reads correctly and remains clear for future readers.
In `@Assets/Scripts/Agents/Messaging/PendingMessage.cs`:
- Around line 3-5: Fix the spelling mistake in the class comment for the mailbox
queue item in PendingMessage.cs by changing "internaal" to "internal" (the
comment describing the mailbox's internal queue item that stores the Message
object and DeliverAt for the PendingMessage class/struct).
🪄 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: b899f98e-8555-4c43-ad9d-ef0b685d1c1e
📒 Files selected for processing (10)
Assets/Scripts/Agents/Messaging.metaAssets/Scripts/Agents/Messaging/IMessagePayload.csAssets/Scripts/Agents/Messaging/IMessagePayload.cs.metaAssets/Scripts/Agents/Messaging/LatencyTable.csAssets/Scripts/Agents/Messaging/LatencyTable.cs.metaAssets/Scripts/Agents/Messaging/Message.csAssets/Scripts/Agents/Messaging/Message.cs.metaAssets/Scripts/Agents/Messaging/MessagePayload.csAssets/Scripts/Agents/Messaging/MessagePayload.cs.metaAssets/Scripts/Agents/Messaging/PendingMessage.cs
tryuan99
left a comment
tryuan99
left a comment
There was a problem hiding this comment.
- Please address or resolve the AI PR review before requesting a review.
- Add newlines to all files.
- Comment style in this repo is
// Some comment that starts with capital letter and ends with a period. - Run
pre-commit installand make sure that clang-format is enabled before each commit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/LatencyTable.cs`:
- Line 51: The exception message is wrong: update the
ArgumentOutOfRangeException thrown for the parameter named node to reference
CommsNode (e.g., "Unknown CommsNode" or "Unknown CommsNode type.")—modify the
throw statement that currently reads throw new
ArgumentOutOfRangeException(nameof(node), node, "Unknown Interceptor Type."); so
it accurately mentions CommsNode (keep nameof(node) and node arguments
unchanged).
🪄 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: 3040908b-ff3c-49ff-bdfc-7edfab1fba8e
📒 Files selected for processing (5)
Assets/Scripts/Agents/Messaging/IMessagePayload.csAssets/Scripts/Agents/Messaging/LatencyTable.csAssets/Scripts/Agents/Messaging/Message.csAssets/Scripts/Agents/Messaging/MessagePayload.csAssets/Scripts/Agents/Messaging/PendingMessage.cs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Message.cs`:
- Around line 32-61: The three concrete message types
(AssignSubInterceptorRequestMessage, AssignTargetMessage,
ReassignTargetRequestMessage) duplicate a typed PayloadData property and an
override of the interface Payload; introduce a generic base Message<TPayload>
where TPayload : IMessagePayload that exposes a typed public TPayload Payload {
get; } and implements the IMessagePayload override (returning the typed Payload)
so concrete classes can inherit Message<AssignSubInterceptorRequestPayload>,
Message<AssignTargetPayload>, Message<ReassignTargetRequestPayload> and drop the
duplicated PayloadData and Payload override boilerplate; update the constructors
to assign the typed Payload in the generic base.
In `@Assets/Scripts/Agents/Messaging/PendingMessage.cs`:
- Around line 11-12: The auto-properties Sender and Receiver use
null-conditional access (Message?.Sender / Message?.Receiver) even though the
PendingMessage constructor enforces a non-null Message; remove the
null-conditional operators and access Message.Sender and Message.Receiver
directly in the Sender and Receiver properties so a default(PendingMessage) will
throw and surface the bug instead of silently returning null (adjust
PendingMessage constructor/validation only if you intend to allow default
instances).
🪄 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: 1a726870-f3e3-4bf3-be5f-f6841c05b992
📒 Files selected for processing (5)
Assets/Scripts/Agents/Messaging/LatencyTable.csAssets/Scripts/Agents/Messaging/Message.csAssets/Scripts/Agents/Messaging/MessagePayload.csAssets/Scripts/Agents/Messaging/PendingMessage.csProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
Joseph0120
force-pushed
the
joseph/communication_delay_PR1
branch
from
8941186 to
a7438b5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Assets/Scripts/Agents/Messaging/Message.cs (1)
3-4:⚠️ Potential issue | 🟡 MinorFix remaining typo in header comment (
envolope→envelope).Line [3] still has the misspelling in the short header comment.
📝 Proposed fix
-// Message is a base envolope for message sending. It always carries Sender, Receiver, Type, and +// Message is a base envelope for message sending. It always carries Sender, Receiver, Type, and🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Assets/Scripts/Agents/Messaging/Message.cs` around lines 3 - 4, The header comment on the Message class contains a typo: change "envolope" to "envelope" in the top-line comment that describes Message as a base envelope for message sending; locate the comment in Assets/Scripts/Agents/Messaging/Message.cs associated with the Message type and update the word to "envelope" to correct the spelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Assets/Scripts/Agents/Messaging/Message.cs`:
- Around line 3-4: The header comment on the Message class contains a typo:
change "envolope" to "envelope" in the top-line comment that describes Message
as a base envelope for message sending; locate the comment in
Assets/Scripts/Agents/Messaging/Message.cs associated with the Message type and
update the word to "envelope" to correct the spelling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b08dceb-c56e-4213-99f0-83073172c8d2
📒 Files selected for processing (10)
Assets/Scripts/Agents/Messaging.metaAssets/Scripts/Agents/Messaging/IMessagePayload.csAssets/Scripts/Agents/Messaging/IMessagePayload.cs.metaAssets/Scripts/Agents/Messaging/LatencyTable.csAssets/Scripts/Agents/Messaging/LatencyTable.cs.metaAssets/Scripts/Agents/Messaging/Message.csAssets/Scripts/Agents/Messaging/Message.cs.metaAssets/Scripts/Agents/Messaging/MessagePayload.csAssets/Scripts/Agents/Messaging/MessagePayload.cs.metaAssets/Scripts/Agents/Messaging/PendingMessage.cs
tryuan99
left a comment
tryuan99
left a comment
There was a problem hiding this comment.
have you run this code? where are the .meta files?
tryuan99
left a comment
tryuan99
left a comment
There was a problem hiding this comment.
have you run this code? where are the .meta files?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Assets/Scripts/Agents/Messaging/PendingMessage.cs (1)
12-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't silently mask an invalid default struct here.
The null-conditional turns an accidental
default(PendingMessage)intonullsender/receiver values instead of failing at the source. If default instances are not valid queue entries, direct access is safer.Suggested fix
- public IAgent Sender => Message?.Sender; - public IAgent Receiver => Message?.Receiver; + public IAgent Sender => Message.Sender; + public IAgent Receiver => Message.Receiver;🤖 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/Scripts/Agents/Messaging/PendingMessage.cs` around lines 12 - 13, The properties Sender and Receiver on PendingMessage currently use the null-conditional (Message?.Sender / Message?.Receiver) which masks an accidental default(PendingMessage); change them to access Message directly (Message.Sender / Message.Receiver) so invalid default instances fail fast; update the PendingMessage type to ensure Message cannot be null at construction or add an explicit guard/assert in the struct (referencing PendingMessage, the Message field/property, and the Sender/Receiver property getters) so callers get a clear failure instead of null values.
🤖 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/Scripts/Agents/Messaging/PendingMessage.cs`:
- Around line 15-17: The PendingMessage constructor accepts invalid deliverAt
values (negative, NaN, ±Infinity) that break queue ordering; update the
PendingMessage(float deliverAt, Message message) constructor to validate
deliverAt using float.IsNaN(deliverAt), float.IsInfinity(deliverAt), and
deliverAt < 0 and throw an ArgumentOutOfRangeException (or ArgumentException)
with a clear message when invalid, while still null-checking Message; ensure
DeliverAt is only assigned after the validation passes.
---
Duplicate comments:
In `@Assets/Scripts/Agents/Messaging/PendingMessage.cs`:
- Around line 12-13: The properties Sender and Receiver on PendingMessage
currently use the null-conditional (Message?.Sender / Message?.Receiver) which
masks an accidental default(PendingMessage); change them to access Message
directly (Message.Sender / Message.Receiver) so invalid default instances fail
fast; update the PendingMessage type to ensure Message cannot be null at
construction or add an explicit guard/assert in the struct (referencing
PendingMessage, the Message field/property, and the Sender/Receiver property
getters) so callers get a clear failure instead of null values.
🪄 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: 1aaeb194-d5a3-412e-9b85-63982d4375c9
⛔ Files ignored due to path filters (1)
Assets/Scripts/Generated/Proto/CommunicationConfig.csis excluded by!**/generated/**
📒 Files selected for processing (3)
Assets/Scripts/Agents/Messaging/Message.csAssets/Scripts/Agents/Messaging/MessagePayload.csAssets/Scripts/Agents/Messaging/PendingMessage.cs
…/micromissiles-unity into joseph/communication_delay_PR1
|
@tryuan99 Should I include Assets/Scripts/Generated/Proto/CommunicationConfig.cs? How about meta files? |
Yes and yes. You should read about what the meta files are used for (https://stackoverflow.com/questions/42462989/what-is-a-meta-file-and-why-does-unity-create-them-for-all-of-my-assets#comment111250115_42471800) |
tryuan99
left a comment
tryuan99
left a comment
There was a problem hiding this comment.
A lot of small comments - after the next revision, should be ready to merge
| @@ -1,36 +1,36 @@ | |||
| { | |||
| "m_Dictionary": { | |||
There was a problem hiding this comment.
Im not sure, this happened before. I am wondering if it is due to Pre-commit?
There was a problem hiding this comment.
Is there a way I could quickly ignore this change in git?
There was a problem hiding this comment.
git checkout master -- ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
There was a problem hiding this comment.
I see what you mean - let's leave this for now.
@Joseph0120 When running the simulator on this branch, do you see this flickering orange cube? |
Never mind, that's the CVN. Might be worth offseting the CVN a bit as to not overlap with the DDG. |
It only flickers because the two blocks (orange and blue) were overlapping. Should be normal. |
2 participants
Files Added:
iMessagePayload.cs -> Interface for MessagePayload
LatencyTable.cs -> Creates table that keeps track of the delay between every agent. Table is n * n sized, n is the size of CommsNode.
Message.cs -> Message is a base envolope for message sending. It always carries Sender, Reciever, Type, and Payload.
This file wraps the payload data with transport metadata. It interntionally layers and splits the
payload and the transportation data. Mailbox only focuses on transportation (enqueue, latency,
and delivery timing)
MessagePayload.cs -> MessagePayload defines different types of new payload objects. Payload is carried by
Message envolopes. Concrete payload content lives in here and are only read explicitly
by receivers.
PendingMessage.cs -> This is the mailbox's internal queue item. It stores the Message object
and DeliverAt into the priority queue and pops the message when DeliverAt
time has reached.