Fix runtime item persistence across save reloads - #138
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Greptile SummaryThis PR fixes runtime item definitions disappearing after a save→menu→reload cycle by introducing
Confidence Score: 4/5Safe to merge. The retention logic is straightforward, well-tested, and guarded by a snapshot-under-lock pattern that avoids holding the lock during native registry calls. The core retention mechanism, the Forget-on-unregister wiring, and the descriptor validation fix are all correct. Two gaps worth monitoring: _adapter is read outside the lock in ReapplyAll (harmless in production but imprecise), and the public RegisterItem/EnsureItemRegistered APIs bypass retention entirely with no documentation warning, which could cause confusion for mod authors doing advanced registration outside the builder pattern. Files Needing Attention: S1API/Items/ItemManager.cs — RegisterItem and EnsureItemRegistered do not call Retain, so items registered through those paths will still vanish after a reload. S1API/Internal/Items/RuntimeItemDefinitionRegistry.cs — _adapter field lacks volatile.
|
| Filename | Overview |
|---|---|
| S1API/Internal/Items/RuntimeItemDefinitionRegistry.cs | New class that retains builder-created item definitions and re-registers them at OnPreLoad after native registry purges them during scene transitions. Logic is correct; minor: _adapter is read outside the lock in ReapplyAll(). |
| S1API/Items/ItemManager.cs | UnregisterItem now calls RuntimeItemDefinitionRegistry.Forget() conditioned on confirmed native removal. Public RegisterItem and EnsureItemRegistered APIs do not call Retain, creating a silent persistence gap for items registered outside the builder. |
| S1API/Items/Storable/StorableItemDefinitionBuilder.cs | Build() now calls RuntimeItemDefinitionRegistry.Retain() immediately after AddToRegistry(). All subclass builders delegate to base.Build(), so they all inherit the retention behavior. |
| S1API/Internal/Products/CustomProductSavePersistence.cs | TryValidate now accepts vanilla (non-namespaced) RepresentationTemplateIds by replacing the strict ProductKindId.Normalize() call with a simple IsNullOrWhiteSpace guard. Change is correct and widens allowed input appropriately. |
| S1API.Tests/Items/RuntimeItemDefinitionRegistryTests.cs | New unit tests cover the three key retention paths: re-apply when absent, skip when already present, and honour Forget(). RecordingAdapter correctly isolates native registry calls. |
| S1API.Tests/Products/CustomProductSavePersistenceTests.cs | Tests confirm vanilla template IDs pass and empty/whitespace IDs are rejected. Uses reflection to invoke the private TryValidate method directly. |
Sequence Diagram
sequenceDiagram
participant Mod
participant Builder as StorableItemDefinitionBuilder
participant Registry as S1Registry (native)
participant Retain as RuntimeItemDefinitionRegistry
participant Lifecycle as GameLifecycle.OnPreLoad
Mod->>Builder: .Build()
Builder->>Registry: AddToRegistry(definition)
Builder->>Retain: Retain(id, definition)
Retain->>Lifecycle: subscribe ReapplyAll (once)
Note over Registry: Scene transition → native registry clears runtime items
Lifecycle-->>Retain: OnPreLoad fires
loop each retained entry
Retain->>Registry: IsRegistered(id)?
alt not present
Retain->>Registry: AddToRegistry(definition)
end
end
Note over Mod: Mod calls UnregisterItem(id)
Mod->>Registry: RemoveFromRegistry(definition)
Registry-->>Mod: "removed = true"
Mod->>Retain: Forget(id)
Comments Outside Diff (1)
-
S1API/Items/ItemManager.cs, line 133-141 (link)RegisterItemandEnsureItemRegisteredsilently skip retentionBoth public registration APIs call
S1Registry.Instance.AddToRegistry(...)directly without callingRuntimeItemDefinitionRegistry.Retain(), so items registered through either path are not reapplied after a scene transition. The XML docs forRegisterItemdescribe it as appropriate for "advanced scenarios" but give no hint that it opts out of cross-load persistence. A mod developer who reaches forRegisterItemto register a dynamically-created item — exactly the case this PR targets — will encounter the same disappearing-item bug after a menu→reload cycle.If the omission is intentional (e.g. these APIs are only expected to be called from within
OnPreLoaditself), a brief note in the doc comment would prevent the confusion.Prompt To Fix With AI
This is a comment left during a code review. Path: S1API/Items/ItemManager.cs Line: 133-141 Comment: **`RegisterItem` and `EnsureItemRegistered` silently skip retention** Both public registration APIs call `S1Registry.Instance.AddToRegistry(...)` directly without calling `RuntimeItemDefinitionRegistry.Retain()`, so items registered through either path are not reapplied after a scene transition. The XML docs for `RegisterItem` describe it as appropriate for "advanced scenarios" but give no hint that it opts out of cross-load persistence. A mod developer who reaches for `RegisterItem` to register a dynamically-created item — exactly the case this PR targets — will encounter the same disappearing-item bug after a menu→reload cycle. If the omission is intentional (e.g. these APIs are only expected to be called from within `OnPreLoad` itself), a brief note in the doc comment would prevent the confusion. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
S1API/Internal/Items/RuntimeItemDefinitionRegistry.cs:57-59
**`_adapter` read outside the lock in `ReapplyAll`**
`_adapter` is assigned inside `lock(Gate)` in `ResetForTesting` (line 134) but is read at line 110 in `ReapplyAll` with no lock held. Without a `volatile` annotation or a read barrier, the JIT is free to hoist the field load out of the loop, so a concurrent test that swaps the adapter mid-iteration could observe the old value indefinitely. In production this is a non-issue since the field never changes after initialization, but making the field `volatile` would make the invariant explicit and keep the testing seam safe under any scheduler.
### Issue 2 of 2
S1API/Items/ItemManager.cs:133-141
**`RegisterItem` and `EnsureItemRegistered` silently skip retention**
Both public registration APIs call `S1Registry.Instance.AddToRegistry(...)` directly without calling `RuntimeItemDefinitionRegistry.Retain()`, so items registered through either path are not reapplied after a scene transition. The XML docs for `RegisterItem` describe it as appropriate for "advanced scenarios" but give no hint that it opts out of cross-load persistence. A mod developer who reaches for `RegisterItem` to register a dynamically-created item — exactly the case this PR targets — will encounter the same disappearing-item bug after a menu→reload cycle.
If the omission is intentional (e.g. these APIs are only expected to be called from within `OnPreLoad` itself), a brief note in the doc comment would prevent the confusion.
Reviews (1): Last reviewed commit: "chore(release): bump version to 3.1.0-be..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static IRuntimeItemDefinitionRegistryAdapter _adapter = | ||
| RuntimeItemDefinitionRegistryAdapter.Instance; | ||
| private static bool _hooked; |
There was a problem hiding this comment.
_adapter read outside the lock in ReapplyAll
_adapter is assigned inside lock(Gate) in ResetForTesting (line 134) but is read at line 110 in ReapplyAll with no lock held. Without a volatile annotation or a read barrier, the JIT is free to hoist the field load out of the loop, so a concurrent test that swaps the adapter mid-iteration could observe the old value indefinitely. In production this is a non-issue since the field never changes after initialization, but making the field volatile would make the invariant explicit and keep the testing seam safe under any scheduler.
Prompt To Fix With AI
This is a comment left during a code review.
Path: S1API/Internal/Items/RuntimeItemDefinitionRegistry.cs
Line: 57-59
Comment:
**`_adapter` read outside the lock in `ReapplyAll`**
`_adapter` is assigned inside `lock(Gate)` in `ResetForTesting` (line 134) but is read at line 110 in `ReapplyAll` with no lock held. Without a `volatile` annotation or a read barrier, the JIT is free to hoist the field load out of the loop, so a concurrent test that swaps the adapter mid-iteration could observe the old value indefinitely. In production this is a non-issue since the field never changes after initialization, but making the field `volatile` would make the invariant explicit and keep the testing seam safe under any scheduler.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
ItemManager.UnregisterItembehavior by forgetting retained entries only after native removal succeeds.3.1.0-beta.7.Root cause
The native registry removes runtime definitions when returning to the menu. Mods that cache their registrations then skip rebuilding them on the next load, so save hydration cannot resolve placed buildables or stored custom items.
Validation
dotnet test S1API.Tests/S1API.Tests.csproj -c MonoMelon --no-restore --no-build— 307 passed.GameAssembly-absent finalizer crash from uninitialized wrapper test objects.