Repository navigation
[IADS] Clean up IADS and simulation manager - #124
Conversation
daniellovell
left a comment
daniellovell
left a comment
There was a problem hiding this comment.
changes since last review LGTM :)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Assets/Scripts/UI/CameraController.cs (2)
326-337: TODO placeholders fall back toGetAllAgentsCenter()which may be misleading.The
FOLLOW_INTERCEPTOR_SWARMandFOLLOW_THREAT_SWARMmodes currently fall back toGetAllAgentsCenter(), making them behave identically toFOLLOW_ALL_AGENTS. This could confuse users who expect swarm-specific tracking. Consider returning early or logging when these modes are selected but not yet implemented.
279-290: Fix centroid initialization logic in SetCameraMode—should initialize when entering follow modes, not exiting.The condition is backwards. When transitioning from FREE to a follow mode, centroids should be initialized. Currently they're only initialized when exiting a follow mode. Change the condition to initialize centroids when
mode != CameraMode.FREE:public void SetCameraMode(CameraMode mode) { if (mode != CameraMode.FREE) { _currentCentroid = target.position; _targetCentroid = target.position; } else { if (_centroidUpdateCoroutine != null) { StopCoroutine(_centroidUpdateCoroutine); _centroidUpdateCoroutine = null; } } cameraMode = mode; }This ensures centroids are properly initialized before the update coroutine begins using them.
Assets/Scripts/Managers/ParticleManager.cs (1)
145-150: Hit marker is added to list twice.
SpawnHitMarkeradds the marker to_hitMarkerListat line 148, and the callers (RegisterInterceptorHitat line 117 andRegisterInterceptorMissat line 125) also add the returned object to_hitMarkerList. This results in duplicate entries.Remove the duplicate add inside
SpawnHitMarker:private GameObject SpawnHitMarker(in Vector3 position) { GameObject hitMarker = Instantiate(Resources.Load<GameObject>("Prefabs/HitMarkerPrefab"), position, Quaternion.identity); - _hitMarkerList.Add(hitMarker); return hitMarker; }
♻️ Duplicate comments (5)
Assets/Scripts/Agents/AgentBase.cs (1)
147-158: Terminate() implementation follows correct event ordering.State is updated (
IsTerminated = true) before firingOnTerminated, ensuring subscribers see consistent state. The null-checks onHierarchicalAgentandMovementpattern-match appropriately.The
Position = Vector3.zerobefore deactivation was flagged in a previous review regarding object pooling concerns.Assets/Scripts/Interceptors/IInterceptor.cs (1)
5-5: Delegate naming has been previously discussed.The delegate name
InterceptHitMissEventHandlerwas discussed in a previous review, with a suggestion to useInterceptorHitMissEventHandlerfor clarity. While the current name is consistent with the codebase pattern, consider the earlier feedback if refactoring for improved readability.Assets/Scripts/Threats/IThreat.cs (1)
7-7: Delegate naming may have been previously addressed.There's a past review comment about delegate naming, but the current name
ThreatHitMissEventHandlerappears to be the suggested form. If the previous comment was proposing this name, it has been implemented.Assets/Scripts/Hierarchical/FixedHierarchical.cs (1)
5-9: Acknowledged design:newkeyword with explicit interface implementation.The
newkeyword shadowing combined with explicitIHierarchicalimplementation is a known workaround (per prior discussion). This ensures correct behavior when accessed through the interface, though accessing through aHierarchicalBasereference will still invoke the base class properties. If all consumers useIHierarchical, this is acceptable.The
IsTerminated => falseoverride is correct for a fixed test object that never terminates.Assets/Scripts/Threats/ThreatBase.cs (1)
160-161: Unsafe cast may causeNullReferenceException.If
HierarchicalAgent.Targetis not aHierarchicalAgent(e.g., it's aFixedHierarchicalas created inStart()), the cast will returnnull, and accessingtargetAgent.Agentwill throw. Use pattern matching for safe access.Apply this diff:
- } else if (HierarchicalAgent.Target is HierarchicalAgent targetAgent && - otherAgent == targetAgent.Agent) { + } else if (HierarchicalAgent?.Target is HierarchicalAgent targetAgent && + otherAgent == targetAgent.Agent) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Assets/Resources/Prefabs/DummyAgent.prefabis excluded by!**/*.prefabAssets/Scenes/MainScene.unityis excluded by!**/*.unityAssets/Scripts/Generated/Proto/FlightPhase.csis excluded by!**/generated/**
📒 Files selected for processing (33)
Assets/Proto/Simulation/flight_phase.proto(1 hunks)Assets/Scripts/Agents/AgentBase.cs(7 hunks)Assets/Scripts/Agents/IAgent.cs(2 hunks)Assets/Scripts/Algorithms/Clustering/Cluster.cs(1 hunks)Assets/Scripts/Behavior/AttackBehaviorFactory.cs(0 hunks)Assets/Scripts/Behavior/AttackBehaviorFactory.cs.meta(0 hunks)Assets/Scripts/Config/ConfigLoader.cs(1 hunks)Assets/Scripts/Hierarchical/FixedHierarchical.cs(1 hunks)Assets/Scripts/Hierarchical/HierarchicalAgent.cs(1 hunks)Assets/Scripts/Hierarchical/HierarchicalBase.cs(3 hunks)Assets/Scripts/Hierarchical/IHierarchical.cs(1 hunks)Assets/Scripts/IADS/IADS.cs(2 hunks)Assets/Scripts/Interceptors/CarrierBase.cs(1 hunks)Assets/Scripts/Interceptors/CarrierInterceptor.cs(1 hunks)Assets/Scripts/Interceptors/IInterceptor.cs(1 hunks)Assets/Scripts/Interceptors/InterceptorBase.cs(2 hunks)Assets/Scripts/Managers/InputManager.cs(1 hunks)Assets/Scripts/Managers/ParticleManager.cs(8 hunks)Assets/Scripts/Managers/SimManager.cs(5 hunks)Assets/Scripts/Monitors/SimMonitor.cs(4 hunks)Assets/Scripts/Movements/MissileMovement.cs(3 hunks)Assets/Scripts/Movements/MovementBase.cs(1 hunks)Assets/Scripts/Release/MassReleaseStrategyBase.cs(1 hunks)Assets/Scripts/Release/SingleReleaseStrategyBase.cs(2 hunks)Assets/Scripts/Threats/IThreat.cs(1 hunks)Assets/Scripts/Threats/ThreatBase.cs(3 hunks)Assets/Scripts/UI/CameraController.cs(7 hunks)Assets/Scripts/UI/UIManager.cs(5 hunks)Assets/StreamingAssets/Configs/Simulations/5_swarms_500_ucav.pbtxt(1 hunks)Assets/Tests/EditMode/AgentBaseTests.cs(3 hunks)Assets/Tests/EditMode/FixedHierarchicalTests.cs(1 hunks)Assets/Tests/EditMode/FixedHierarchicalTests.cs.meta(1 hunks)Assets/Tests/PlayMode/ConfigsTests.cs(2 hunks)
💤 Files with no reviewable changes (2)
- Assets/Scripts/Behavior/AttackBehaviorFactory.cs.meta
- Assets/Scripts/Behavior/AttackBehaviorFactory.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T05:39:03.474Z
Learnt from: tryuan99
Repo: PisterLab/micromissiles-unity PR: 110
File: Assets/Scripts/Algorithms/Clustering/Cluster.cs:123-126
Timestamp: 2025-10-17T05:39:03.474Z
Learning: In `Assets/Scripts/Algorithms/Clustering/Cluster.cs`, the `IsFullyTerminated()` method intentionally treats null agents as terminated via `?? true`, which differs from the null-handling in `Centroid()` and `Velocity()` methods that skip null agents entirely. This semantic difference is by design.
Applied to files:
Assets/Scripts/Hierarchical/IHierarchical.csAssets/Scripts/Hierarchical/HierarchicalAgent.csAssets/Scripts/Agents/AgentBase.csAssets/Scripts/Agents/IAgent.csAssets/Scripts/Algorithms/Clustering/Cluster.csAssets/Scripts/Threats/ThreatBase.cs
🧬 Code graph analysis (18)
Assets/Scripts/Interceptors/CarrierInterceptor.cs (3)
Assets/Scripts/Agents/AgentBase.cs (1)
LateUpdate(181-181)Assets/Scripts/Movements/MissileMovement.cs (2)
MissileMovement(14-120)MissileMovement(31-33)Assets/Scripts/Agents/Agent.cs (1)
FlightPhase(73-75)
Assets/Scripts/Release/MassReleaseStrategyBase.cs (3)
Assets/Scripts/Interceptors/CarrierBase.cs (1)
CarrierBase(9-73)Assets/Scripts/Release/SingleReleaseStrategyBase.cs (1)
List(15-64)Assets/Scripts/Release/IReleaseStrategy.cs (1)
List(13-13)
Assets/Scripts/Movements/MovementBase.cs (3)
Assets/Scripts/Movements/IMovement.cs (1)
Vector3(12-12)Assets/Scripts/Movements/GroundMovement.cs (1)
Vector3(12-17)Assets/Scripts/Movements/IdealMovement.cs (1)
Vector3(12-14)
Assets/Scripts/Hierarchical/IHierarchical.cs (2)
Assets/Scripts/Agents/Agent.cs (1)
IsTerminated(81-83)Assets/Scripts/Hierarchical/HierarchicalBase.cs (5)
AddSubHierarchical(42-46)RemoveSubHierarchical(48-50)AddPursuer(56-60)RemovePursuer(62-64)IsEscapingPursuers(66-87)
Assets/Scripts/Interceptors/IInterceptor.cs (1)
Assets/Scripts/Managers/SimManager.cs (1)
IInterceptor(134-167)
Assets/Scripts/Managers/InputManager.cs (1)
Assets/Scripts/Managers/SimManager.cs (1)
SimManager(7-308)
Assets/Scripts/Threats/IThreat.cs (3)
Assets/Scripts/Managers/SimManager.cs (2)
IThreat(171-202)IAgent(204-211)Assets/Scripts/Threats/ThreatBase.cs (3)
IAgent(123-140)LookupPowerTable(32-35)HandleIntercept(37-40)Assets/Scripts/Config/ConfigLoader.cs (4)
Configs(41-46)Configs(48-57)Configs(59-63)Configs(65-70)
Assets/Scripts/Agents/AgentBase.cs (2)
Assets/Scripts/Agents/Agent.cs (6)
IsTerminated(81-83)FlightPhase(73-75)Vector3(187-189)Vector3(199-201)Vector3(207-209)Vector3(405-416)Assets/Scripts/Hierarchical/HierarchicalAgent.cs (2)
HierarchicalAgent(7-33)HierarchicalAgent(30-32)
Assets/Scripts/Config/ConfigLoader.cs (1)
Assets/Scripts/Plugins/Protobuf.cs (1)
Protobuf(4-52)
Assets/Scripts/Release/SingleReleaseStrategyBase.cs (6)
Assets/Scripts/Interceptors/CarrierBase.cs (3)
CarrierBase(9-73)RegisterSubInterceptorHit(22-35)RegisterSubInterceptorMiss(38-42)Assets/Scripts/Release/ReleaseStrategyBase.cs (1)
List(12-12)Assets/Scripts/Hierarchical/HierarchicalBase.cs (1)
IsEscapingPursuers(66-87)Assets/Scripts/Utils/Coordinates3.cs (2)
Simulation(64-70)Coordinates3(11-71)Assets/Scripts/Hierarchical/HierarchicalAgent.cs (2)
HierarchicalAgent(7-33)HierarchicalAgent(30-32)Assets/Scripts/UI/UIManager.cs (2)
UIManager(9-233)LogActionMessage(89-91)
Assets/Tests/EditMode/FixedHierarchicalTests.cs (1)
Assets/Scripts/Hierarchical/HierarchicalBase.cs (1)
Vector3(89-102)
Assets/Scripts/Agents/IAgent.cs (2)
Assets/Scripts/Agents/Agent.cs (1)
IsTerminated(81-83)Assets/Scripts/Agents/AgentBase.cs (4)
MaxForwardAcceleration(106-108)MaxNormalAcceleration(110-115)UpdateTargetModel(117-133)Terminate(147-158)
Assets/Scripts/Algorithms/Clustering/Cluster.cs (2)
Assets/Scripts/Agents/Agent.cs (1)
IsTerminated(81-83)Assets/Scripts/Hierarchical/HierarchicalBase.cs (1)
Vector3(89-102)
Assets/Scripts/Hierarchical/HierarchicalBase.cs (1)
Assets/Scripts/Hierarchical/IHierarchical.cs (2)
AddPursuer(25-25)IsEscapingPursuers(28-28)
Assets/Scripts/Movements/MissileMovement.cs (3)
Assets/Scripts/Movements/MovementBase.cs (2)
Vector3(15-15)Vector3(18-29)Assets/Scripts/Movements/IMovement.cs (1)
Vector3(12-12)Assets/Scripts/Movements/AerialMovement.cs (2)
CalculateDrag(18-26)CalculateLiftInducedDrag(30-36)
Assets/Scripts/Threats/ThreatBase.cs (3)
Assets/Scripts/Threats/IThreat.cs (2)
LookupPowerTable(22-22)HandleIntercept(25-25)Assets/Scripts/Agents/AgentBase.cs (3)
Terminate(147-158)Start(166-166)CheckFloorCollision(201-204)Assets/Scripts/Agents/IAgent.cs (1)
Terminate(40-40)
Assets/Scripts/Managers/SimManager.cs (3)
Assets/Scripts/Config/ConfigLoader.cs (5)
Configs(41-46)Configs(48-57)Configs(59-63)Configs(65-70)ConfigLoader(6-96)Assets/Scripts/Release/SingleReleaseStrategyBase.cs (1)
List(15-64)Assets/Scripts/Hierarchical/HierarchicalAgent.cs (2)
HierarchicalAgent(7-33)HierarchicalAgent(30-32)
Assets/Scripts/Hierarchical/FixedHierarchical.cs (1)
Assets/Scripts/Hierarchical/HierarchicalBase.cs (2)
HierarchicalBase(9-103)Vector3(89-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build plugins for windows-latest
- GitHub Check: Test plugins on windows-latest
- GitHub Check: Test plugins on macos-latest
- GitHub Check: Test plugins on ubuntu-latest
🔇 Additional comments (62)
Assets/Scripts/Movements/MovementBase.cs (1)
20-21: LGTM! Clearer intent withProjectOnPlane.Using
Vector3.ProjectOnPlaneto compute the normal (perpendicular) component is more explicit and readable than the likely previous approach of subtracting the forward component. Both are mathematically equivalent, but this refactor improves code clarity.Assets/Scripts/Algorithms/Clustering/Cluster.cs (1)
21-25: LGTM! Properly handles empty sequences and filters terminated hierarchicals.The implementation correctly uses
DefaultIfEmpty()and a null-check to preventInvalidOperationExceptionwhen all subHierarchicals are terminated. This aligns with the termination-aware logic inHierarchicalBase.GetMean()which similarly skips terminated entries.Assets/Scripts/Agents/IAgent.cs (1)
7-8: Clean termination API design.The interface additions provide a complete termination lifecycle:
IsTerminatedfor state queriesOnTerminatedevent for reactive subscribersTerminate()for explicit terminationThis enables event-driven architecture for agent lifecycle management as described in the PR objectives.
Also applies to: 28-28, 30-30, 33-33, 40-40
Assets/Scripts/Agents/AgentBase.cs (1)
177-184: Virtual lifecycle stubs provide extension points for subclasses.Empty
Update,LateUpdate, andOnDestroystubs follow Unity's common pattern for allowing derived classes to override without requiring a base call.Assets/Scripts/Config/ConfigLoader.cs (1)
59-63: No change needed:LoadSimulatorConfigcorrectly reflects the file location.The
simulator.pbtxtfile is intentionally stored atAssets/StreamingAssets/simulator.pbtxt(the root level), not in aConfigs/Simulatorssubdirectory like other config types. The method's implementation is correct—it passesconfigFiledirectly without prepending a directory path because the file's location differs from other config types. The apparent API inconsistency is by design and should not be changed.Likely an incorrect or invalid review comment.
Assets/Proto/Simulation/flight_phase.proto (1)
12-13: Verify proto enum value renumbering context.The change to
BALLISTIC = 5andTERMINATED = 6is a wire-breaking change only if previously serialized data exists withTERMINATEDat value 5. While the codebase uses symbolic names (mitigating immediate risk), unable to confirm the prior enum state. If this proto is consumed by external systems or backward compatibility is required, the enum values should remain fixed. If this is a new feature branch with no legacy data, the change is acceptable.Assets/Tests/EditMode/AgentBaseTests.cs (1)
25-71: LGTM! Improved NUnit assertion order.The assertion argument order has been corrected to follow NUnit conventions (expected before actual). This improves test readability and ensures proper error messages when assertions fail.
Assets/Scripts/Release/MassReleaseStrategyBase.cs (1)
21-23: LGTM! Proper guard clause for carrier validation.The early-return guard correctly prevents release operations when the agent is not a CarrierBase or when no sub-interceptors remain. This aligns with the carrier lifecycle management introduced in CarrierBase.
Assets/Scripts/Hierarchical/IHierarchical.cs (1)
20-28: LGTM! Well-designed interface additions.The IsTerminated property and IsEscapingPursuers() method extend the IHierarchical interface consistently with the broader termination and pursuit-tracking architecture. The additions integrate well with implementations in HierarchicalBase and FixedHierarchical.
Assets/Scripts/Interceptors/IInterceptor.cs (1)
7-15: LGTM! Well-structured interface evolution.The interface now properly inherits from IAgent and introduces consistent event-based hit/miss signaling. This aligns well with the broader termination architecture and provides a clean event-driven pattern for interceptor lifecycle management.
Assets/Scripts/Managers/InputManager.cs (1)
185-185: LGTM! Proper migration to property-based API.The change from method call to property access aligns with SimManager's refactored API. Using properties for simple state queries follows C# best practices and improves code readability.
Assets/Scripts/Threats/IThreat.cs (1)
9-25: LGTM! Comprehensive interface evolution.The interface properly inherits from IAgent and introduces consistent event-based lifecycle management. The OnHit/OnMiss events and HandleIntercept() method provide clear semantics for threat termination scenarios, aligning well with the broader architecture refactoring.
Assets/Scripts/Movements/MissileMovement.cs (3)
62-64: LGTM!The ballistic phase case correctly routes to the new
ActBallisticmethod, maintaining consistency with the existing flight phase handling pattern.
107-110: LGTM!The ballistic phase correctly applies zero control acceleration while still accounting for drag and gravity through
ActMidCourse. The named parameter syntax makes the intent clear.
112-119: LGTM!Including gravity in the lift-induced drag calculation is physically correct—the lift force must counteract gravity in addition to any commanded normal acceleration, so the total should be considered when computing induced drag.
Assets/Scripts/Release/SingleReleaseStrategyBase.cs (5)
11-11: LGTM!Increasing
_initialSpeedfrom1e-6fto1e-3fprovides a more stable initial velocity for rotation calculation while remaining negligibly small relative to actual missile speeds.
20-23: LGTM!The carrier cast with null check is appropriate defensive programming. This ensures the release strategy gracefully handles cases where it's attached to a non-carrier agent.
27-35: LGTM!The guard conditions are well-structured:
- Prevents over-allocation by tracking remaining capacity minus already-released count
- Skips terminated targets
- Avoids redundant launches on targets already effectively pursued
50-53: LGTM!The event subscriptions correctly wire
OnHittoRegisterSubInterceptorHitandOnMisstoRegisterSubInterceptorMiss, properly handling both hit and miss outcomes for sub-interceptors.
55-59: LGTM!The logging provides useful observability for interceptor launches, with both debug console and UI output for different consumers.
Assets/Scripts/Interceptors/InterceptorBase.cs (3)
10-11: LGTM!The
OnHitandOnMissevents align with theIInterceptorinterface, enabling event-driven interception handling.
44-50: LGTM!Downgrading to
LogWarningis appropriate since a missing controller type is handled gracefully (setsController = null) rather than causing a failure.
68-80: LGTM!The threat collision logic correctly implements probabilistic kill handling:
- Invokes
OnHitand callsHandleIntercepton successful kill- Invokes
OnMisswhen probability check fails- Terminates the interceptor in both cases (consumed on collision)
Assets/Scripts/Hierarchical/HierarchicalBase.cs (4)
28-30: LGTM!The null check before
AddPursuerprevents NRE when target is set to null.
36-40: LGTM!The virtual properties correctly expose aggregated state from sub-hierarchicals.
IsTerminatedusingAll()correctly returnstruefor empty collections (no active items means effectively terminated).
66-87: LGTM!The
IsEscapingPursuersimplementation properly addresses the edge cases raised in past reviews:
- Returns
falsewhenTarget == nullorSpeed <= 0- Guards against
pursuer.Speed <= 0to prevent division by zero- Returns
true(vacuously) when there are no pursuers
89-102: LGTM!The refactored
GetMeanaddresses the past review concern about memory allocation by using a single-pass manual iteration instead ofToList(). Correctly handles the all-terminated case by returningVector3.zero.Assets/Tests/EditMode/FixedHierarchicalTests.cs (1)
4-14: LGTM!The test setup correctly initializes
FixedHierarchicalwith test values and stores it asIHierarchicalto verify polymorphic access.Assets/Scripts/Interceptors/CarrierBase.cs (2)
44-55: LGTM!Proper lifecycle management with coroutine cleanup in
OnDestroy.
62-72: Good implementation of the suggested improvement.The coroutine now correctly terminates when all sub-interceptors are released, and resets the reference to allow restart. This addresses the previous review feedback.
Assets/Tests/PlayMode/ConfigsTests.cs (2)
28-30: API migration looks correct.The method calls have been updated to match the new
SimManagerAPI (LoadNewSimulationConfigandElapsedSimulationTimeproperty).
52-53: LGTM!Property access updated consistently with the API changes.
Assets/Scripts/Hierarchical/HierarchicalAgent.cs (2)
11-14: LGTM!Properly overriding base class virtual properties to proxy to the underlying
Agent. This maintains correct polymorphic behavior.
16-28: Target lifecycle management looks correct.The setter properly manages dummy agent creation/destruction. One edge case to verify: if
base.Targetis set to a terminated/destroyed hierarchical, accessingPositionandVelocityon line 25 might yield stale or invalid data. Ensure terminated targets are filtered upstream before assignment.Assets/Scripts/IADS/IADS.cs (4)
7-24: LGTM!Clean singleton implementation with proper encapsulation of mutable collections via
AsReadOnly().
68-75: LGTM!Periodic clustering with proper guard for empty threat list.
106-119: Verify recursive target assignment is intentional.The local
AssignTargetfunction assigns the sametargetto all sub-hierarchicals recursively (line 109). This means every threat in a swarm will target the same launcher. If sub-hierarchicals should target different entities (e.g., specific carrier interceptors), this needs adjustment.Also, consider removing
Debug.Logstatements (lines 86, 97) in favor of the existingUIManagerlogging to reduce console noise in production.
91-95: Good fix for zero launchers edge case.The guard at lines 92-94 properly handles the case when no launchers are registered, preventing issues with
KMeansClusterer. This addresses the previous review feedback.Assets/Scripts/Threats/ThreatBase.cs (5)
7-8: LGTM!The speed error threshold constant is well-defined for controlling forward acceleration behavior.
29-30: LGTM!Events for hit/miss align with the PR's goal of exposing lifecycle events for threats.
37-40: LGTM!
HandleIntercept()properly invokesOnMissand terminates the threat, fulfilling the interface contract.
42-61: LGTM!Target selection logic correctly finds the nearest launcher and falls back to a fixed position at origin when no launchers exist.
63-91: LGTM!The speed-aware acceleration logic properly decomposes acceleration into forward and normal components, applying speed-based scaling to regulate thrust according to the desired waypoint power.
Assets/Scripts/UI/UIManager.cs (6)
29-43: LGTM!The new UI text handles and counter variables are well-organized for tracking interceptor hits, misses, and remaining counts for both interceptors and threats.
59-61: LGTM!Event subscriptions correctly wire up to the new interface-based events from
SimManager.
125-125: LGTM!Config loading correctly delegates to the new
SimManager.LoadNewSimulationConfigAPI.
143-145: LGTM!Time display correctly uses the new
ElapsedSimulationTimeproperty.
181-186: LGTM!Counters are properly reset when the simulation ends.
196-227: LGTM!Event handlers correctly use interface-based types (
IInterceptor,IThreat,IAgent) and properly manage counter state. The type checks inRegisterAgentTerminatedcorrectly distinguish between interceptors and threats.Assets/Scripts/Monitors/SimMonitor.cs (3)
93-124: LGTM!Telemetry recording correctly uses interface-based agent access and filters terminated agents. The binary format writes position, velocity, and agent type consistently.
127-158: LGTM!Binary-to-CSV conversion correctly reads the updated telemetry format with position and velocity components.
200-245: LGTM!Event registration methods correctly use interface types (
IThreat,IInterceptor,IAgent). The interceptor hit/miss handlers properly wire up to the newOnHit/OnMissevents.Assets/Scripts/Managers/ParticleManager.cs (4)
93-103: LGTM!Event registration correctly uses interface types and wires up to the new
OnHit/OnMissevents for both interceptors and threats.
105-130: LGTM!Handler methods correctly use interface types (
IAgent,IInterceptor,IThreat). The empty handlers for threat hit/miss are placeholders for future functionality.
132-143: LGTM!Trail renderer commandeering correctly uses interface-based type checking to determine material assignment.
161-161: LGTM!Using
inparameter forVector3is appropriate for avoiding unnecessary copies of the struct.Assets/Scripts/Managers/SimManager.cs (6)
133-167: LGTM!The interceptor creation flow with config loading, hierarchical agent assignment, cost tracking, and event invocation is well-structured.
169-202: LGTM!The threat creation logic correctly loads config, assigns the hierarchical agent, subscribes to the miss event for cost tracking, and invokes the new threat event.
204-216: LGTM!Dummy agent creation and destruction are cleanly implemented with proper list management.
218-252: LGTM!The agent creation helpers properly handle prefab loading, coordinate conversion, and randomization with configurable noise.
254-272: LGTM!The singleton pattern implementation with
DontDestroyOnLoadand conditional startup logic is correct.
283-307: LGTM!Initialization methods correctly iterate over swarm configs and fire appropriate events. Cost tracking via
RegisterThreatMissaligns with the event-based architecture for tracking neutralized threats.
| IAgent otherAgent = other.gameObject.GetComponentInParent<IAgent>(); | ||
| // Dummy agents are virtual targets and should not trigger collisions. | ||
| if (otherAgent == null || otherAgent is DummyAgent) { | ||
| return; | ||
| } | ||
| // Check if the collision is with another threat or with the intended target. | ||
| if (otherAgent is IThreat) { | ||
| OnMiss?.Invoke(this); | ||
| Terminate(); |
There was a problem hiding this comment.
Early return missing after floor collision terminates the agent.
When floor collision is detected at lines 146-149, the method invokes OnMiss and Terminate() but continues execution. Subsequent code may access the now-terminated agent or trigger additional events. Add a return after termination.
Apply this diff:
if (CheckFloorCollision(other)) {
OnMiss?.Invoke(this);
Terminate();
+ return;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Assets/Scripts/Threats/ThreatBase.cs around lines 151 to 159, the
floor-collision branch calls OnMiss?.Invoke(this) and Terminate() but does not
return, so execution continues after the agent has been terminated; add an
immediate return statement right after Terminate() to stop further processing in
that method and avoid accessing the terminated agent or firing additional
events.
| public void SnapToSwarm(List<IAgent> swarm, bool forceFreeMode = true) { | ||
| // TODO(titan): To be implemented. | ||
| } | ||
|
|
||
| public void SnapToNextInterceptorSwarm(bool forceFreeMode = true) { | ||
| if (SimManager.Instance.GetInterceptorSwarms().Count == 0) { | ||
| UIManager.Instance.LogActionWarning("[CAM] No interceptor swarms to follow"); | ||
| return; | ||
| } | ||
|
|
||
| // Set pre-set view 1 | ||
| _selectedInterceptorSwarmIndex += 1; | ||
| _selectedThreatSwarmIndex = -1; | ||
| if (_selectedInterceptorSwarmIndex >= SimManager.Instance.GetInterceptorSwarms().Count) { | ||
| _selectedInterceptorSwarmIndex = 0; | ||
| } | ||
| List<(Agent, bool)> swarm = | ||
| SimManager.Instance.GetInterceptorSwarms()[_selectedInterceptorSwarmIndex]; | ||
| string swarmTitle = SimManager.Instance.GenerateInterceptorSwarmTitle(swarm); | ||
|
|
||
| // Filter out inactive agents | ||
| List<(Agent, bool)> activeAgents = swarm.FindAll(tuple => tuple.Item2); | ||
| List<Agent> activeAgentsList = activeAgents.ConvertAll(tuple => tuple.Item1); | ||
| Vector3 swarmCenter = SimManager.Instance.GetSwarmCenter(activeAgentsList); | ||
| SetCameraTargetPosition(swarmCenter); | ||
| if (forceFreeMode) { | ||
| SetCameraMode(CameraMode.FREE); | ||
| } | ||
| UIManager.Instance.LogActionMessage($"[CAM] Snap to interceptor swarm: {swarmTitle}"); | ||
| // TODO(titan): To be implemented. | ||
| return; | ||
| } | ||
|
|
||
| public void SnapToNextThreatSwarm(bool forceFreeMode = true) { | ||
| if (SimManager.Instance.GetThreatSwarms().Count == 0) { | ||
| return; | ||
| } | ||
| _selectedInterceptorSwarmIndex = -1; | ||
| _selectedThreatSwarmIndex += 1; | ||
| if (_selectedThreatSwarmIndex >= SimManager.Instance.GetThreatSwarms().Count) { | ||
| _selectedThreatSwarmIndex = 0; | ||
| } | ||
| List<(Agent, bool)> swarm = SimManager.Instance.GetThreatSwarms()[_selectedThreatSwarmIndex]; | ||
| string swarmTitle = SimManager.Instance.GenerateThreatSwarmTitle(swarm); | ||
| // Filter out inactive agents | ||
| List<(Agent, bool)> activeAgents = swarm.FindAll(tuple => tuple.Item2); | ||
| List<Agent> activeAgentsList = activeAgents.ConvertAll(tuple => tuple.Item1); | ||
| Vector3 swarmCenter = SimManager.Instance.GetSwarmCenter(activeAgentsList); | ||
| SetCameraTargetPosition(swarmCenter); | ||
| if (forceFreeMode) { | ||
| SetCameraMode(CameraMode.FREE); | ||
| } | ||
| UIManager.Instance.LogActionMessage($"[CAM] Snap to threat swarm: {swarmTitle}"); | ||
| // TODO(titan): To be implemented. | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CameraController.cs" -type fRepository: PisterLab/micromissiles-unity
Length of output: 112
🏁 Script executed:
wc -l ./Assets/Scripts/UI/CameraController.csRepository: PisterLab/micromissiles-unity
Length of output: 116
🏁 Script executed:
sed -n '250,310p' ./Assets/Scripts/UI/CameraController.csRepository: PisterLab/micromissiles-unity
Length of output: 1888
🏁 Script executed:
sed -n '220,250p' ./Assets/Scripts/UI/CameraController.csRepository: PisterLab/micromissiles-unity
Length of output: 1056
TODO stubs in follow methods may create silent failures.
SnapToNextInterceptorSwarm and SnapToNextThreatSwarm are stubbed with early returns, but callers like FollowNextInterceptorSwarm() and FollowNextThreatSwarm() continue execution after these methods return. This causes the camera mode to switch and the centroid update coroutine to start without actually setting any swarm target position, resulting in undefined follow behavior.
Consider either disabling the follow functionality entirely until implemented, or logging a warning when these methods are called.
🤖 Prompt for AI Agents
In Assets/Scripts/UI/CameraController.cs around lines 256-268, the three TODO
stub methods (SnapToSwarm, SnapToNextInterceptorSwarm, SnapToNextThreatSwarm)
return or do nothing which lets callers proceed and start follow behavior with
no target; change these stubs so they either (a) log a clear warning/error and
early-return before any camera mode changes or coroutine starts, or (b)
implement the actual snapping behavior; minimally, add a warning log in each
method and ensure they do NOT trigger camera mode switching or start the
centroid-follow coroutine when no valid swarm target is set, and update callers
or method return semantics if needed so callers can detect the failure and avoid
continuing follow logic.
| private Vector3 GetAllAgentsCenter() { | ||
| var agentPositions = SimManager.Instance.Agents.Select(agent => agent.Position).ToList(); | ||
| if (agentPositions.Count == 0) { | ||
| return Vector3.zero; | ||
| } | ||
|
|
||
| var sum = Vector3.zero; | ||
| foreach (var position in agentPositions) { | ||
| sum += position; | ||
| } | ||
| return sum / agentPositions.Count; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using LINQ for center calculation.
The helper can be simplified using LINQ's Aggregate or a simple average pattern.
private Vector3 GetAllAgentsCenter() {
- var agentPositions = SimManager.Instance.Agents.Select(agent => agent.Position).ToList();
- if (agentPositions.Count == 0) {
+ var agents = SimManager.Instance.Agents;
+ if (!agents.Any()) {
return Vector3.zero;
}
-
- var sum = Vector3.zero;
- foreach (var position in agentPositions) {
- sum += position;
- }
- return sum / agentPositions.Count;
+ return agents.Aggregate(Vector3.zero, (sum, agent) => sum + agent.Position) / agents.Count;
}📝 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.
| private Vector3 GetAllAgentsCenter() { | |
| var agentPositions = SimManager.Instance.Agents.Select(agent => agent.Position).ToList(); | |
| if (agentPositions.Count == 0) { | |
| return Vector3.zero; | |
| } | |
| var sum = Vector3.zero; | |
| foreach (var position in agentPositions) { | |
| sum += position; | |
| } | |
| return sum / agentPositions.Count; | |
| } | |
| private Vector3 GetAllAgentsCenter() { | |
| var agents = SimManager.Instance.Agents; | |
| if (!agents.Any()) { | |
| return Vector3.zero; | |
| } | |
| return agents.Aggregate(Vector3.zero, (sum, agent) => sum + agent.Position) / agents.Count(); | |
| } |
🤖 Prompt for AI Agents
In Assets/Scripts/UI/CameraController.cs around lines 479 to 490, the
GetAllAgentsCenter method manually builds a list and loops to sum positions;
replace that with a concise LINQ-based calculation (e.g., project Positions and
use Aggregate or compute component-wise Average) and handle the empty case by
returning Vector3.zero. Ensure you avoid creating an intermediate list if
unnecessary (operate on the IEnumerable directly), and keep the result as a
Vector3 average of agent positions.
[All] Implement fully hierarchical defense strategy * [Agent] Add agent-related interfaces and base implementations (#95) * [Interceptor/Threat] Add interceptor and threat interfaces (#96) * [Algorithm] Add algorithm interfaces and base implementations (#97) * [Movements] Implement all aerial and ground-based movements (#99) * [Gemini] Disable Gemini pull request summary (#142) * [Sensor] Add sensor interface and ideal sensor implementation (#100) * [Chore] Clean up private properties (#101) * [Evasion] Implement evasion behaviors (#102) * [Agent] Add agent-related interfaces and base implementations (#95) * [Interceptor/Threat] Add interceptor and threat interfaces (#96) * [Algorithm] Add algorithm interfaces and base implementations (#97) * [Movements] Implement all aerial and ground-based movements (#99) * [Sensor] Add sensor interface and ideal sensor implementation (#100) * [Chore] Clean up private properties (#101) * [Evasion] Implement evasion behaviors (#102) * [Attack Behavior] Implement direct attack behavior (#103) * [Controller] Add controller interface and implementations (#105) * [Predictor/Planner] Clean up predictor and planner classes (#108) * [Clustering] Change clustering algorithms to act on hierarchical objects (#111) * [Utils] Clean up coordinates classes and tests (#112) * [Assignment] Refactor assignment algorithms (#113) * [Accessor] Use init accessor for read-only fields (#116) * [Interceptors/Threats] Add Legacy prefix to existing interceptor and threat class names (#121) * [Interceptors/Threats] Implement all interceptor and threat types (#122) * [Proto/Configs] Add launcher types to proto definition and simulation configs (#123) * [IADS] Clean up IADS and simulation manager (#124) * [UI] Remove swarm status dialog and tactical panel (#125) * [All] Remove all legacy code (#127) * [Prefabs] Add prefabs for launchers (#128) * [Release] Adjust minimum distance in proximity release (#129) * [Configs] Add simulation configuration with 2 launchers (#130) * [Simulation] Destroy terminated game objects (#132) * [UI] Draw gizmos for agents (#133) * [UI] Fix summary text label sizes (#134) * [Clustering] Implement recursive clustering within hierarchy (#139) * [UI] Clean up UI scripts (#140) * [Reassignment] Assign new targets to excess interceptors (#141) * [Evasion] Fix threat evasion of interceptors (#143) * [Reassignment] Use capacity remaining to reassign interceptors (#144) * [Agent] Add IsPursuable property (#145) * [Reassignment] Implement threat reassignment (#146) * [Escape] Add escape detectors (#147) * [Escape] Check for escaping targets (#148) * [Movement] Fix missile movement to avoid the ground (#149) * [Reassignment] Fix relaunching multiple carrier interceptors (#150) * [Unity] Fix serializable fields and references (#152) * [Docs] Update docs with hierarchical strategy (#153) * [Utils] Sample from normal distribution (#154) * [Proto] Add run configuration proto (#155) * [Visualization] Fix visualizing simulation monitor logs (#157) * [Tools] Build standalone player (#158) * [Manager] Add run manager to handle run configurations (#159) * [Tools] Refactor common simulation-related constants and utilities (#160) * [Tools] Add metric and aggregator classes (#161) * [Tools] Process the aggregated logs of a simulation run (#162) * [Docs] Update documentation with run configuration and logging (#163) * [Tests] Remove unused Rigidbody variables from unit tests (#166) * [Agent] Add target model API in Agent interface (#167) * [Logging] Emit more detailed events (#168) * [Metrics] Consider only missile interceptors in metrics (#169) * [Logging] Remove logging from CP-SAT solver (#171) * [Tools] Add script to animate telemetry log (#172) * [Docs] Remove unnecessary docs image files (#174) * [Interceptor] Rename IsPursuable to IsPursuer (#175) * [UI] Reimplement tactical panel (#176) * [UI] Add camera to follow and rotate around interceptors or threats (#177) * [Configs] Add configuration for 5 swarms and 1000 UCAVs (#179) * [Agent] Cache the agent transform (#180) * [Agent] Clean up finding relative transformations (#181) * [Configs] Exclude 5_swarms_1000_ucav configuration from CI (#182) * [Agents] Fix collisions with terminated agents (#184) * [Controls] Implement vector proportional navigation (#186) * [Reassignment] Find subhierarchicals with at least one active target (#187)
This PR cleans up the IADS and simulation manager to manage the hierarchical strategy. Some important changes to note: