Add Clothing Wrappers and Registry Helpers#77
Conversation
- Add NativeDefinition, NativeClothingDefinition, NativeClothingInstance public properties - Add CreateInstance() to ClothingItemDefinition for instance creation - Add IsItemRegistered(), EnsureItemRegistered() to ItemManager (idempotent registry check) - Add NativePlayer property to Player - Add CurrentBasicAvatarSettings wrapper property to Player - Add InsertClothing() and SendAppearance() proxy methods to Player
📝 WalkthroughWalkthroughAdds new public APIs to the Player entity for avatar and clothing management, including methods to retrieve settings, equip clothing with color customization, and refresh appearance. Extends ClothingItemDefinition with instance creation overloads, updates ItemManager with registration utility methods, and refactors documentation examples to use a two-phase initialization lifecycle pattern. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
added
the
game update
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
S1API/docs/clothing-items.md (1)
145-202:⚠️ Potential issue | 🟡 MinorExample may re-register the clothing item on every save load.
GameLifecycle.OnPreLoadfires on every save load (perS1API/Lifecycle/GameLifecycle.cs), butInitializeCustomClothingis subscribed once with no idempotency check. Each subsequent load will re-runAccessoryFactory.CreateAndRegisterAccessory(...)andClothingItemCreator.CloneFrom("cap").Build(), which callsS1Registry.Instance.AddToRegistry(_definition)unconditionally — likely producing duplicate registry entries forcustom_cap.Since this PR introduces
ItemManager.IsItemRegistered/EnsureItemRegisteredfor exactly this scenario, the canonical example should leverage them so mods following this guide don't accidentally double-register.📝 Suggested guard
private void InitializeCustomClothing() { + if (ItemManager.IsItemRegistered("custom_cap")) + { + customCap = ItemManager.GetItemDefinition("custom_cap") as ClothingItemDefinition; + return; + } + // Step 1: Create and register custom accessorySimilarly,
AddCustomClothingToShopswill run on every load and re-add the item to compatible shops; consider noting whetherShopManager.AddToCompatibleShopsis itself idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/docs/clothing-items.md` around lines 145 - 202, InitializeCustomClothing currently re-registers the accessory and clothing on every GameLifecycle.OnPreLoad invocation, causing duplicate entries; before calling AccessoryFactory.CreateAndRegisterAccessory and ClothingItemCreator.CloneFrom("cap").Build (which triggers S1Registry.Instance.AddToRegistry), use ItemManager.IsItemRegistered("custom_cap") or call ItemManager.EnsureItemRegistered to make the registration idempotent, and only perform the accessory registration/Build when the item is not already registered; likewise, guard AddCustomClothingToShops by checking if the item is already added to shops or rely on ShopManager.AddToCompatibleShops being idempotent to avoid repeated additions.
🧹 Nitpick comments (5)
S1API/docs/clothing-items.md (1)
7-7: Tighten the wording around "after theMainscene starts".The phrase "Hook setup after the
Mainscene starts, then register clothing duringGameLifecycle.OnPreLoad" is slightly ambiguous —OnPreLoadfires before the save data loads, which can occur on any subsequent save load, not just first entry into Main. Consider clarifying that theMain-scene hook is for one-time event subscription and that the actual registration runs every timeOnPreLoadfires (hence the need for an idempotency check viaIsItemRegistered/EnsureItemRegistered).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/docs/clothing-items.md` at line 7, Clarify the wording to distinguish the one-time hook and the repeated registration: state that you should set up the hook once when the Main scene starts (subscribe to GameLifecycle.OnPreLoad), and that the clothing registration logic runs every time GameLifecycle.OnPreLoad fires (so it must be idempotent); explicitly mention using IsItemRegistered or EnsureItemRegistered in the documentation to show the required per-onPreLoad idempotency check.S1API/Items/ClothingItemDefinition.cs (1)
47-51: Consider also overriding the baseCreateInstance(int quantity = 1).
ItemDefinition.CreateInstance(int quantity = 1)isvirtualand still returns a plainItemInstance. Callers that hold aClothingItemDefinitionas the base type, or that follow the base API and pass a quantity, will silently bypass this clothing-specific factory and miss theClothingItemInstancewrapper / color initialization. Consider overriding the virtual to delegate to the new color-aware factory (e.g., usingDefaultColor) so the two creation paths stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinition.cs` around lines 47 - 51, The base virtual CreateInstance(int quantity = 1) on ClothingItemDefinition should be overridden so callers using the base signature still get a ClothingItemInstance with color initialized; add an override of CreateInstance(int quantity = 1) in ClothingItemDefinition that delegates to the existing CreateInstance(ClothingColor color) (or the color-aware factory) using the class's DefaultColor and the provided quantity so both creation paths return a ClothingItemInstance initialized consistently.S1API/Entities/Player.cs (1)
283-289: Allocates a new wrapper on each access.Each call to
CurrentBasicAvatarSettingsconstructs a freshBasicAvatarSettingswrapper. This is inexpensive but meansplayer.CurrentBasicAvatarSettings == player.CurrentBasicAvatarSettingswould be false unlessBasicAvatarSettingsdefines value-style equality, which could surprise consumers comparing references. Consider noting in the XML doc that a new wrapper is returned per access (or caching it lazily if cheap to do safely).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Entities/Player.cs` around lines 283 - 289, The property Player.CurrentBasicAvatarSettings currently allocates a new BasicAvatarSettings wrapper on every access (wrapping S1Player.CurrentAvatarSettings), which can surprise consumers comparing references; either update the XML doc for CurrentBasicAvatarSettings to explicitly state "returns a new BasicAvatarSettings instance on each access" or implement a simple lazy cache on the Player instance (store a private BasicAvatarSettings? field and refresh it when S1Player.CurrentAvatarSettings changes) so repeated accesses return the same reference; refer to the CurrentBasicAvatarSettings property, the BasicAvatarSettings type, and S1Player.CurrentAvatarSettings when applying the change.S1API/Items/ItemManager.cs (1)
96-130: LGTM with a small consistency nit.The null/whitespace +
S1Registry.Instancenull guards, the early-return-on-already-registered fast path, and the post-add re-verification are all sound and make the helpers safely idempotent.Minor consistency nit: the file mixes the
S1.Registrynamespace alias and theS1Registrytype alias for the same static class (e.g.,S1.Registry.ItemExistson line 103 vs.S1Registry.Instance.AddToRegistryon line 128). Picking one alias for this type would slightly improve readability, but it's not a functional concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ItemManager.cs` around lines 96 - 130, The code mixes two names for the same registry (S1.Registry and S1Registry); make them consistent by choosing one alias and using it everywhere—e.g., change the call in IsItemRegistered from S1.Registry.ItemExists(itemID) to S1Registry.ItemExists(itemID) (or conversely update uses of S1Registry.Instance to S1.Registry.Instance) so that IsItemRegistered, EnsureItemRegistered, and the AddToRegistry invocation all reference the same identifier consistently.S1API/Items/ClothingItemDefinitionBuilder.cs (1)
244-269: Silent best-effort: consider logging when no UI is borrowed.If
S1Registry.Instanceis null,GetAllItems()returns null, or no existing clothing has aCustomItemUI, this method silently leaves_definition.CustomItemUIas null andBuild()proceeds. That can lead to a clothing item registering with a missing UI and the failure mode showing up only at runtime in the inventory UI. Consider emitting a debug/warning log on the no-match path so mod authors can diagnose ordering issues (e.g., callingBuild()before any base-game clothing is registered).Also worth noting: assigning the borrowed
CustomItemUIreference shares the same prefab/object across all custom clothing items built this way. Please confirm the native clothing UI template is intended to be shared (versus cloned per-definition) — if the game ever mutates per-item state onCustomItemUI, sharing the reference could cause cross-item bleed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinitionBuilder.cs` around lines 244 - 269, EnsureNativeClothingItemUi currently silently leaves _definition.CustomItemUI null when S1Registry.Instance is null, GetAllItems() returns null, or no matching clothing is found; update EnsureNativeClothingItemUi to emit a debug/warning via your logger when it cannot borrow a UI (include context that Build() was called before any native clothing registered), and when you successfully borrow a CustomItemUI from another S1Clothing.ClothingDefinition, either clone/instantiate the prefab/object before assigning to _definition.CustomItemUI (to avoid shared mutable state) or clearly document/guard the shared-reference behavior; key symbols: EnsureNativeClothingItemUi, S1Registry.Instance, GetAllItems(), S1Clothing.ClothingDefinition.CustomItemUI, and _definition.CustomItemUI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@S1API/docs/clothing-items.md`:
- Around line 145-202: InitializeCustomClothing currently re-registers the
accessory and clothing on every GameLifecycle.OnPreLoad invocation, causing
duplicate entries; before calling AccessoryFactory.CreateAndRegisterAccessory
and ClothingItemCreator.CloneFrom("cap").Build (which triggers
S1Registry.Instance.AddToRegistry), use
ItemManager.IsItemRegistered("custom_cap") or call
ItemManager.EnsureItemRegistered to make the registration idempotent, and only
perform the accessory registration/Build when the item is not already
registered; likewise, guard AddCustomClothingToShops by checking if the item is
already added to shops or rely on ShopManager.AddToCompatibleShops being
idempotent to avoid repeated additions.
---
Nitpick comments:
In `@S1API/docs/clothing-items.md`:
- Line 7: Clarify the wording to distinguish the one-time hook and the repeated
registration: state that you should set up the hook once when the Main scene
starts (subscribe to GameLifecycle.OnPreLoad), and that the clothing
registration logic runs every time GameLifecycle.OnPreLoad fires (so it must be
idempotent); explicitly mention using IsItemRegistered or EnsureItemRegistered
in the documentation to show the required per-onPreLoad idempotency check.
In `@S1API/Entities/Player.cs`:
- Around line 283-289: The property Player.CurrentBasicAvatarSettings currently
allocates a new BasicAvatarSettings wrapper on every access (wrapping
S1Player.CurrentAvatarSettings), which can surprise consumers comparing
references; either update the XML doc for CurrentBasicAvatarSettings to
explicitly state "returns a new BasicAvatarSettings instance on each access" or
implement a simple lazy cache on the Player instance (store a private
BasicAvatarSettings? field and refresh it when S1Player.CurrentAvatarSettings
changes) so repeated accesses return the same reference; refer to the
CurrentBasicAvatarSettings property, the BasicAvatarSettings type, and
S1Player.CurrentAvatarSettings when applying the change.
In `@S1API/Items/ClothingItemDefinition.cs`:
- Around line 47-51: The base virtual CreateInstance(int quantity = 1) on
ClothingItemDefinition should be overridden so callers using the base signature
still get a ClothingItemInstance with color initialized; add an override of
CreateInstance(int quantity = 1) in ClothingItemDefinition that delegates to the
existing CreateInstance(ClothingColor color) (or the color-aware factory) using
the class's DefaultColor and the provided quantity so both creation paths return
a ClothingItemInstance initialized consistently.
In `@S1API/Items/ClothingItemDefinitionBuilder.cs`:
- Around line 244-269: EnsureNativeClothingItemUi currently silently leaves
_definition.CustomItemUI null when S1Registry.Instance is null, GetAllItems()
returns null, or no matching clothing is found; update
EnsureNativeClothingItemUi to emit a debug/warning via your logger when it
cannot borrow a UI (include context that Build() was called before any native
clothing registered), and when you successfully borrow a CustomItemUI from
another S1Clothing.ClothingDefinition, either clone/instantiate the
prefab/object before assigning to _definition.CustomItemUI (to avoid shared
mutable state) or clearly document/guard the shared-reference behavior; key
symbols: EnsureNativeClothingItemUi, S1Registry.Instance, GetAllItems(),
S1Clothing.ClothingDefinition.CustomItemUI, and _definition.CustomItemUI.
In `@S1API/Items/ItemManager.cs`:
- Around line 96-130: The code mixes two names for the same registry
(S1.Registry and S1Registry); make them consistent by choosing one alias and
using it everywhere—e.g., change the call in IsItemRegistered from
S1.Registry.ItemExists(itemID) to S1Registry.ItemExists(itemID) (or conversely
update uses of S1Registry.Instance to S1.Registry.Instance) so that
IsItemRegistered, EnsureItemRegistered, and the AddToRegistry invocation all
reference the same identifier consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caa14fea-c005-4ed4-a504-57f61b0f3b72
📒 Files selected for processing (8)
S1API/Entities/Player.csS1API/Items/ClothingItemCreator.csS1API/Items/ClothingItemDefinition.csS1API/Items/ClothingItemDefinitionBuilder.csS1API/Items/ClothingItemInstance.csS1API/Items/ItemDefinition.csS1API/Items/ItemManager.csS1API/docs/clothing-items.md
sethwalker1
changed the title
…convention - Replace the one-shot native clothing UI warning with keyed per-reason dedupe. - Keep the richer registry/template failure diagnostics while avoiding repeated log spam. - Match the existing `GrowContainerAdditives` warning pattern without changing its behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
S1API/Items/ClothingItemDefinitionBuilder.cs (2)
29-29: Minor:OrdinalIgnoreCaseon internally-authored warning keys is unnecessary.The reasons fed into
WarnedMissingNativeClothingItemUiReasonsare hard-coded English strings produced by this class itself, so case-insensitive comparison adds no value and can mask near-duplicate keys that happen to differ only in casing. PlainOrdinal(the default) is sufficient.♻️ Suggested change
- private static readonly HashSet<string> WarnedMissingNativeClothingItemUiReasons = new HashSet<string>(System.StringComparer.OrdinalIgnoreCase); + private static readonly HashSet<string> WarnedMissingNativeClothingItemUiReasons = new HashSet<string>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinitionBuilder.cs` at line 29, The HashSet WarnedMissingNativeClothingItemUiReasons is initialized with StringComparer.OrdinalIgnoreCase unnecessarily; change its initialization to use the default ordinal comparer (either remove the StringComparer parameter and use new HashSet<string>() or explicitly use System.StringComparer.Ordinal) so keys remain case-sensitive and near-duplicate keys differing only by case are not masked; update the static field declaration for WarnedMissingNativeClothingItemUiReasons accordingly.
41-41: Address PR open question: defaultStackLimitfor clothing should be1.The PR description flags this as open. Clothing items are inherently non-stackable in S1 (each instance carries its own
Color/per-instance state), so a default of10is a footgun: mods that don't think to setStackLimit = 1will produce stackable hats/shirts that lose per-instance color/equip state when stacked. Defaulting to1matches the domain semantics and pushes any opt-in for stacking onto the (rare) caller that actually wants it. The compatibility argument is weak here because today all clothing definitions in the game ship withStackLimit = 1anyway, so flipping the builder default aligns with reality rather than diverging from it.♻️ Suggested change
- _definition.StackLimit = 10; + // Clothing instances carry per-instance state (color, equipped flag). Stacking + // would discard that state, so default to non-stackable. Callers can opt into + // larger stacks explicitly if they really want it. + _definition.StackLimit = 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinitionBuilder.cs` at line 41, The builder currently sets _definition.StackLimit = 10 which makes clothing incorrectly stackable; change the default in ClothingItemDefinitionBuilder to _definition.StackLimit = 1 so clothing items are non-stackable by default (update the constructor or Build/Init method that assigns _definition.StackLimit), ensuring existing callers that rely on per-instance state (Color/equip) keep correct semantics and only opt into stacking by explicitly overriding StackLimit.S1API/docs/clothing-items.md (1)
165-190: Doc example: missing null-check oncustomTexturebefore use.
TextureUtils.LoadTextureFromResourcecan returnnullif the embedded resource name is mistyped (a common copy-paste mistake for readers following the doc). The example then puts thatnullinto the texture-replacement dictionary and hands it toAccessoryFactory.CreateAndRegisterAccessory, which is at best a confusing failure mode. A short guard makes the example safer to copy.📘 Suggested doc change
var customTexture = TextureUtils.LoadTextureFromResource( assembly, "MyMod.Resources.CustomCap.custom_cap_texture.png"); + + if (customTexture == null) + { + MelonLogger.Error("Failed to load custom cap texture from embedded resources."); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/docs/clothing-items.md` around lines 165 - 190, Add a null-check after calling TextureUtils.LoadTextureFromResource to ensure customTexture is not null before it is inserted into textureReplacements and passed to AccessoryFactory.CreateAndRegisterAccessory; if TextureUtils.LoadTextureFromResource returns null, call MelonLogger.Error with a clear message including the resource name and return early to avoid passing null into textureReplacements and subsequent registration code (keep references to RuntimeResourceRegistry.IsRegistered, TextureUtils.LoadTextureFromResource, textureReplacements, and AccessoryFactory.CreateAndRegisterAccessory to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@S1API/docs/clothing-items.md`:
- Around line 157-161: The example uses a silent "as" cast that can produce a
null customCap and an early return that hides the problem: update the example
around ItemManager.IsItemRegistered/ItemManager.GetItemDefinition and the
CustomItemId flow to defensively check the type result (i.e., check if the cast
to ClothingItemDefinition returned null), and if null log a clear error or
warning via your logger (or throw) explaining that the registered ID is not a
ClothingItemDefinition before returning; ensure AddCustomClothingToShops only
proceeds when customCap != null and show the log/check in the example so readers
copy the safer pattern.
---
Nitpick comments:
In `@S1API/docs/clothing-items.md`:
- Around line 165-190: Add a null-check after calling
TextureUtils.LoadTextureFromResource to ensure customTexture is not null before
it is inserted into textureReplacements and passed to
AccessoryFactory.CreateAndRegisterAccessory; if
TextureUtils.LoadTextureFromResource returns null, call MelonLogger.Error with a
clear message including the resource name and return early to avoid passing null
into textureReplacements and subsequent registration code (keep references to
RuntimeResourceRegistry.IsRegistered, TextureUtils.LoadTextureFromResource,
textureReplacements, and AccessoryFactory.CreateAndRegisterAccessory to locate
the change).
In `@S1API/Items/ClothingItemDefinitionBuilder.cs`:
- Line 29: The HashSet WarnedMissingNativeClothingItemUiReasons is initialized
with StringComparer.OrdinalIgnoreCase unnecessarily; change its initialization
to use the default ordinal comparer (either remove the StringComparer parameter
and use new HashSet<string>() or explicitly use System.StringComparer.Ordinal)
so keys remain case-sensitive and near-duplicate keys differing only by case are
not masked; update the static field declaration for
WarnedMissingNativeClothingItemUiReasons accordingly.
- Line 41: The builder currently sets _definition.StackLimit = 10 which makes
clothing incorrectly stackable; change the default in
ClothingItemDefinitionBuilder to _definition.StackLimit = 1 so clothing items
are non-stackable by default (update the constructor or Build/Init method that
assigns _definition.StackLimit), ensuring existing callers that rely on
per-instance state (Color/equip) keep correct semantics and only opt into
stacking by explicitly overriding StackLimit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02696603-fa33-4dca-b009-34f02c1e796b
📒 Files selected for processing (6)
S1API/Entities/Player.csS1API/Items/ClothingItemDefinition.csS1API/Items/ClothingItemDefinitionBuilder.csS1API/Items/ClothingItemInstance.csS1API/Items/ItemDefinition.csS1API/docs/clothing-items.md
🚧 Files skipped from review as they are similar to previous changes (3)
- S1API/Items/ItemDefinition.cs
- S1API/Items/ClothingItemDefinition.cs
- S1API/Entities/Player.cs
ifBars
left a comment
ifBars
left a comment
There was a problem hiding this comment.
There is useful clothing DX in this PR, but I’m not comfortable merging it as-is.
The main thing holding me back is that it mixes some solid wrapper improvements with new public Native* escape hatches that don’t feel aligned with S1API’s abstraction and cross-compatibility goals. Player.EquipClothing also currently ignores ClothingItemDefinition.DefaultColor.
I also think the stack-limit question should be resolved a bit more clearly. To me, 10 reads more like a generic storable-item default that leaked into the clothing builder rather than something clothing-specific. Since clothing instances carry per-instance state and feed into avatar/equip behavior, non-stackable feels like the safer default, and mods that really want a higher stack size can still opt into that explicitly.
I think this would benefit from another pass before merge:
- remove or rethink the public
Native*properties - fix
EquipClothingso the no-argument path respects the definition’s default color - keep the PR focused on wrapper improvements that fit S1API’s public API direction
- either resolve the clothing stack-limit direction here or pull it out of this PR’s scope
There is definitely mergeable value here, but I think the public API additions need a bit more tightening so they line up with the design goals of S1API.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
S1API/Items/ClothingItemDefinitionBuilder.cs (1)
83-103:⚠️ Potential issue | 🟠 MajorHandle null
source.SlotsToBlockin the Mono clone path.Line 101 can throw
ArgumentNullExceptionwhensource.SlotsToBlockis null, while the IL2CPP branch already handles null safely. Please align both paths to avoid runtime crashes during cloning.Proposed fix
internal ClothingItemDefinitionBuilder(S1Clothing.ClothingDefinition source) { + if (source == null) + throw new System.ArgumentNullException(nameof(source)); + _definition = ScriptableObject.CreateInstance<S1Clothing.ClothingDefinition>(); @@ `#else` - _definition.SlotsToBlock = new List<S1Clothing.EClothingSlot>(source.SlotsToBlock); + _definition.SlotsToBlock = source.SlotsToBlock != null + ? new List<S1Clothing.EClothingSlot>(source.SlotsToBlock) + : new List<S1Clothing.EClothingSlot>(); `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinitionBuilder.cs` around lines 83 - 103, The Mono-path can throw ArgumentNullException when assigning _definition.SlotsToBlock = new List<S1Clothing.EClothingSlot>(source.SlotsToBlock) if source.SlotsToBlock is null; make it mirror the IL2CPP branch by checking for null before copying: if source.SlotsToBlock is null set _definition.SlotsToBlock to an empty List<S1Clothing.EClothingSlot> (or new List<...>()), otherwise create the new List from source.SlotsToBlock; update the code around _definition.SlotsToBlock and source.SlotsToBlock in ClothingItemDefinitionBuilder.cs to perform this null-safe assignment.
🧹 Nitpick comments (2)
S1API/Items/ClothingItemDefinitionBuilder.cs (2)
249-286: Cache the borrowed nativeCustomItemUItemplate after first resolution.
EnsureNativeClothingItemUi()scans the full registry every timeCustomItemUIis missing. For bulk mod registration, a static cache of the first valid template would avoid repeated O(N) scans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinitionBuilder.cs` around lines 249 - 286, EnsureNativeClothingItemUi currently scans S1Registry.Instance.GetAllItems() every time _definition.CustomItemUI is null; add a static cached template field (e.g., a private static Il2CppObject/CustomItemUIType s_cachedNativeCustomItemUI) and check it first in EnsureNativeClothingItemUi, assign _definition.CustomItemUI from the static cache if present, and when you find a valid S1Clothing.ClothingDefinition.CustomItemUI during the scan set both _definition.CustomItemUI and the static cache so subsequent calls skip the O(N) scan.
28-29: Add synchronization to protect the static warning dedup set from concurrent access.The static
WarnedMissingNativeClothingItemUiReasonsHashSet is mutated at line 296 without synchronization. While the current codebase shows no threading patterns around clothing builder invocations, this represents a potential race condition ifBuildInternal()is ever called concurrently from multiple threads. Wrap theAddcall in a lock to ensure thread safety.Proposed fix
private static readonly HashSet<string> WarnedMissingNativeClothingItemUiReasons = new HashSet<string>(); +private static readonly object WarnedMissingNativeClothingItemUiLock = new object(); @@ - if (WarnedMissingNativeClothingItemUiReasons.Add(reason)) - { - Logger.Warning($"Could not borrow a native clothing CustomItemUI template ({reason}). Custom clothing inventory UI may be incomplete. This usually means Build() was called before any native clothing registered."); - } + lock (WarnedMissingNativeClothingItemUiLock) + { + if (WarnedMissingNativeClothingItemUiReasons.Add(reason)) + { + Logger.Warning($"Could not borrow a native clothing CustomItemUI template ({reason}). Custom clothing inventory UI may be incomplete. This usually means Build() was called before any native clothing registered."); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Items/ClothingItemDefinitionBuilder.cs` around lines 28 - 29, The static HashSet WarnedMissingNativeClothingItemUiReasons is mutated without synchronization in BuildInternal(); introduce a private static readonly object (e.g., WarnedMissingNativeClothingItemUiReasonsLock) and wrap the HashSet.Add(...) call inside a lock(WarnedMissingNativeClothingItemUiReasonsLock) block to prevent concurrent access; update references in ClothingItemDefinitionBuilder to use this lock when adding to the set to ensure thread safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@S1API/Items/ClothingItemDefinitionBuilder.cs`:
- Around line 83-103: The Mono-path can throw ArgumentNullException when
assigning _definition.SlotsToBlock = new
List<S1Clothing.EClothingSlot>(source.SlotsToBlock) if source.SlotsToBlock is
null; make it mirror the IL2CPP branch by checking for null before copying: if
source.SlotsToBlock is null set _definition.SlotsToBlock to an empty
List<S1Clothing.EClothingSlot> (or new List<...>()), otherwise create the new
List from source.SlotsToBlock; update the code around _definition.SlotsToBlock
and source.SlotsToBlock in ClothingItemDefinitionBuilder.cs to perform this
null-safe assignment.
---
Nitpick comments:
In `@S1API/Items/ClothingItemDefinitionBuilder.cs`:
- Around line 249-286: EnsureNativeClothingItemUi currently scans
S1Registry.Instance.GetAllItems() every time _definition.CustomItemUI is null;
add a static cached template field (e.g., a private static
Il2CppObject/CustomItemUIType s_cachedNativeCustomItemUI) and check it first in
EnsureNativeClothingItemUi, assign _definition.CustomItemUI from the static
cache if present, and when you find a valid
S1Clothing.ClothingDefinition.CustomItemUI during the scan set both
_definition.CustomItemUI and the static cache so subsequent calls skip the O(N)
scan.
- Around line 28-29: The static HashSet WarnedMissingNativeClothingItemUiReasons
is mutated without synchronization in BuildInternal(); introduce a private
static readonly object (e.g., WarnedMissingNativeClothingItemUiReasonsLock) and
wrap the HashSet.Add(...) call inside a
lock(WarnedMissingNativeClothingItemUiReasonsLock) block to prevent concurrent
access; update references in ClothingItemDefinitionBuilder to use this lock when
adding to the set to ensure thread safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c11e32e3-32cb-4205-879d-303e99c51233
📒 Files selected for processing (4)
S1API/Entities/Player.csS1API/Items/ClothingItemDefinition.csS1API/Items/ClothingItemDefinitionBuilder.csS1API/docs/clothing-items.md
🚧 Files skipped from review as they are similar to previous changes (3)
- S1API/Entities/Player.cs
- S1API/Items/ClothingItemDefinition.cs
- S1API/docs/clothing-items.md
|
I was also feeling non-stackable clothing would be a better default, but wasn't sure how to proceed due to the default already being 10. I've now adjusted it to 1. |
ifBars
left a comment
ifBars
left a comment
There was a problem hiding this comment.
This looks much cleaner now. Thanks for taking another pass on it.
2 participants
Adds native clothing and item wrappers plus helper APIs for custom clothing workflows, so mods can register definitions, verify registry state, equip clothing, refresh appearance, and respond to revives without dropping down into game internals. Also updates the clothing docs to recommend the GameLifecycle.OnPreLoad / OnLoadComplete flow and ensures newly built clothing definitions inherit a native clothing item UI when one is available.
Open Question: Clothing Stack Default
I considered changing ClothingItemDefinitionBuilder to default clothing stacks to 1, but left the current default of 10 intact in this PR.
The reason is compatibility: clothing currently inherits the same default stack behavior used by the other builders and item helpers, and changing that default here would be a behavioral change rather than just an API/documentation improvement. That feels risky for existing mods that may already rely on the current builder behavior.
For mod-specific cases where a clothing item should clearly be non-stackable, setting StackLimit = 1 explicitly on the built definition still works today.
Please advise on how you would prefer to proceed from here:
Release Notes
GetCurrentBasicAvatarSettings(),InsertClothing(),EquipClothing(),RefreshClothingAppearance(),SendAppearance(), andOnReviveevent for handling player revival eventsItemManager:IsItemRegistered()for safe registry lookups andEnsureItemRegistered()for conditional registrationStackLimitof 10 to 1, making clothing non-stackable by defaultGameLifecycle.OnPreLoadfor definition registration andGameLifecycle.OnLoadCompletefor shop integration, with idempotent implementation checksContributions by Author