PredictedModule GetInitialState and Destroyed - #55
Conversation
Lifecycle events mirror PredictedIdentity
📝 WalkthroughWalkthroughThe PR adds module lifecycle management to PurrDiction. Modules gain a protected virtual ChangesModule lifecycle management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/PurrDiction/Runtime/Core/PredictedIdentity.Module.cs`:
- Around line 319-323: TriggerModuleDestroyedEvents currently iterates the live
_modules list and calls each module's TriggerDestroyedEvent, which can mutate
_modules (modules disposing themselves) and cause later modules to be skipped;
fix by iterating a stable snapshot of the collection (e.g., copy _modules to an
array or List first) and invoke TriggerDestroyedEvent on the snapshot so
mutations during callbacks do not affect which modules get destroyed. Ensure you
reference TriggerModuleDestroyedEvents, _modules, and TriggerDestroyedEvent when
applying the change.
In `@Assets/PurrDiction/Runtime/Core/PredictedModuleState.cs`:
- Around line 40-44: ResetStateToInitialState currently overwrites
fullPredictedState.state without disposing the existing state, risking resource
leaks; before assigning fullPredictedState.state = GetInitialState() (and
similarly in the other reset method around the same area), call Dispose() on the
existing fullPredictedState.state if it implements IDisposable (or cast to
IDisposable) to release resources, then assign the new initial state; ensure
state types implementing IPredictedData also implement IDisposable and
IPackedAuto as required by codegen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78544d3d-c178-4423-8483-e4043d819e96
📒 Files selected for processing (4)
Assets/PurrDiction/Runtime/Core/PredictedIdentity.Module.csAssets/PurrDiction/Runtime/Core/PredictedIdentity.csAssets/PurrDiction/Runtime/Core/PredictedModule.csAssets/PurrDiction/Runtime/Core/PredictedModuleState.cs
| private void TriggerModuleDestroyedEvents() | ||
| { | ||
| for (int i = 0; i < _modules.Count; i++) | ||
| _modules[i].TriggerDestroyedEvent(); | ||
| } |
There was a problem hiding this comment.
Avoid iterating the live _modules list during destroyed callbacks.
If a module disposes itself inside Destroyed(), _modules is mutated and later modules can be skipped. Dispatch from a snapshot (or iterate safely against mutation).
Suggested fix
private void TriggerModuleDestroyedEvents()
{
- for (int i = 0; i < _modules.Count; i++)
- _modules[i].TriggerDestroyedEvent();
+ var modulesSnapshot = new List<PredictedModule>(_modules);
+ for (int i = 0; i < modulesSnapshot.Count; i++)
+ modulesSnapshot[i].TriggerDestroyedEvent();
}📝 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 TriggerModuleDestroyedEvents() | |
| { | |
| for (int i = 0; i < _modules.Count; i++) | |
| _modules[i].TriggerDestroyedEvent(); | |
| } | |
| private void TriggerModuleDestroyedEvents() | |
| { | |
| var modulesSnapshot = new List<PredictedModule>(_modules); | |
| for (int i = 0; i < modulesSnapshot.Count; i++) | |
| modulesSnapshot[i].TriggerDestroyedEvent(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/PurrDiction/Runtime/Core/PredictedIdentity.Module.cs` around lines 319
- 323, TriggerModuleDestroyedEvents currently iterates the live _modules list
and calls each module's TriggerDestroyedEvent, which can mutate _modules
(modules disposing themselves) and cause later modules to be skipped; fix by
iterating a stable snapshot of the collection (e.g., copy _modules to an array
or List first) and invoke TriggerDestroyedEvent on the snapshot so mutations
during callbacks do not affect which modules get destroyed. Ensure you reference
TriggerModuleDestroyedEvents, _modules, and TriggerDestroyedEvent when applying
the change.
| private void ResetStateToInitialState() | ||
| { | ||
| fullPredictedState.prediction.wasOnSimulationStartCalled = false; | ||
| fullPredictedState.state = GetInitialState(); | ||
| } |
There was a problem hiding this comment.
Dispose the previous full state before resetting to a new initial state.
ResetStateToInitialState() overwrites fullPredictedState.state during setup without disposing the prior value, which can leak resources across respawns.
Suggested fix
private void ResetStateToInitialState()
{
+ fullPredictedState.Dispose();
+ fullPredictedState = default;
fullPredictedState.prediction.wasOnSimulationStartCalled = false;
fullPredictedState.state = GetInitialState();
}As per coding guidelines, "State types implementing IPredictedData must also implement IDisposable and IPackedAuto for auto-serialization codegen."
Also applies to: 61-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Assets/PurrDiction/Runtime/Core/PredictedModuleState.cs` around lines 40 -
44, ResetStateToInitialState currently overwrites fullPredictedState.state
without disposing the existing state, risking resource leaks; before assigning
fullPredictedState.state = GetInitialState() (and similarly in the other reset
method around the same area), call Dispose() on the existing
fullPredictedState.state if it implements IDisposable (or cast to IDisposable)
to release resources, then assign the new initial state; ensure state types
implementing IPredictedData also implement IDisposable and IPackedAuto as
required by codegen.
Source: Coding guidelines
|
🎉 This PR is included in version 1.3.0-beta.36 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Lifecycle events mirror PredictedIdentity.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit