Repository navigation
[Manager] Add run manager to handle run configurations - #159
Conversation
|
🧱 Stack PR · Base of stack Stack Structure:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Assets/Scripts/Monitors/SimMonitor.cs (2)
60-71: Potential timestamp inconsistency between SimMonitor and SimManager.
SimMonitor.Timestampis set independently fromSimManager.Timestamp. Both are initialized toDateTime.Now.ToString("yyyyMMdd_HHmmss")but at different times—SimManager.TimestampinAwake()andSimMonitor.TimestampinRegisterSimulationStarted(). This could lead to mismatched timestamps in logs if there's a delay between initialization and simulation start.Consider using
SimManager.Instance.Timestampdirectly inInitializeSessionDirectory()and file naming to ensure consistency, or clarify why separate timestamps are intentional.
259-269: Consider clearing the event log cache after writing.After
WriteEventsToFile()completes,_eventLogCachestill holds references to all event records. For batch runs where multiple simulations execute sequentially, clearing the cache would free memory sooner.private void WriteEventsToFile() { using (StreamWriter writer = new StreamWriter(_eventLogPath, false)) { // Write the CSV header. writer.WriteLine("Time,PositionX,PositionY,PositionZ,Event,Details"); foreach (var record in _eventLogCache) { writer.WriteLine( $"{record.Time:F2},{record.PositionX:F2},{record.PositionY:F2},{record.PositionZ:F2},{record.EventType},{record.Details}"); } } + _eventLogCache.Clear(); }Assets/Scripts/Managers/SimManager.cs (1)
89-115: Consider unsubscribing event handlers in EndSimulation.When destroying interceptors and threats, their
OnTerminatedhandlers are still subscribed. While the objects are destroyed, explicitly unsubscribing before destruction would be cleaner and prevent potential issues if object destruction is delayed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Assets/Resources/Prefabs/Managers/RunManager.prefabis excluded by!**/*.prefabAssets/Scenes/MainScene.unityis excluded by!**/*.unity
📒 Files selected for processing (13)
Assets/Resources/Prefabs/Managers/RunManager.prefab.meta(1 hunks)Assets/Scripts/Config/ConfigLoader.cs(1 hunks)Assets/Scripts/IADS/IADS.cs(1 hunks)Assets/Scripts/Managers/InputManager.cs(3 hunks)Assets/Scripts/Managers/ParticleManager.cs(1 hunks)Assets/Scripts/Managers/RunManager.cs(1 hunks)Assets/Scripts/Managers/RunManager.cs.meta(1 hunks)Assets/Scripts/Managers/SimManager.cs(11 hunks)Assets/Scripts/Monitors/SimMonitor.cs(9 hunks)Assets/Scripts/UI/CameraController.cs(1 hunks)Assets/Scripts/UI/UIManager.cs(2 hunks)Assets/Tests/PlayMode/ConfigsTests.cs(2 hunks)Tools/visualize_log.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Assets/Scripts/Config/ConfigLoader.cs (1)
Assets/Scripts/Plugins/Protobuf.cs (1)
Protobuf(4-65)
Assets/Tests/PlayMode/ConfigsTests.cs (1)
Assets/Scripts/Managers/SimManager.cs (1)
SimManager(8-384)
Assets/Scripts/Managers/RunManager.cs (2)
Assets/Scripts/Config/ConfigLoader.cs (6)
Configs(41-46)Configs(48-52)Configs(54-63)Configs(65-69)Configs(71-76)ConfigLoader(6-102)Assets/Scripts/Managers/SimManager.cs (3)
Start(300-306)SimManager(8-384)LoadNewSimulationConfig(149-162)
⏰ 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). (3)
- GitHub Check: Build plugins for windows-latest
- GitHub Check: Test plugins on windows-latest
- GitHub Check: Test plugins on macos-latest
🔇 Additional comments (22)
Assets/Scripts/Managers/RunManager.cs.meta (1)
1-2: Meta file looks fine (formatVersion + guid only).
No concerns for a new script asset meta.Assets/Resources/Prefabs/Managers/RunManager.prefab.meta (1)
1-7: Prefab .meta looks standard (PrefabImporter fields empty).
No issues spotted.Assets/Scripts/Managers/ParticleManager.cs (1)
34-41: Singleton guard is now robust (destroy + return, then assign + persist).
This prevents overwriting the canonical instance and avoids persisting duplicates across scenes.Assets/Scripts/UI/CameraController.cs (1)
225-234: Singleton guard is now robust (destroy + return, then assign + persist).
Looks consistent with the project-wide singleton hardening described in the PR.Assets/Tests/PlayMode/ConfigsTests.cs (1)
30-30: Test update correctly aligns withSimManager.Instance.ElapsedTime.
This looks like a straightforward rename sync; no additional issues in the changed lines.Also applies to: 52-53
Assets/Scripts/UI/UIManager.cs (1)
142-144: Elapsed-time UI now correctly usesSimManager.Instance.ElapsedTime.Assets/Scripts/Managers/InputManager.cs (2)
12-19: Singleton initialization is now robust (duplicate guard +DontDestroyOnLoad).
73-80: Pause toggle correctly usesSimManager.Instance.IsPaused.Assets/Scripts/Managers/RunManager.cs (2)
26-44: Good pre-scene-load CLI parsing + null handling (prevents NRE crash path).
This addresses the common “bad path / malformed config” failure mode cleanly.
118-138: Command-line parsing looks solid (supports--config fooand--config=foo).Assets/Scripts/Monitors/SimMonitor.cs (4)
22-24: LGTM on singleton and Timestamp properties.The singleton pattern and Timestamp property are correctly implemented.
40-47: LGTM on singleton guard.The Awake method correctly guards against duplicate instances and marks the object as persistent across scene loads.
150-169: Good fix: Final telemetry capture before shutdown.This addresses the previous review feedback about capturing the final 0-0.1 seconds of data. The coroutine is stopped first, then a final
RecordTelemetry()is called before destroying logging resources.
191-223: LGTM on telemetry recording.The use of
SimManager.Instance.ElapsedTimeis consistent with the new timing approach across the codebase.Assets/Scripts/Managers/SimManager.cs (8)
41-59: LGTM on new state tracking properties.The public properties for simulation state (
IsRunning,IsPaused,ElapsedTime,Timestamp,AutoRestartOnEnd) are well-designed and provide clear visibility into the simulation lifecycle.
74-77: LGTM on spawn/termination counters.These counters address previous review feedback about tracking spawned and terminated counts for accurate simulation end conditions.
137-147: LGTM on ResetAndStartSimulation.The method correctly resets all state (elapsed time, costs, spawn counters) before starting a new simulation, addressing the previous review feedback about counter resets and the naming concern.
288-298: Timestamp is only set once in Awake, not per simulation run.
Timestampis set inAwake()and never updated. For batch runs, all simulations will share the same timestamp. Meanwhile,SimMonitor.Timestampis set fresh inRegisterSimulationStarted(). This creates an inconsistency:
SimManager.Instance.Timestamp→ used inInitializeSessionDirectory()for the parent directorySimMonitor.Timestamp→ used for individual telemetry/event file namesIf this is intentional (parent directory gets batch timestamp, files get per-run timestamps), consider documenting it. Otherwise, ensure timestamps are synchronized.
337-345: Good: Logging configuration moved to LoadSimConfigs.This addresses the earlier review suggestion to have
SimManagerbe responsible for its own configuration rather than havingSimMonitormodify it.
314-319: Verify LateUpdate timing for EndSimulation.The PR notes mention that simulation end checks were moved from
FixedUpdatetoLateUpdateto avoid incomplete logs. This is correct—LateUpdateruns after all other updates in the frame, ensuring events from the current frame are captured before ending.
186-192: LGTM on interceptor naming with spawn counter.Using
_numInterceptorsSpawnedfor naming ensures unique IDs even if interceptors are removed from the list, addressing the previous review feedback.
226-231: LGTM on threat naming with spawn counter.Using
_numThreatsSpawnedfor naming ensures unique IDs, addressing the previous review feedback about potential duplicate IDs when using_threats.Count.
| public static Configs.RunConfig LoadRunConfig(string configFile) { | ||
| return LoadProtobufConfig<Configs.RunConfig>(Path.Combine("Configs/Runs", configFile), | ||
| Protobuf.Protobuf_RunConfig_GetSerializedLength, | ||
| Protobuf.Protobuf_RunConfig_LoadToBinary); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LoadRunConfig looks consistent; clarify/guard whether configFile is a filename vs path.
If users pass Configs/Runs/foo.pbtxt from CLI, Path.Combine("Configs/Runs", configFile) becomes duplicated and will fail. Consider either documenting “filename only” or normalizing when configFile already contains a directory.
| private void Awake() { | ||
| if (Instance == null) { | ||
| Instance = this; | ||
| } else { | ||
| if (Instance != null && Instance != this) { | ||
| Destroy(gameObject); | ||
| } else { | ||
| Instance = this; | ||
| } | ||
| } |
There was a problem hiding this comment.
Return immediately after destroying duplicate singleton to avoid accidental event subscriptions.
Even if Unity usually won’t call Start() on a soon-to-be-destroyed object, the safe pattern is “destroy + return”.
private void Awake() {
- if (Instance != null && Instance != this) {
- Destroy(gameObject);
- } else {
- Instance = this;
- }
+ if (Instance != null && Instance != this) {
+ Destroy(gameObject);
+ return;
+ }
+ Instance = this;
}🤖 Prompt for AI Agents
In Assets/Scripts/IADS/IADS.cs around lines 26 to 32, the Awake() method
destroys duplicate singleton instances but does not return immediately, risking
accidental further initialization or event subscriptions; update the method to
return right after Destroy(gameObject) so the duplicate instance stops executing
the rest of Awake() (and avoids running Start()/subscriptions) — i.e., call
Destroy(gameObject); then immediately return.
| if (Input.GetKeyDown(KeyCode.R)) { | ||
| SimManager.Instance.RestartSimulation(); | ||
| SimManager.Instance.EndSimulation(); | ||
| SimManager.Instance.ResetAndStartSimulation(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restart hotkey may race end-of-frame cleanup/logging; consider a one-frame defer or a single SimManager API.
Calling EndSimulation() immediately followed by ResetAndStartSimulation() assumes cleanup and observers (e.g., logging/telemetry) are fully done synchronously. If any cleanup is deferred, you can get inconsistent state or incomplete logs.
if (Input.GetKeyDown(KeyCode.R)) {
- SimManager.Instance.EndSimulation();
- SimManager.Instance.ResetAndStartSimulation();
+ // Prefer a single atomic SimManager API, or defer reset one frame after end.
+ SimManager.Instance.EndSimulation();
+ StartCoroutine(ResetNextFrame());
}
+
+ private IEnumerator ResetNextFrame() {
+ yield return null;
+ SimManager.Instance.ResetAndStartSimulation();
+ }📝 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.
| if (Input.GetKeyDown(KeyCode.R)) { | |
| SimManager.Instance.RestartSimulation(); | |
| SimManager.Instance.EndSimulation(); | |
| SimManager.Instance.ResetAndStartSimulation(); | |
| } | |
| if (Input.GetKeyDown(KeyCode.R)) { | |
| // Prefer a single atomic SimManager API, or defer reset one frame after end. | |
| SimManager.Instance.EndSimulation(); | |
| StartCoroutine(ResetNextFrame()); | |
| } | |
| private IEnumerator ResetNextFrame() { | |
| yield return null; | |
| SimManager.Instance.ResetAndStartSimulation(); | |
| } |
🤖 Prompt for AI Agents
In Assets/Scripts/Managers/InputManager.cs around lines 60-63, calling
SimManager.Instance.EndSimulation() immediately followed by
SimManager.Instance.ResetAndStartSimulation() can race with end-of-frame cleanup
or deferred observers; change this to either (a) invoke a single SimManager API
like RestartSimulation() that performs EndSimulation, waits for any deferred
cleanup (or yields a frame) and then performs ResetAndStartSimulation
internally, or (b) defer the restart by one frame from the input handler (e.g.,
schedule the ResetAndStartSimulation call via a coroutine/next-frame callback)
so EndSimulation completes synchronously before restart. Ensure the chosen fix
centralizes sequencing inside SimManager or uses a one-frame defer to avoid
inconsistent state/logging.
| public Configs.RunConfig RunConfig { get; set; } | ||
|
|
||
| // If true, the simulation is currently running. | ||
| public bool IsRunning { get; private set; } = false; | ||
|
|
||
| public int RunIndex { get; private set; } = 0; | ||
|
|
||
| public int Seed { get; private set; } = 0; | ||
|
|
||
| public bool HasRunConfig() { | ||
| return RunConfig != null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make RunConfig immutable from outside to prevent mid-run mutation.
A public setter makes batch invariants (RunIndex/Seed progression) easy to break accidentally.
- public Configs.RunConfig RunConfig { get; set; }
+ public Configs.RunConfig RunConfig { get; private set; }📝 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.
| public Configs.RunConfig RunConfig { get; set; } | |
| // If true, the simulation is currently running. | |
| public bool IsRunning { get; private set; } = false; | |
| public int RunIndex { get; private set; } = 0; | |
| public int Seed { get; private set; } = 0; | |
| public bool HasRunConfig() { | |
| return RunConfig != null; | |
| } | |
| public Configs.RunConfig RunConfig { get; private set; } | |
| // If true, the simulation is currently running. | |
| public bool IsRunning { get; private set; } = false; | |
| public int RunIndex { get; private set; } = 0; | |
| public int Seed { get; private set; } = 0; | |
| public bool HasRunConfig() { | |
| return RunConfig != null; | |
| } |
🤖 Prompt for AI Agents
In Assets/Scripts/Managers/RunManager.cs around lines 13–24, RunConfig is
currently declared with a public setter which allows external mutation; change
the property to prevent outside mutation (e.g., make it public Configs.RunConfig
RunConfig { get; private set; } or readonly and assign only via the class
constructor or an internal/explicit SetRunConfig(...) method), update
HasRunConfig() to work with the new access, and ensure all call sites that
previously set RunConfig are updated to use the intended internal/constructor
method so RunIndex/Seed progression cannot be broken mid-run.
| private void Start() { | ||
| if (RunConfig != null) { | ||
| Application.targetFrameRate = -1; | ||
|
|
||
| // Disable automatically restarting at the end of the simulation. | ||
| SimManager.Instance.AutoRestartOnEnd = false; | ||
| SimManager.Instance.OnSimulationStarted += RegisterSimulationStarted; | ||
| SimManager.Instance.OnSimulationEnded += RegisterSimulationEnded; | ||
|
|
||
| StartCoroutine(Run()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Defensively handle SimManager.Instance not being ready (script execution order).
If RunManager.Start() runs before SimManager is initialized, this will NRE in batch mode. Even if it “usually works”, it’s brittle across Unity execution order changes.
private void Start() {
if (RunConfig != null) {
Application.targetFrameRate = -1;
// Disable automatically restarting at the end of the simulation.
- SimManager.Instance.AutoRestartOnEnd = false;
- SimManager.Instance.OnSimulationStarted += RegisterSimulationStarted;
- SimManager.Instance.OnSimulationEnded += RegisterSimulationEnded;
+ if (SimManager.Instance == null) {
+ Debug.LogError("RunManager started in batch mode but SimManager.Instance is null.");
+ SimManager.Instance?.QuitSimulation();
+ return;
+ }
+ SimManager.Instance.AutoRestartOnEnd = false;
+ SimManager.Instance.OnSimulationStarted += RegisterSimulationStarted;
+ SimManager.Instance.OnSimulationEnded += RegisterSimulationEnded;
StartCoroutine(Run());
}
}📝 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 void Start() { | |
| if (RunConfig != null) { | |
| Application.targetFrameRate = -1; | |
| // Disable automatically restarting at the end of the simulation. | |
| SimManager.Instance.AutoRestartOnEnd = false; | |
| SimManager.Instance.OnSimulationStarted += RegisterSimulationStarted; | |
| SimManager.Instance.OnSimulationEnded += RegisterSimulationEnded; | |
| StartCoroutine(Run()); | |
| } | |
| } | |
| private void Start() { | |
| if (RunConfig != null) { | |
| Application.targetFrameRate = -1; | |
| // Disable automatically restarting at the end of the simulation. | |
| if (SimManager.Instance == null) { | |
| Debug.LogError("RunManager started in batch mode but SimManager.Instance is null."); | |
| return; | |
| } | |
| SimManager.Instance.AutoRestartOnEnd = false; | |
| SimManager.Instance.OnSimulationStarted += RegisterSimulationStarted; | |
| SimManager.Instance.OnSimulationEnded += RegisterSimulationEnded; | |
| StartCoroutine(Run()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In Assets/Scripts/Managers/RunManager.cs around lines 53 to 64, the Start method
assumes SimManager.Instance is already initialized which can cause a null
reference if RunManager executes first; modify Start to defensively check
SimManager.Instance for null and handle it by either (a) acquiring the instance
via FindObjectOfType<SimManager>() if available, or (b) yielding until
SimManager.Instance != null (e.g., StartCoroutine to wait a few frames) before
accessing AutoRestartOnEnd and subscribing to events, and log a clear warning if
SimManager cannot be found after a timeout so batch runs don't NRE.
| private void SetGameSpeed() { | ||
| if (IsPaused) { | ||
| Time.fixedDeltaTime = 0; | ||
| SetTimeScale(timeScale: 0); | ||
| } else { | ||
| Time.fixedDeltaTime = 1.0f / SimulatorConfig.PhysicsUpdateRate; | ||
| SetTimeScale(SimulationConfig.TimeScale); | ||
| } | ||
| } |
There was a problem hiding this comment.
Setting Time.fixedDeltaTime = 0 when paused may cause issues.
Setting fixedDeltaTime to 0 is unconventional and may cause unexpected behavior in Unity's physics system. The standard approach for pausing is to set Time.timeScale = 0 while keeping fixedDeltaTime at its original value.
Consider this alternative:
private void SetGameSpeed() {
if (IsPaused) {
- Time.fixedDeltaTime = 0;
SetTimeScale(timeScale: 0);
} else {
Time.fixedDeltaTime = 1.0f / SimulatorConfig.PhysicsUpdateRate;
SetTimeScale(SimulationConfig.TimeScale);
}
}📝 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 void SetGameSpeed() { | |
| if (IsPaused) { | |
| Time.fixedDeltaTime = 0; | |
| SetTimeScale(timeScale: 0); | |
| } else { | |
| Time.fixedDeltaTime = 1.0f / SimulatorConfig.PhysicsUpdateRate; | |
| SetTimeScale(SimulationConfig.TimeScale); | |
| } | |
| } | |
| private void SetGameSpeed() { | |
| if (IsPaused) { | |
| SetTimeScale(timeScale: 0); | |
| } else { | |
| Time.fixedDeltaTime = 1.0f / SimulatorConfig.PhysicsUpdateRate; | |
| SetTimeScale(SimulationConfig.TimeScale); | |
| } | |
| } |
| private bool ShouldEndSimulation() { | ||
| if (IsRunning && ElapsedTime >= SimulationConfig.EndTime) { | ||
| return true; | ||
| } | ||
| // The simulation can be ended before the actual end time if there is a run configuration and | ||
| // all spawned threats have been terminated. | ||
| if (RunManager.Instance.HasRunConfig() && _numThreatsSpawned > 0 && | ||
| _numThreatsTerminated >= _numThreatsSpawned) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Missing IsRunning check in the early termination condition.
The second condition for ending the simulation (all threats terminated) doesn't check IsRunning. If the simulation has already ended but PostSimulation triggers another check, this could cause issues.
private bool ShouldEndSimulation() {
if (IsRunning && ElapsedTime >= SimulationConfig.EndTime) {
return true;
}
// The simulation can be ended before the actual end time if there is a run configuration and
// all spawned threats have been terminated.
- if (RunManager.Instance.HasRunConfig() && _numThreatsSpawned > 0 &&
+ if (IsRunning && RunManager.Instance.HasRunConfig() && _numThreatsSpawned > 0 &&
_numThreatsTerminated >= _numThreatsSpawned) {
return true;
}
return false;
}📝 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 bool ShouldEndSimulation() { | |
| if (IsRunning && ElapsedTime >= SimulationConfig.EndTime) { | |
| return true; | |
| } | |
| // The simulation can be ended before the actual end time if there is a run configuration and | |
| // all spawned threats have been terminated. | |
| if (RunManager.Instance.HasRunConfig() && _numThreatsSpawned > 0 && | |
| _numThreatsTerminated >= _numThreatsSpawned) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| private bool ShouldEndSimulation() { | |
| if (IsRunning && ElapsedTime >= SimulationConfig.EndTime) { | |
| return true; | |
| } | |
| // The simulation can be ended before the actual end time if there is a run configuration and | |
| // all spawned threats have been terminated. | |
| if (IsRunning && RunManager.Instance.HasRunConfig() && _numThreatsSpawned > 0 && | |
| _numThreatsTerminated >= _numThreatsSpawned) { | |
| return true; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In Assets/Scripts/Managers/SimManager.cs around lines 372 to 383, the early
termination branch (all spawned threats terminated) lacks an IsRunning check;
update that condition so it only returns true when the simulation is currently
running — e.g. include IsRunning && RunManager.Instance.HasRunConfig() &&
_numThreatsSpawned > 0 && _numThreatsTerminated >= _numThreatsSpawned — or add
an early guard that returns false if !IsRunning before evaluating the
threat-termination logic.
| private void InitializeSessionDirectory() { | ||
| string timestamp = DateTime.Now.ToString("yyyyMMdd_HHmmss"); | ||
| _sessionDirectory = Path.Combine(Application.persistentDataPath, "Logs", timestamp); | ||
| if (RunManager.Instance.HasRunConfig()) { | ||
| _sessionDirectory = | ||
| Path.Combine(Application.persistentDataPath, "Logs", | ||
| $"{RunManager.Instance.RunConfig.Name}_{SimManager.Instance.Timestamp}", | ||
| $"run_{RunManager.Instance.RunIndex + 1}_seed_{RunManager.Instance.Seed}"); | ||
| } else { | ||
| _sessionDirectory = Path.Combine(Application.persistentDataPath, "Logs", | ||
| $"run_{SimManager.Instance.Timestamp}"); | ||
| } | ||
| Directory.CreateDirectory(_sessionDirectory); | ||
| Debug.Log($"Monitoring simulation logs to {_sessionDirectory}."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "RunManager.cs" --exec cat -n {} +Repository: PisterLab/micromissiles-unity
Length of output: 5216
🏁 Script executed:
fd -t f "SimMonitor.cs" -exec head -n 200 {} +Repository: PisterLab/micromissiles-unity
Length of output: 303
🏁 Script executed:
fd "SimMonitor.cs" -t fRepository: PisterLab/micromissiles-unity
Length of output: 153
🏁 Script executed:
cat -n "Assets/Scripts/Monitors/SimMonitor.cs" | head -n 200Repository: PisterLab/micromissiles-unity
Length of output: 7998
Add null safety check before accessing RunManager.Instance.
At line 121, RunManager.Instance can be null if RunManager hasn't been initialized (e.g., when running in non-batch mode without a config file). This will cause a NullReferenceException when calling HasRunConfig(). Add a null check:
if (RunManager.Instance != null && RunManager.Instance.HasRunConfig()) {🤖 Prompt for AI Agents
In Assets/Scripts/Monitors/SimMonitor.cs around lines 120 to 132, calling
RunManager.Instance directly can cause a NullReferenceException; first cache
RunManager.Instance into a local variable and check it for null before calling
HasRunConfig(), and when building the path use that cached variable only after
verifying it's non-null (otherwise fall back to the timestamp-only Logs path);
finally ensure Directory.CreateDirectory and the Debug.Log use the computed
_sessionDirectory.
| private void Awake() { | ||
| if (Instance == null) | ||
| Instance = this; | ||
| else | ||
| if (Instance != null && Instance != this) { | ||
| Destroy(gameObject); | ||
| } else { | ||
| Instance = this; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Singleton guard is correct; consider early-return after Destroy(gameObject) for future safety.
Right now it’s fine, but adding return; avoids any later-added Awake logic running on a soon-to-be-destroyed instance.
private void Awake() {
if (Instance != null && Instance != this) {
Destroy(gameObject);
+ return;
} else {
Instance = this;
}
}📝 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 void Awake() { | |
| if (Instance == null) | |
| Instance = this; | |
| else | |
| if (Instance != null && Instance != this) { | |
| Destroy(gameObject); | |
| } else { | |
| Instance = this; | |
| } | |
| } | |
| private void Awake() { | |
| if (Instance != null && Instance != this) { | |
| Destroy(gameObject); | |
| return; | |
| } else { | |
| Instance = this; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Assets/Scripts/UI/UIManager.cs around lines 90 to 96, the Awake() singleton
guard calls Destroy(gameObject) when a duplicate instance is found but does not
early-return; modify Awake so that immediately after Destroy(gameObject) the
method returns (i.e., add a return statement) to prevent any subsequent Awake
logic from running on the destroyed duplicate and ensure Instance is only set by
the surviving instance.
| list_of_files = glob.glob( | ||
| os.path.join(directory, "**", file_pattern), | ||
| recursive=True, | ||
| ) | ||
| if not list_of_files: | ||
| logging.warning( | ||
| f"No files found matching the pattern {file_pattern} in the directory: {directory}." | ||
| ) | ||
| logging.warning(f"No files found matching the pattern {file_pattern} " | ||
| f"in the directory: {directory}.") | ||
| return None | ||
| latest_file = max(list_of_files, key=os.path.getctime) | ||
| logging.info(f"Using latest file found: {latest_file}.") |
There was a problem hiding this comment.
Fix return type + “latest” timestamp semantics (and prefer parameterized logging).
_find_latest_file returns None when no matches but is annotated as -> str. Also getctime can be misleading on Unix (ctime = metadata change), so “latest” may not mean “newest log”.
import glob
import os
import platform
+from typing import Optional
@@
-def _find_latest_file(directory: str, file_pattern: str) -> str:
+def _find_latest_file(directory: str, file_pattern: str) -> Optional[str]:
@@
if not list_of_files:
- logging.warning(f"No files found matching the pattern {file_pattern} "
- f"in the directory: {directory}.")
+ logging.warning(
+ "No files found matching pattern %r in directory: %s.",
+ file_pattern,
+ directory,
+ )
return None
- latest_file = max(list_of_files, key=os.path.getctime)
+ latest_file = max(list_of_files, key=os.path.getmtime)
logging.info(f"Using latest file found: {latest_file}.")
return latest_fileCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Tools/visualize_log.py around lines 46–55, the function currently declares ->
str but returns None on no matches, uses os.path.getctime (which is inode-change
time on Unix) and uses f-strings for logs; change the return annotation to
Optional[str], keep returning None for no matches (or raise if caller requires),
replace os.path.getctime with os.path.getmtime to reflect file modification time
for "newest" logs, switch logging calls to use parameterized logging (e.g.
logging.warning("No files found matching %s in %s", file_pattern, directory) and
logging.info("Using newest file found: %s", newest_file)), and if useful include
the mtime in the info log by formatting it from os.path.getmtime before logging.
daniellovell
left a comment
daniellovell
left a comment
There was a problem hiding this comment.
the rabbit seems to not know we need to collapse this stack
[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 adds a run manager to handle simulations with a run configuration. The run configuration allows kicking off simulation runs from the command line and running batch simulations. The run manager parses the run configuration provided through a commandline flag. The run manager then sets the seed and effectively drives the
SimManagerin loading the simulation configuration, starting a simulation, and ending a simulation.Other changes:
SimManagertracks the number of spawned interceptors, spawned threats, and terminated threats. If a run manager is provided, the simulation can end after all threats have been terminated.run_<timestamp>since there could be multiple different simulation configurations being run depending on the user actions. If a run configuration is provided, a parent directory called<run configuration name>_<timestamp>is generated. All individual runs are then placed in subdirectories calledrun_<run index+1>_seed_<seed>.SimManagerchecks for whether to end a simulation in theLateUpdateinstead ofFixedUpdate. Ending a simulation inFixedUpdateis not a good idea because if the simulation ends before an interceptor or threat event is logged, the log will be incomplete. It is better to end the simulation inLateUpdateas a result.Awakefunction whether theInstanceis not equal to null and not equal tothis.