[IADS] Visualize and assign threats to target assets #202
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 introduces assets into the simulation, which are now the primary targets for threats. The implementation includes visualizing these assets and updating the threat assignment logic in the IADS to target the closest asset. The changes are logical and well-aligned with the PR's goal. I've provided a couple of suggestions to improve performance: one related to finding the closest asset more efficiently, and another regarding how asset colors are set in Unity to avoid performance pitfalls. Overall, this is a good addition.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughSimManager adds asset lifecycle support: a NewAssetEventHandler delegate, an OnNewAsset event, and InitializeAssets invoked during StartSimulation to create assets from SimulationConfig.AssetConfigs (assets are colored orange and raise OnNewAsset). IADS adds a private _assets list, a public Assets read-only property, subscribes to SimManager.OnNewAsset, exposes RegisterNewAsset, and adjusts hierarchy targeting to prefer the closest asset to each swarm, falling back to the launcher if no assets exist. AgentBase now logs a warning for unknown SensorConfig types instead of an error. Two model config files (cvn.pbtxt, ddg.pbtxt) have their cost fields set to 0. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🤖 Fix all issues with AI agents
In `@Assets/Scripts/Agents/AgentBase.cs`:
- Around line 224-228: The default case in AgentBase that currently logs a
warning for an unknown AgentConfig.DynamicConfig?.SensorConfig?.Type should not
leave Sensor null; either restore the original LogError or (preferred) assign a
fallback sensor instance and still log a warning so agents continue to
function—e.g., in the default branch of the switch create/assign an IdealSensor
(or other fallback) to the Sensor field and call Debug.LogWarning with the
unknown type and note that fallback was used; ensure UpdateTargetModel continues
to operate with the fallback Sensor.
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 39-45: The Start method subscribes to SimManager events
(RegisterSimulationStarted, RegisterSimulationEnded, RegisterNewAsset,
RegisterNewLauncher, RegisterNewThreat) but never unsubscribes; add an
OnDestroy() that checks SimManager.Instance != null and unsubscribes each
handler (using -=) from OnSimulationStarted, OnSimulationEnded, OnNewAsset,
OnNewLauncher, and OnNewThreat to prevent callbacks to a destroyed IADS
instance.
- Around line 117-124: The LINQ selection for closestAsset currently considers
terminated assets in _assets; update the query in the AssignTarget logic to
filter out terminated/destroyed assets (e.g., _assets.Where(a =>
!a.IsTerminated) or a.IsAlive flag) before OrderBy, or modify RegisterNewAsset
to subscribe to the asset's termination event and remove it from the _assets
collection when terminated (following the interceptor removal pattern used in
SimManager). Ensure the symbol names referenced are _assets, AssignTarget, and
RegisterNewAsset so the change is applied to the correct location.
In `@Assets/Scripts/Managers/SimManager.cs`:
- Around line 323-331: InitializeAssets is dereferencing possible nulls:
CreateInterceptor(...) can return null and
GetComponentInChildren<MeshRenderer>() can return null; update InitializeAssets
to guard those cases by checking the returned IInterceptor from
CreateInterceptor and skipping (or logging) when it's null before accessing
asset.gameObject, and check the MeshRenderer result before using
renderer.material.color (skip coloring if renderer is null); keep invoking
OnNewAsset only for non-null asset instances and use descriptive logging when
either is missing to aid debugging.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Assets/Scripts/Managers/SimManager.cs`:
- Around line 326-340: In InitializeAssets, replace the single-run
GetComponentInChildren<MeshRenderer>() logic so you retrieve all mesh renderers
for the asset (use asset.gameObject.GetComponentsInChildren<MeshRenderer>()) and
loop through each renderer; for every renderer build/apply a
MaterialPropertyBlock and set color for both common shader properties
("_BaseColor" and "_Color") or choose the property dynamically by checking
renderer.sharedMaterial.HasProperty before setting, then call
renderer.SetPropertyBlock for each renderer so multi-mesh assets and both
URP/HDRP and Standard shaders receive the color change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Assets/Scripts/Managers/SimManager.cs`:
- Around line 331-336: The current code calls
asset.gameObject.GetComponentsInChildren<MeshRenderer>() which misses other
renderer types (e.g., SkinnedMeshRenderer); change the call to
asset.gameObject.GetComponentsInChildren<Renderer>() so all Renderer subclasses
are returned, then continue using the existing MaterialPropertyBlock setup
(propertyBlock.SetColor("_BaseColor", _assetColor);
propertyBlock.SetColor("_Color", _assetColor);
renderer.SetPropertyBlock(propertyBlock)) so tinting applies to all mesh/skin
types.
- Around line 326-339: The InitializeAssets loop currently calls
CreateInterceptor which incorrectly adds assets to interceptor accounting
(_interceptors, _numInterceptorsSpawned, CostLaunchedInterceptors) and skews
UI/metrics; fix by introducing a dedicated creation path: either add a new
CreateAsset(...) method (used by InitializeAssets) that mirrors
CreateInterceptor's instantiation logic but does NOT modify _interceptors,
_numInterceptorsSpawned, or CostLaunchedInterceptors and still fires OnNewAsset,
or add a boolean flag to CreateInterceptor (e.g., isAsset) that, when true,
skips updating those interceptor counters and collections; update
InitializeAssets to call the new CreateAsset or pass the flag so assets are
colored and emitted via OnNewAsset without affecting interceptor metrics.
This PR visualizes the assets and changes the IADS to assign the threats to target the closest asset. Assets are drawn with an orange color in the 3D view: