Repository navigation
[PR2] Added IADS Proxy - #211
Joseph0120 wants to merge 5 commits into
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 system for inter-agent communication, including message envelopes, payloads, and a latency management table. Feedback focuses on improving the robustness of the LatencyTable using a dictionary, fixing documentation typos, and refactoring IadsCommsAgent to address Liskov Substitution Principle violations and remove redundant serialized fields.
| public sealed class LatencyTable { | ||
| private readonly float[,] _latency; | ||
|
|
||
| // creates empty _latency array with defaultSeconds seconds each | ||
| public LatencyTable(float defaultSeconds = 0f) { | ||
| int n = Enum.GetValues(typeof(CommsNode)).Length; | ||
| _latency = new float[n, n]; | ||
| for (int i = 0; i < n; i++) { | ||
| for (int j = 0; j < n; j++) { | ||
| _latency[i, j] = defaultSeconds; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void Set(CommsNode from, CommsNode to, float seconds) { | ||
| _latency[(int) from, (int) to] = Mathf.Max(0f, seconds); | ||
| } | ||
|
|
||
| public float Get(CommsNode from, CommsNode to) { | ||
| return _latency[(int) from, (int) to]; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The current implementation of LatencyTable is fragile because it uses an array indexed by casting the CommsNode enum to an integer. This assumes that enum values are contiguous and start at 0. If the enum is modified in the future (e.g., adding non-contiguous values or changing the start index), this will lead to an IndexOutOfRangeException. Using a Dictionary with a tuple key is a more robust and maintainable approach that avoids these assumptions.
public sealed class LatencyTable {
private readonly System.Collections.Generic.Dictionary<(CommsNode, CommsNode), float> _latency = new();
private readonly float _defaultSeconds;
public LatencyTable(float defaultSeconds = 0f) {
_defaultSeconds = defaultSeconds;
}
public void Set(CommsNode from, CommsNode to, float seconds) {
_latency[(from, to)] = Mathf.Max(0f, seconds);
}
public float Get(CommsNode from, CommsNode to) {
return _latency.TryGetValue((from, to), out float seconds) ? seconds : _defaultSeconds;
}
}| /* 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) */ |
There was a problem hiding this comment.
Correcting several typos in the documentation comments ('envolope' -> 'envelope', 'Reciever' -> 'Receiver', 'interntionally' -> 'intentionally') to improve readability and maintain professional standards.
/* 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) */|
|
||
| /* IADS Proxy for supporting mailbox message sending and recreceiving. */ | ||
|
|
||
| public class IadsCommsAgent : MonoBehaviour, IAgent { |
There was a problem hiding this comment.
Implementing the full IAgent interface while throwing NotSupportedException for the majority of its members is a violation of the Liskov Substitution Principle. This suggests that the IAgent interface may be too broad, combining messaging identity with physical simulation capabilities. Consider refactoring the interface hierarchy to separate these concerns, allowing proxy agents to implement only the relevant subset of functionality.
| [SerializeField] | ||
| private Vector3 _velocity = Vector3.zero; | ||
|
|
||
| [SerializeField] | ||
| private Vector3 _acceleration = Vector3.zero; | ||
|
|
||
| [SerializeField] | ||
| private Vector3 _accelerationInput = Vector3.zero; | ||
|
|
||
| public HierarchicalAgent HierarchicalAgent { | ||
| get => _hierarchicalAgent; | ||
| set => _hierarchicalAgent = value; | ||
| } | ||
|
|
||
| public Configs.StaticConfig StaticConfig { get; set; } | ||
| public Configs.AgentConfig AgentConfig { get; set; } | ||
| public IMovement Movement { get; set; } | ||
| public IController Controller { get; set; } | ||
| public ISensor Sensor { get; set; } | ||
| public IAgent TargetModel { get; set; } | ||
|
|
||
| public Vector3 Position { | ||
| get => transform.position; | ||
| set => transform.position = value; | ||
| } | ||
|
|
||
| public Vector3 Velocity { | ||
| get => _velocity; | ||
| set => _velocity = value; | ||
| } | ||
|
|
||
| public float Speed => _velocity.magnitude; | ||
|
|
||
| public Vector3 Acceleration { | ||
| get => _acceleration; | ||
| set => _acceleration = value; | ||
| } | ||
|
|
||
| public Vector3 AccelerationInput { | ||
| get => _accelerationInput; | ||
| set => _accelerationInput = value; | ||
| } |
There was a problem hiding this comment.
The serialized fields _velocity, _acceleration, and _accelerationInput are redundant for a 'comms-only' proxy agent. Since this agent does not participate in physical simulation and its movement-related methods throw NotSupportedException, these fields should be removed to avoid confusion in the Unity Inspector and reduce memory overhead. The properties should return Vector3.zero and 0f directly.
public HierarchicalAgent HierarchicalAgent {
get => _hierarchicalAgent;
set => _hierarchicalAgent = value;
}
public Configs.StaticConfig StaticConfig { get; set; }
public Configs.AgentConfig AgentConfig { get; set; }
public IMovement Movement { get; set; }
public IController Controller { get; set; }
public ISensor Sensor { get; set; }
public IAgent TargetModel { get; set; }
public Vector3 Position {
get => transform.position;
set => transform.position = value;
}
public Vector3 Velocity {
get => Vector3.zero;
set { }
}
public float Speed => 0f;
public Vector3 Acceleration {
get => Vector3.zero;
set { }
}
public Vector3 AccelerationInput {
get => Vector3.zero;
set { }
}
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces a complete messaging system for inter-agent communication. It adds an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ 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 |
File Changed:
IadsCommsAgent.cs
Description:
IadsCommsAgent.cs is an IADS Proxy for supporting mailbox message sending and receiving.