[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?
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? |
| public void AttachCommsNode(CommsNode commsNode) { | ||
| if (commsNode == null) { | ||
| throw new ArgumentNullException(nameof(commsNode)); | ||
| } | ||
| if (CommsNode != null && !ReferenceEquals(CommsNode, commsNode)) { | ||
| throw new InvalidOperationException($"{name} already has a comms node."); | ||
| } | ||
| CommsNode = commsNode; |
There was a problem hiding this comment.
overly verbose - it's the responsibility of the comms manager to check this. the ICommsEndpoint should just have a setter and getter
| public enum CommsEndpointType { | ||
| Invalid, | ||
| Iads, | ||
| Vessel, | ||
| ShoreBattery, | ||
| CarrierInterceptor, | ||
| MissileInterceptor, | ||
| FixedWingThreat, | ||
| RotaryWingThreat, |
There was a problem hiding this comment.
This looks like https://github.com/PisterLab/micromissiles-unity/blob/master/Assets/Proto/Configs/static_config.proto#L6-L17 now. I think you should reuse that. In AgentType, I'd add the IADS as index 1 and move the rest up by 1.
|
|
||
| public event Action<Message> OnMessageReceived; | ||
|
|
||
| public bool IsTerminated => _isTerminated; |
There was a problem hiding this comment.
why not just a public getter/private setter?
|
|
||
| public bool IsTerminated => _isTerminated; | ||
|
|
||
| public CommsNode(CommsEndpointType endpointType = CommsEndpointType.Invalid) { |
|
|
||
| public IReadOnlyList<IHierarchical> Launchers => _launchers.AsReadOnly(); | ||
|
|
||
| public CommsNode CommsNode { get; } = new CommsNode(CommsEndpointType.Iads); |
There was a problem hiding this comment.
no, CommsManager should listen for OnSimulationStarted and OnSimulationEnded and allocate/destroy a comms node to IADS in those callbacks
| // any interceptors that already exist. | ||
| private IEnumerator Start() { | ||
| while (SimManager.Instance == null) { | ||
| yield return null; |
There was a problem hiding this comment.
no need for this if you define the priorities of the managers
| SimManager.Instance.OnSimulationEnded += RegisterSimulationEnded; | ||
| SimManager.Instance.OnNewInterceptor += RegisterNewInterceptor; | ||
|
|
||
| foreach (var agent in SimManager.Instance.Interceptors) { |
There was a problem hiding this comment.
no, comms manager should be there before interceptors get added. you then listen for RegisterNewInterceptor to add a comms node to each newly created interceptor
| if (SimManager.Instance != null) { | ||
| SimManager.Instance.OnSimulationEnded -= RegisterSimulationEnded; | ||
| SimManager.Instance.OnNewInterceptor -= RegisterNewInterceptor; |
|
|
||
| // Terminate every tracked comms node when the simulation ends or the manager is torn down. | ||
| private void RegisterSimulationEnded() { | ||
| foreach (var agentNodePair in _nodesByAgent) { |
There was a problem hiding this comment.
is this necessary? the agent will die anyway. why not just clear _nodesByAgent?
| } | ||
|
|
||
| private static CommsEndpointType GetEndpointType(IInterceptor interceptor) { | ||
| return interceptor.StaticConfig?.AgentType switch { |
There was a problem hiding this comment.
if you use AgentType, you don't need this map :)
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.