[tryuan99]

This PR adds the interceptor types VESSEL and SHORE_BATTERY to the AgentType proto enumeration and regenerates the plugins. This PR also modifies all simulation configurations to define launchers.

[daniellovell]

LGTM

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

Assets/StreamingAssets/Configs/Simulations/7_ucav.pbtxt (1)

24-51: Inconsistent syntax: extra colon on line 39.

Line 39 uses agent_config: with a colon, while line 26 uses agent_config without a colon. While protobuf text format may accept both, this inconsistency should be fixed for maintainability.

Apply this diff:

        sub_agent_config {
          num_sub_agents: 7
-          agent_config: {
+          agent_config {
            config_file: "micromissile.pbtxt"

Assets/StreamingAssets/Configs/Simulations/5_swarms_7_ucav.pbtxt (1)

5-51: Consistent with other simulation configs.

The interceptor hierarchy changes (DDG → 126 hydra70 → 7 micromissiles each) match the pattern across all updated simulation files.

Note: Line 39 has the same agent_config: syntax inconsistency flagged in 7_ucav.pbtxt.

Assets/StreamingAssets/Configs/Simulations/5_swarms_7_quadcopters.pbtxt (1)

5-54: Interceptor configuration is identical across simulation files.

This interceptor block (DDG → hydra70 → micromissile hierarchy) is duplicated verbatim across multiple simulation files. Consider extracting the common interceptor configuration to a shared include file or template to reduce maintenance burden and ensure consistency.

♻️ Duplicate comments (3)

Assets/Proto/Configs/static_config.proto (1)

9-17: Enum values updated with new launcher types.

The addition of VESSEL and SHORE_BATTERY types with the renumbering of existing values has been discussed in previous reviews. Since the author confirmed this is for internal use only with no persisted data compatibility concerns, the changes are acceptable.

Assets/StreamingAssets/Configs/Simulations/5_swarms_500_ucav.pbtxt (1)

5-51: Consistent configuration changes.

Same interceptor hierarchy pattern applied. Line 39 has the agent_config: syntax inconsistency noted in other files.

Assets/StreamingAssets/Configs/Simulations/5_swarms_100_ucav.pbtxt (1)

5-51: Consistent configuration changes.

Same interceptor hierarchy pattern applied. Line 39 has the agent_config: syntax inconsistency noted in other files.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a377ecb and 7c8dea5.

⛔ Files ignored due to path filters (5)

• Assets/Plugins/assignment.dll is excluded by !**/*.dll
• Assets/Plugins/libprotobuf.dylib is excluded by !**/*.dylib
• Assets/Plugins/libprotobuf.so is excluded by !**/*.so
• Assets/Plugins/protobuf.dll is excluded by !**/*.dll
• Assets/Scripts/Generated/Proto/StaticConfig.cs is excluded by !**/generated/**

📒 Files selected for processing (16)

• Assets/Proto/Configs/static_config.proto (1 hunks)
• Assets/Scripts/Algorithms/Prediction/LinearExtrapolator.cs (1 hunks)
• Assets/StreamingAssets/Configs/Models/ddg.pbtxt (1 hunks)
• Assets/StreamingAssets/Configs/Models/ddg.pbtxt.meta (1 hunks)
• Assets/StreamingAssets/Configs/Models/hydra70.pbtxt (1 hunks)
• Assets/StreamingAssets/Configs/Models/shore_battery.pbtxt (1 hunks)
• Assets/StreamingAssets/Configs/Models/shore_battery.pbtxt.meta (1 hunks)
• Assets/StreamingAssets/Configs/Simulations/5_swarms_100_ucav.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/5_swarms_500_ucav.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/5_swarms_7_brahmos.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/5_swarms_7_quadcopters.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/5_swarms_7_ucav.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/7_brahmos.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/7_quadcopters.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/7_quadcopters_15_ucav.pbtxt (2 hunks)
• Assets/StreamingAssets/Configs/Simulations/7_ucav.pbtxt (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

Assets/Scripts/Algorithms/Prediction/LinearExtrapolator.cs (2)

Assets/Scripts/Algorithms/Prediction/IPredictor.cs (1)

• PredictorState (8-8)

Assets/Scripts/Algorithms/Prediction/PredictorBase.cs (1)

• PredictorState (10-10)

⏰ 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: Test plugins on windows-latest
• GitHub Check: Test plugins on macos-latest
• GitHub Check: Test plugins on ubuntu-latest
• GitHub Check: Build plugins for windows-latest

🔇 Additional comments (11)

Assets/StreamingAssets/Configs/Models/hydra70.pbtxt (1)

24-27: LGTM!

The visualization symbol names now correctly align with the CARRIER_INTERCEPTOR agent type, improving consistency.

Assets/StreamingAssets/Configs/Models/shore_battery.pbtxt (1)

1-10: LGTM!

The minimal configuration is appropriate for a stationary launcher platform. The SHORE_BATTERY agent type aligns with the new enum value added in the proto definition.

Assets/StreamingAssets/Configs/Models/shore_battery.pbtxt.meta (1)

1-7: LGTM!

Standard Unity meta file with a unique GUID for the new shore_battery.pbtxt asset.

Assets/StreamingAssets/Configs/Models/ddg.pbtxt.meta (1)

1-7: LGTM!

Standard Unity meta file with a unique GUID for the new ddg.pbtxt asset.

Assets/StreamingAssets/Configs/Simulations/5_swarms_7_brahmos.pbtxt (2)

3-55: Interceptor swarm configuration is well-structured.

The hierarchical interceptor configuration matches the pattern established in other simulation files. The DDG → Hydra70 → Micromissile hierarchy is correctly configured.

---

56-270: Threat swarm configurations are correctly placed at top level.

The five threat_swarm_configs blocks are properly positioned as top-level fields, with staggered z-positions (30000 to 50000) creating waves of incoming threats. This addresses the nesting issue flagged in a previous review.

Assets/StreamingAssets/Configs/Simulations/7_quadcopters.pbtxt (1)

4-53: Hierarchical interceptor structure implemented correctly.

The three-level hierarchy (DDG → Hydra70 × 126 → Micromissile × 7) is well-structured. The sensor frequency differentiation (1000 Hz for DDG, 100 Hz for sub-agents) is appropriate for the carrier platform. All referenced model configs (ddg.pbtxt, hydra70.pbtxt, micromissile.pbtxt) exist in the Models directory.

Assets/StreamingAssets/Configs/Simulations/7_ucav.pbtxt (1)

5-17: LGTM on the DDG configuration with initial state.

The switch to ddg.pbtxt with explicit initial state (position at origin, velocity 5 m/s in x-direction) aligns with the vessel launcher type being added in this PR.

Assets/StreamingAssets/Configs/Models/ddg.pbtxt (1)

1-15: LGTM. DDG model configuration is sound.

The configuration values are reasonable and consistent with the project:

• Mass of 9,000,000 kg (~9,000 metric tons) matches real destroyer displacement
• Reference speed of 13 m/s (~25 knots) is appropriate for naval vessels
• Zero forward acceleration suits a launcher platform role
• Visualization symbols follow the established naming convention used across all other model configs

Assets/StreamingAssets/Configs/Simulations/7_brahmos.pbtxt (1)

5-54: Configuration approved for brahmos scenario.

The hierarchical interceptor setup is appropriate for engaging high-speed supersonic threats. The configuration matches the pattern established across other simulation files.

Assets/StreamingAssets/Configs/Simulations/7_quadcopters_15_ucav.pbtxt (1)

5-54: Interceptor hierarchy configuration is valid with all referenced config files present.

The three-tier hierarchy (DDG → 126 hydra70 → 7 micromissiles each = 882 total interceptors) aligns with the PR objective of adding launcher types. All referenced config files exist: ddg.pbtxt, hydra70.pbtxt, and micromissile.pbtxt are properly located in Assets/StreamingAssets/Configs/Models/. The configuration structure is consistent with the hierarchical agent model.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find the LinearExtrapolator class definition
fd "LinearExtrapolator.cs" -x wc -l {}

Repository: PisterLab/micromissiles-unity

Length of output: 204

---

🏁 Script executed:

# Read the file to see the full context
cat -n Assets/Scripts/Algorithms/Prediction/LinearExtrapolator.cs

Repository: PisterLab/micromissiles-unity

Length of output: 999

---

🏁 Script executed:

# Find PredictorBase class to understand inheritance
fd "PredictorBase.cs" -x head -50 {}

Repository: PisterLab/micromissiles-unity

Length of output: 478

---

🏁 Script executed:

# Search for Hierarchical property definition and assignments
rg -n "Hierarchical" --type=cs -B2 -A2

Repository: PisterLab/micromissiles-unity

Length of output: 50385

---

🏁 Script executed:

# Look at PlannerReleaseStrategy to see how Hierarchical is set
cat -n Assets/Scripts/Release/PlannerReleaseStrategy.cs | head -20

Repository: PisterLab/micromissiles-unity

Length of output: 672

---

🏁 Script executed:

# Check if target can be null in real usage
rg -n "\.Hierarchical\s*=" --type=cs -B1 -A1 | head -40

Repository: PisterLab/micromissiles-unity

Length of output: 368

---

🏁 Script executed:

# Look for MassReleaseStrategyBase usage to understand the null scenario
cat -n Assets/Scripts/Release/MassReleaseStrategyBase.cs | grep -A5 "IHierarchical target"

Repository: PisterLab/micromissiles-unity

Length of output: 284

---

Hierarchical can be null due to the public settable property, despite the constructor requiring a non-null parameter.

The null-check is necessary because Hierarchical has a public setter that allows it to be set to null after construction. This occurs in practice—for example, PlannerReleaseStrategy sets Planner.Predictor.Hierarchical = target where target can be null. Returning an empty PredictorState() with zero vectors silently masks this condition, which could lead to incorrect trajectory predictions.

Consider:

1. Add logging: At minimum, log a warning when this occurs to aid debugging.

   public override PredictorState Predict(float time) {
     if (Hierarchical == null) {
+      Debug.LogWarning("LinearExtrapolator.Predict called with null Hierarchical - returning default state");
       return new PredictorState();
     }

2. Document the behavior: If returning a zero state is intentional for null targets, document when and why this occurs.

3. Consider throwing: If null Hierarchical represents a genuine error condition (likely given the non-nullable constructor), throwing an exception would surface bugs immediately rather than silently producing zero predictions.

📝 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.

Suggested change

	if (Hierarchical == null) {
	return new PredictorState();
	}
	if (Hierarchical == null) {
	Debug.LogWarning("LinearExtrapolator.Predict called with null Hierarchical - returning default state");
	return new PredictorState();
	}

🤖 Prompt for AI Agents

In Assets/Scripts/Algorithms/Prediction/LinearExtrapolator.cs around lines
11-13, Hierarchical can be set to null after construction but the current code
silently returns a default PredictorState(); update the method to detect null
Hierarchical and handle it explicitly: add a warning log entry when Hierarchical
is null (include contextual info like caller state or target), and either
document the decision in the method summary comment that a null Hierarchical
yields an empty PredictorState or, if null represents an error in your domain,
throw an ArgumentNullException/InvalidOperationException instead of returning
zeros; implement one consistent approach (log+document for tolerated nulls, or
throw for invalid state) and adjust callers/tests accordingly.