[Communication] PR2: Set up IADS proxy for Mailbox use#213
Conversation
|
🧱 Stack PR · Base of stack (9 PRs total) Stack Structure:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Joseph0120
changed the title
|
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 a new Unity MonoBehaviour IadsCommsAgent (Assets/Scripts/IADS/IadsCommsAgent.cs) implementing IAgent as a comms-only mailbox proxy. It exposes serialized references and kinematic fields, property mappings (Position/Transform, Speed, orientation vectors, InverseRotation), ElapsedTime access via SimManager, an idempotent Terminate() with OnTerminated, and many IAgent methods that throw NotSupportedException. Also adds messaging primitives (IMessagePayload, Message types, payloads, PendingMessage), EditMode tests validating wiring, termination and PendingMessage validation, and updates to package manifest/lockfile and a regenerated protobuf runtime header. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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: 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/IADS/IadsCommsAgent.cs`:
- Line 4: Update the class-level comment at the top of IadsCommsAgent.cs (the
comment referring to the IADS Proxy for supporting mailbox message sending and
recieving) to correct the typo by changing "recieving" to "receiving" so the
comment reads "IADS Proxy for supporting mailbox message sending and receiving."
- Around line 21-24: The HierarchicalAgent property may be null when
OnTerminated handlers access it; ensure it cannot be null by initializing
_hierarchicalAgent and guarding accesses: set a safe default in IadsCommsAgent
(e.g., instantiate a new HierarchicalAgent or a lightweight placeholder into the
backing field _hierarchicalAgent), and also add null-checks where OnTerminated
handlers dereference agent.HierarchicalAgent (or bail out early) to avoid race
conditions; update the HierarchicalAgent getter/setter to validate assigned
values (throw or log) so callers cannot set it to null.
🪄 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: 2c791cae-aa45-48c9-bc64-d51cb1c61988
📒 Files selected for processing (1)
Assets/Scripts/IADS/IadsCommsAgent.cs
Joseph0120
force-pushed
the
joseph/communication_delay_PR1
branch
from
8941186 to
a7438b5
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR2
branch
2 times, most recently
from
b0b9526 to
a45b09b
Compare
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/IADS/IadsCommsAgent.cs`:
- Line 57: Fix the typo in the comment inside IadsCommsAgent (file/class
IadsCommsAgent.cs) — change "Setting ElpasedTime has to be dependent on
SimManager to have uniform clock time." to "Setting ElapsedTime has to be
dependent on SimManager to have uniform clock time." so the comment reads
correctly.
In `@Assets/Tests/EditMode/IadsCommsAgentTests.cs`:
- Around line 71-75: The test currently uses a hard-coded compiler backing field
name in SetSimManagerInstance; instead use the SimManager.Instance property via
reflection: locate the PropertyInfo for "Instance" on typeof(SimManager)
(BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic), call
PropertyInfo.SetValue(null, simManager) if a setter exists, and if not present
throw a clear exception indicating the Instance property is read-only so the
test fails with an actionable message; update references to FieldInfo/GetField
to use PropertyInfo/GetProperty and keep method name SetSimManagerInstance and
symbol SimManager.Instance for locating 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: 7c09a180-3b32-46b3-af37-c58ffb263e43
⛔ Files ignored due to path filters (1)
Assets/Scripts/Generated/Proto/CommunicationConfig.csis excluded by!**/generated/**
📒 Files selected for processing (3)
Assets/Scripts/IADS/IadsCommsAgent.csAssets/Tests/EditMode/IadsCommsAgentTests.csAssets/Tests/EditMode/IadsCommsAgentTests.cs.meta
Joseph0120
force-pushed
the
joseph/communication_delay_PR2
branch
from
0f2c041 to
8ed1ce6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-18: The PendingMessage constructor currently allows invalid
deliverAt values; validate deliverAt in the PendingMessage(float deliverAt,
Message message) constructor to ensure it's a finite, non-negative number
(reject float.IsNaN(deliverAt), float.IsInfinity(deliverAt) and deliverAt < 0)
and throw an ArgumentOutOfRangeException (or ArgumentException) naming deliverAt
if validation fails; keep the existing null check for Message and only assign
DeliverAt and Message after validation.
In `@Assets/Tests/EditMode/IadsCommsAgentTests.cs`:
- Around line 12-23: The TearDown currently nulls the global SimManager by
calling SetSimManagerInstance(null) and does not restore the prior singleton;
capture the existing SimManager.Instance in SetUp (e.g., store in a private
field like _previousSimManager) before calling SetSimManagerInstance(null) and
then restore it in TearDown by calling
SetSimManagerInstance(_previousSimManager) (ensure restoration happens before or
after destroying _agent as appropriate); update the SetUp and TearDown methods
to use the private field and keep existing _agent cleanup intact.
🪄 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: a00ffbc5-6111-49c2-8a45-09b1d1811b18
📒 Files selected for processing (7)
Assets/Scripts/Agents/Messaging/IMessagePayload.csAssets/Scripts/Agents/Messaging/Message.csAssets/Scripts/Agents/Messaging/MessagePayload.csAssets/Scripts/Agents/Messaging/PendingMessage.csAssets/Scripts/IADS/IadsCommsAgent.csAssets/Tests/EditMode/IadsCommsAgentTests.csAssets/Tests/EditMode/IadsCommsAgentTests.cs.meta
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)
Tools/pb/run_config_pb2.py (1)
5-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
protobufruntime to the generated requirement (7.34.1)
Tools/pb/run_config_pb2.pyincludes an import-timeValidateProtobufRuntimeVersion(..., 7, 34, 1, ...)check, so any environment with a differentgoogle.protobufruntime will fail to import. The repo doesn’t contain common Python dependency manifests with aprotobufpin, so CI/dev must installprotobuf==7.34.1(or regenerate the checked-in_pb2.pywith the protobuf version you intend to support).🤖 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 `@Tools/pb/run_config_pb2.py` around lines 5 - 16, The checked-in generated file run_config_pb2.py calls ValidateProtobufRuntimeVersion(..., 7, 34, 1) which will break imports unless the environment uses protobuf 7.34.1; either add an explicit pin for protobuf==7.34.1 to the project dependency manifests used by CI/dev (e.g., requirements.txt or pyproject.toml/poetry.lock) so installers will install that exact runtime, or regenerate the _pb2.py files with the protobuf tool version you want to support and update the generated ValidateProtobufRuntimeVersion call accordingly; locate run_config_pb2.py and the CI/dependency manifest to apply the change.
🤖 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 `@Tools/pb/run_config_pb2.py`:
- Around line 5-16: The checked-in generated file run_config_pb2.py calls
ValidateProtobufRuntimeVersion(..., 7, 34, 1) which will break imports unless
the environment uses protobuf 7.34.1; either add an explicit pin for
protobuf==7.34.1 to the project dependency manifests used by CI/dev (e.g.,
requirements.txt or pyproject.toml/poetry.lock) so installers will install that
exact runtime, or regenerate the _pb2.py files with the protobuf tool version
you want to support and update the generated ValidateProtobufRuntimeVersion call
accordingly; locate run_config_pb2.py and the CI/dependency manifest to apply
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae663900-b359-4353-9a77-7aa5fbf3d4df
📒 Files selected for processing (5)
Assets/Scripts/Agents/Messaging/PendingMessage.csAssets/Tests/EditMode/IadsCommsAgentTests.csPackages/manifest.jsonPackages/packages-lock.jsonTools/pb/run_config_pb2.py
|
@tryuan99 Github tests are failing despite the code working locally. Any reason? |
|
Two things you should do first:
|
tryuan99
left a comment
tryuan99
left a comment
There was a problem hiding this comment.
Why are there 500k lines of deletion? Are you sure they're all necessary?
|
|
||
| // IADS Proxy for supporting mailbox message sending and receiving. | ||
|
|
||
| public class IadsCommsAgent : MonoBehaviour, IAgent { |
There was a problem hiding this comment.
why not inherit from AgentBase or even DummyAgent?
There was a problem hiding this comment.
I thought AgentBase is only for physical agents (ie. mass and velocity and other parameters). Goal of IadsCommsAgent is just to add a communication "interface" for the IADS.
There was a problem hiding this comment.
Not necessarily, I think. Right now, you're implementing a lot of pieces of the interface that make it seem like a dummy agent.
The deletions are because I deleted the Assets/UnityTechnologies/ParticlePack and I let unity regenerate it. Im not sure if it is the correct procedure. At least it works; I think it is because of the version mismatches (new computer). But it seems like a conflict in the particle pack. I wonder if the particle pack is crucial for this project? Wondering what was the purpose of the particle pack in the beginning when you imported it? |
|
@daniellovell Could you please help review the particle pack changes? |
2 participants
Files Modified:
IadsCommsAgent.cs -> IADS Proxy for supporting mailbox message sending and recieving. Created a proxy that extends IAgent so that Mailbox can support IADS. Mailbox only supports Agents and adding a proxy makes IADS a part of an agent.