[Reassignment] Interceptor reassignment optimizes for intercept speed #200
Conversation
|
🧱 Stack PR · Branch end (2 PRs in 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 effectively refactors the fractional speed calculation into a dedicated utility, enhancing code reuse and clarity. The core change to optimize interceptor reassignment based on intercept speed is a significant improvement. However, I've identified a critical risk of a division-by-zero error in the new FractionalSpeed utility. Additionally, there are opportunities to improve the robustness of the new reassignment logic and fix a potential logic error in target selection. Please see the detailed comments for specific suggestions.
📝 WalkthroughWalkthroughRefactors fractional speed computation into a new utility FractionalSpeed.Calculate(IAgent, Vector3) and replaces inlined calculations in MaxSpeedAssignment and SpeedEscapeDetector with calls to that method. Adds FractionalSpeed class and unit tests. Changes hierarchical target-assignment API: renames AssignNewTarget(IHierarchical,int) -> FindNewTarget(IHierarchical,int) returning IHierarchical and updates HierarchicalBase, IHierarchical, IADS, InterceptorBase. Adds IInterceptor.EvaluateReassignedTarget(IHierarchical) and implements reassignment-evaluation logic in InterceptorBase. Adjusts AgentBase max-normal-acceleration fallback behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Scripts/Hierarchical/HierarchicalBase.cs (1)
202-218: Consider handling the edge case whereassignmentsis empty.
MaxSpeedAssignment.Assignmay return an empty list. While line 216-218 checks forassignments.Count != 1, it's worth noting that this also returnsnullif more than one assignment is returned, which seems intentional but could warrant a comment for clarity.
🤖 Fix all issues with AI agents
In `@Assets/Scripts/Hierarchical/HierarchicalBase.cs`:
- Around line 223-228: The current metric returns higher-is-better for
FractionalSpeed.Calculate but lower-is-better for Vector3.Distance while the
call uses OrderByDescending uniformly; update the sorting so the comparison
aligns with metric semantics—either (A) change the LINQ to choose
OrderByDescending(metric) when hierarchical is a HierarchicalAgent and
OrderBy(metric) when using the distance fallback, or (B) normalize metric so it
always yields higher-is-better (e.g., return -Vector3.Distance(...) for the
distance branch) and keep OrderByDescending; adjust the code around the metric
lambda, the ternary branch that checks hierarchical is HierarchicalAgent /
HierarchicalAgent hierarchicalAgent, and the filteredSubHierarchicals assignment
accordingly.
In `@Assets/Scripts/Utils/FractionalSpeed.cs`:
- Around line 8-14: distanceTimeConstant and minTurningRadius can become
Infinity/NaN when config values are missing or zero; update the calculation in
FractionalSpeed to guard denominators by clamping key inputs (e.g., result of
Constants.CalculateAirDensityAtAltitude(agent.Position.y),
agent.StaticConfig.LiftDragConfig?.DragCoefficient,
agent.StaticConfig.BodyConfig?.CrossSectionalArea) to a small positive epsilon
using Mathf.Max(..., epsilon) before dividing for distanceTimeConstant, and
check MaxNormalAcceleration() (or use Mathf.Max(agent.MaxNormalAcceleration(),
epsilon)) before computing minTurningRadius to avoid division-by-zero/NaN; keep
angleTimeConstant as-is but ensure any null coalescing uses safe defaults if
used elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Assets/Scripts/Agents/AgentBase.cs`:
- Around line 116-123: MaxNormalAcceleration can produce NaN when
MaxReferenceNormalAcceleration is missing (Infinity) and Speed==0 because 0 *
Infinity => NaN; update the MaxNormalAcceleration method to guard: detect when
maxReferenceNormalAcceleration is infinite (use
float.IsInfinity/Mathf.IsInfinity) or when Speed is approximately zero
(Mathf.Approximately) and return 0f immediately, otherwise compute the normal
acceleration as before; reference the MaxNormalAcceleration method and ensure
downstream callers like FractionalSpeed.Calculate / minTurningRadius get a
finite value.
In `@Assets/Tests/EditMode/FractionalSpeedTests.cs`:
- Around line 7-28: Add a [TearDown] method to destroy the test GameObject
created in SetUp: implement a TearDown method (e.g., TearDown or Cleanup) that
checks _agent and calls UnityEngine.Object.DestroyImmediate(_agent.gameObject)
(or Destroy if running play mode) and then sets _agent = null to avoid test
pollution; reference the existing SetUp, _agent, and AgentBase so the cleanup
targets the same object created in SetUp.
- Around line 30-46: The tests Calculate_DistanceOnly_ReturnsCorrectly and
Calculate_DistanceAndBearing_ReturnsCorrectly use Assert.AreEqual for float
comparisons which can fail due to floating-point precision; update both
assertions to use the Assert.AreEqual(expected, actual, delta) overload (or
Assert.AreApproximatelyEqual equivalent) with a small tolerance (e.g., 1e-6f or
1e-5f) when calling FractionalSpeed.Calculate(_agent, targetPosition) so the
expectedFractionalSpeed is compared within that tolerance.
This PR resolves #197. The problem with the previous interceptor reassignment occurs when a missile interceptor with capacity 1 is reassigned to a new threat that is selected from a leaf hierarchical's target cluster. Previously, this selection was based on the distance from the threat's position to the cluster's centroid (so all interceptors were assigned to the same threat). This PR changes this selection process to optimize for the intercept speed.
To clean up the code, calculating the fractional speed was refactored into its standalone static class
FractionalSpeed. Furthermore, I modified theAssignNewTargetmethod toFindNewTargetto return a candidate threat. Each interceptor will then decide whether to accept the new threat or not in theEvaluateReassignedTargetmethod.In this screenshot, you can see that most interceptors are reassigned to the next swarm of threats instead of forced to turn around to deal with the single missed threat from the first swarm. The reassigned targets are also spread out depending on the interceptor locations.
cc: @Joseph0120