Add temperature customization and quality calculation methods to custom Station Recipes#81
Open
k073l wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChemistry recipes now support configurable quality calculation modes (Additive or Absolute) and temperature constraints. A new ChangesChemistry Recipe Quality and Temperature Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
S1API/Internal/Patches/ChemistryStationPatches.cs (2)
290-296: 💤 Low valueAdd explicit null check for clearer error messaging.
While the pattern match on line 292 handles null
itemDefinitioncorrectly, an explicit null check would provide a more specific error message to help developers diagnose configuration issues.💡 Proposed refinement for clarity
var product = currentAddedRecipe.Product.ItemId; var itemDefinition = ItemManager.GetItemDefinition(product); +if (itemDefinition == null) +{ + Logger.Warning($"[S1API] Absolute quality calculation method specified for recipe '{currentAddedRecipe.RecipeID}' but product '{product}' could not be resolved. Falling back to default calculation."); + return true; +} if (itemDefinition is not QualityItemDefinition qualityItemDefinition) { Logger.Warning($"[S1API] Absolute quality calculation method specified for recipe '{currentAddedRecipe.RecipeID}' but product '{product}' is not a quality item. Falling back to default calculation."); return true; }🤖 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 `@S1API/Internal/Patches/ChemistryStationPatches.cs` around lines 290 - 296, The code currently pattern-matches itemDefinition into QualityItemDefinition but doesn't give a separate message when ItemManager.GetItemDefinition(product) returns null; add an explicit null check for itemDefinition right after calling ItemManager.GetItemDefinition(product) and log a clear warning referencing currentAddedRecipe.RecipeID and product when null, then keep the existing type-check and its Logger.Warning for the case where the definition exists but is not a QualityItemDefinition; ensure the method still returns true in both early-exit cases.
284-285: ⚖️ Poor tradeoffConsider caching recipe lookups for performance.
FirstOrDefaultperforms a linear search through all registered recipes on every quality calculation. If quality is calculated frequently (e.g., during cooking or when hovering over recipes in the UI), this could become a performance bottleneck.Consider adding a dictionary in
ChemistryStationRecipesto enable O(1) lookups by RecipeID:// In ChemistryStationRecipes class: private static readonly Dictionary<string, ChemistryStationRecipe> _recipeByIdCache = new Dictionary<string, ChemistryStationRecipe>(StringComparer.OrdinalIgnoreCase); public static ChemistryStationRecipe? GetByRecipeId(string recipeId) { return _recipeByIdCache.TryGetValue(recipeId ?? "", out var recipe) ? recipe : null; }Then use in the patch:
var currentAddedRecipe = ChemistryStationRecipes.GetByRecipeId(instanceRecipeId);🤖 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 `@S1API/Internal/Patches/ChemistryStationPatches.cs` around lines 284 - 285, The lookup using ChemistryStationRecipes.GetAll().FirstOrDefault(...) in ChemistryStationPatches (the variable currentAddedRecipe) causes repeated O(n) searches; add an O(1) cache in the ChemistryStationRecipes class: introduce a Dictionary<string, ChemistryStationRecipe> (case-insensitive) and a public accessor GetByRecipeId(string) that returns the recipe or null, ensure the cache is populated/updated whenever recipes are registered/loaded (e.g., in the same code that builds the list returned by GetAll or in the recipe registration method), and replace the FirstOrDefault call in ChemistryStationPatches with a call to ChemistryStationRecipes.GetByRecipeId(__instance.RecipeID) (or the local instanceRecipeId variable) to use the cache.S1API/Stations/QualityCalculationMethod.cs (1)
16-18: ⚡ Quick winClarify the remark to reference the product definition type.
The remark states "Requires the result to be of type QualityItemInstance" but the implementation in
ChemistryStationPatches.cs(line 292) checks whether the product is aQualityItemDefinition, notQualityItemInstance. The remark should describe the product definition requirement rather than the result instance type.📝 Proposed documentation fix
/// <remarks> -/// Requires the result to be of type QualityItemInstance or its inheritor. +/// Requires the recipe product to be of type QualityItemDefinition. /// </remarks>🤖 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 `@S1API/Stations/QualityCalculationMethod.cs` around lines 16 - 18, The XML remark in QualityCalculationMethod.cs incorrectly states "Requires the result to be of type QualityItemInstance" while the runtime check in ChemistryStationPatches.cs uses the product definition type QualityItemDefinition; update the remark on QualityCalculationMethod (the class/method documented in QualityCalculationMethod.cs) to state that the product must be a QualityItemDefinition (or its inheritor) rather than a QualityItemInstance, so the documentation matches the implementation and references the product definition type used by ChemistryStationPatches.cs.S1API/Stations/ChemistryStationRecipeBuilder.cs (1)
149-150: 💤 Low valueConsider whether zero tolerance should be allowed.
The validation permits
tolerance == 0, which would create a single acceptable temperature point (CookTemperature ± 0). Depending on the cooking minigame mechanics, this could make the recipe impossible or extremely difficult to execute if players cannot hit the exact temperature.If zero tolerance is a valid design choice (e.g., for "expert-level" recipes), this is fine. Otherwise, consider requiring
tolerance > 0:🎮 Proposed validation adjustment
-if (tolerance < 0) - throw new ArgumentOutOfRangeException(nameof(tolerance), "Cook temperature tolerance cannot be negative."); +if (tolerance <= 0) + throw new ArgumentOutOfRangeException(nameof(tolerance), "Cook temperature tolerance must be > 0.");🤖 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 `@S1API/Stations/ChemistryStationRecipeBuilder.cs` around lines 149 - 150, The current validation allows tolerance == 0 which may be invalid; update the check in ChemistryStationRecipeBuilder where the parameter/variable tolerance is validated to require a positive value (change the condition from tolerance < 0 to tolerance <= 0) and throw an ArgumentOutOfRangeException with a clearer message such as "Cook temperature tolerance must be positive." so callers cannot create a zero-tolerance recipe unless explicitly intended.
🤖 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 `@S1API/Internal/Patches/ChemistryStationPatches.cs`:
- Around line 284-287: Accessing __instance.RecipeID can throw like other
locations; wrap the read in a try-catch and bail out if it throws. Specifically,
retrieve RecipeID inside a try block, then call
ChemistryStationRecipes.GetAll().FirstOrDefault(r => r.RecipeID == id) using
that captured id; in the catch block return true (preserve original behavior) so
the Harmony patch won't fail. Update the code around currentAddedRecipe
accordingly (use a local id variable and try-catch) and mirror the exception
handling pattern used at the other RecipeID accesses in this file.
In `@S1API/Items/ItemCreator.cs`:
- Around line 119-128: The requiresLevelToPurchase parameter is never used so
gating is always determined only by requiredRank; update CreateItem to apply the
builder's rank gating conditionally: call
StorableItemDefinitionBuilder.WithRequiredRank(requiredRank) only when
requiresLevelToPurchase is true (and otherwise omit calling it), so passing
requiresLevelToPurchase:false disables gating even if requiredRank is provided;
alternatively remove the requiresLevelToPurchase parameter from the public API
if you prefer the opposite behavior, but prefer the conditional call to
WithRequiredRank in the CreateItem implementation to preserve the new flag.
In `@S1API/Items/QualityItemDefinitionBuilder.cs`:
- Around line 306-333: CopyPropertiesFrom currently copies all fields except the
item identifier so cloned definitions end up with a blank ID; update
CopyPropertiesFrom to copy source.ID into _definition.ID (or the appropriate
identifier property) and add a validation in Build (the method called after
QualityItemCreator.CloneFrom(...).Build()) to reject or throw when
_definition.ID is null/empty; additionally expose or ensure WithBasicInfo/WithID
(or add a narrow WithID method) is available on the builder so callers can set a
new ID for clones before registration. Ensure references to CopyPropertiesFrom,
QualityItemCreator.CloneFrom, Build, WithBasicInfo/WithID and the _definition.ID
field are used when making these changes.
- Around line 62-69: The builder currently always allocates a hidden GameObject
placeholder (_storedItemPlaceholder) with a StoredItem component and calls
DontDestroyOnLoad, leaking one hidden Unity object per
QualityItemDefinitionBuilder; change this by creating the placeholder lazily
only when _definition.StoredItem is requested and null (defer creation to the
accessor/ensure method) or, if WithStoredItem(...) or Build() assigns a real
stored item, immediately destroy and null out _storedItemPlaceholder (call
Object.Destroy and clear references) so no placeholder persists; update
QualityItemDefinitionBuilder's WithStoredItem, Build, and any
getter/ensureStoredItem helper to either lazy-create the placeholder or to
teardown _storedItemPlaceholder when replaced.
In `@S1API/Items/StorableItemDefinitionBuilder.cs`:
- Around line 294-323: CopyPropertiesFrom currently omits the source ID so
clones can end up with a blank ID; update CopyPropertiesFrom to copy source.ID
into _definition.ID, add a public SetID(string id) method on
StorableItemDefinitionBuilder so callers (e.g., ItemCreator.CloneFrom(...)) can
explicitly set a new ID, and add validation in
StorableItemDefinitionBuilder.Build() to guard against empty IDs (throw an
ArgumentException or similar) so registration cannot proceed with a blank ID.
- Around line 61-70: StorableItemDefinitionBuilder is creating a hidden
_storedItemPlaceholder GameObject in the constructor and calling
Object.DontDestroyOnLoad, which leaks one Unity object per builder; change this
to either create the placeholder lazily (only when _definition.StoredItem is
read and null) or ensure you destroy the placeholder as soon as a real stored
item is assigned or after Build() completes. Specifically, update the
StorableItemDefinitionBuilder constructor/fields so _storedItemPlaceholder is
null initially, add a helper (e.g., EnsureStoredItemPlaceholder()) that creates
the GameObject and S1Storage.StoredItem only when needed, and modify any setter
or Build() method that sets _definition.StoredItem to destroy
(Object.DestroyImmediate or Destroy) and null out _storedItemPlaceholder if a
real stored item is provided so the temporary GameObject is not kept with
DontDestroyOnLoad. Ensure references to _storedItemPlaceholder and
_definition.StoredItem are updated consistently (constructor, Build(), and any
SetStoredItem methods) to avoid leaks.
---
Nitpick comments:
In `@S1API/Internal/Patches/ChemistryStationPatches.cs`:
- Around line 290-296: The code currently pattern-matches itemDefinition into
QualityItemDefinition but doesn't give a separate message when
ItemManager.GetItemDefinition(product) returns null; add an explicit null check
for itemDefinition right after calling ItemManager.GetItemDefinition(product)
and log a clear warning referencing currentAddedRecipe.RecipeID and product when
null, then keep the existing type-check and its Logger.Warning for the case
where the definition exists but is not a QualityItemDefinition; ensure the
method still returns true in both early-exit cases.
- Around line 284-285: The lookup using
ChemistryStationRecipes.GetAll().FirstOrDefault(...) in ChemistryStationPatches
(the variable currentAddedRecipe) causes repeated O(n) searches; add an O(1)
cache in the ChemistryStationRecipes class: introduce a Dictionary<string,
ChemistryStationRecipe> (case-insensitive) and a public accessor
GetByRecipeId(string) that returns the recipe or null, ensure the cache is
populated/updated whenever recipes are registered/loaded (e.g., in the same code
that builds the list returned by GetAll or in the recipe registration method),
and replace the FirstOrDefault call in ChemistryStationPatches with a call to
ChemistryStationRecipes.GetByRecipeId(__instance.RecipeID) (or the local
instanceRecipeId variable) to use the cache.
In `@S1API/Stations/ChemistryStationRecipeBuilder.cs`:
- Around line 149-150: The current validation allows tolerance == 0 which may be
invalid; update the check in ChemistryStationRecipeBuilder where the
parameter/variable tolerance is validated to require a positive value (change
the condition from tolerance < 0 to tolerance <= 0) and throw an
ArgumentOutOfRangeException with a clearer message such as "Cook temperature
tolerance must be positive." so callers cannot create a zero-tolerance recipe
unless explicitly intended.
In `@S1API/Stations/QualityCalculationMethod.cs`:
- Around line 16-18: The XML remark in QualityCalculationMethod.cs incorrectly
states "Requires the result to be of type QualityItemInstance" while the runtime
check in ChemistryStationPatches.cs uses the product definition type
QualityItemDefinition; update the remark on QualityCalculationMethod (the
class/method documented in QualityCalculationMethod.cs) to state that the
product must be a QualityItemDefinition (or its inheritor) rather than a
QualityItemInstance, so the documentation matches the implementation and
references the product definition type used by ChemistryStationPatches.cs.
🪄 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: 2b18d081-ef37-4839-95fd-a4a5eb29acd6
📒 Files selected for processing (12)
S1API/Internal/Patches/ChemistryStationPatches.csS1API/Items/ItemCreator.csS1API/Items/ItemManager.csS1API/Items/QualityItemCreator.csS1API/Items/QualityItemDefinition.csS1API/Items/QualityItemDefinitionBuilder.csS1API/Items/QualityItemInstance.csS1API/Items/StorableItemDefinition.csS1API/Items/StorableItemDefinitionBuilder.csS1API/Stations/ChemistryStationRecipe.csS1API/Stations/ChemistryStationRecipeBuilder.csS1API/Stations/QualityCalculationMethod.cs
Comment on lines
+119
to
+128
| bool requiresLevelToPurchase = false, | ||
| FullRank? requiredRank = null, | ||
| Sprite icon = null, | ||
| Equippable equippable = null) | ||
| { | ||
| var builder = new StorableItemDefinitionBuilder() | ||
| .WithBasicInfo(id, name, description, category) | ||
| .WithStackLimit(stackLimit) | ||
| .WithPricing(basePurchasePrice, resellMultiplier) | ||
| .WithRequiredRank(requiredRank) |
There was a problem hiding this comment.
requiresLevelToPurchase is a no-op right now.
The new flag is never read. CreateItem(requiresLevelToPurchase: false, requiredRank: someRank) still enables gating, while CreateItem(requiresLevelToPurchase: true, requiredRank: null) leaves gating off. Please make the builder call depend on the flag, or remove the flag from the public API.
Suggested fix
- .WithPricing(basePurchasePrice, resellMultiplier)
- .WithRequiredRank(requiredRank)
+ .WithPricing(basePurchasePrice, resellMultiplier)
+ .WithRequiredRank(
+ requiresLevelToPurchase
+ ? requiredRank ?? throw new ArgumentException(
+ "requiredRank must be provided when requiresLevelToPurchase is true.",
+ nameof(requiredRank))
+ : null)📝 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.
Suggested change
| bool requiresLevelToPurchase = false, | |
| FullRank? requiredRank = null, | |
| Sprite icon = null, | |
| Equippable equippable = null) | |
| { | |
| var builder = new StorableItemDefinitionBuilder() | |
| .WithBasicInfo(id, name, description, category) | |
| .WithStackLimit(stackLimit) | |
| .WithPricing(basePurchasePrice, resellMultiplier) | |
| .WithRequiredRank(requiredRank) | |
| bool requiresLevelToPurchase = false, | |
| FullRank? requiredRank = null, | |
| Sprite icon = null, | |
| Equippable equippable = null) | |
| { | |
| var builder = new StorableItemDefinitionBuilder() | |
| .WithBasicInfo(id, name, description, category) | |
| .WithStackLimit(stackLimit) | |
| .WithPricing(basePurchasePrice, resellMultiplier) | |
| .WithRequiredRank( | |
| requiresLevelToPurchase | |
| ? requiredRank ?? throw new ArgumentException( | |
| "requiredRank must be provided when requiresLevelToPurchase is true.", | |
| nameof(requiredRank)) | |
| : null) |
🤖 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 `@S1API/Items/ItemCreator.cs` around lines 119 - 128, The
requiresLevelToPurchase parameter is never used so gating is always determined
only by requiredRank; update CreateItem to apply the builder's rank gating
conditionally: call StorableItemDefinitionBuilder.WithRequiredRank(requiredRank)
only when requiresLevelToPurchase is true (and otherwise omit calling it), so
passing requiresLevelToPurchase:false disables gating even if requiredRank is
provided; alternatively remove the requiresLevelToPurchase parameter from the
public API if you prefer the opposite behavior, but prefer the conditional call
to WithRequiredRank in the CreateItem implementation to preserve the new flag.
Comment on lines
+62
to
+69
| // Provide a minimal StoredItem placeholder so the field is never null in tooling/inspectors. | ||
| _storedItemPlaceholder = new GameObject("S1API_DefaultStoredItem"); | ||
| _storedItemPlaceholder.SetActive(false); | ||
| _storedItemPlaceholder.hideFlags = HideFlags.HideAndDontSave; | ||
| Object.DontDestroyOnLoad(_storedItemPlaceholder); | ||
| var storedItemComponent = _storedItemPlaceholder.AddComponent<S1Storage.StoredItem>(); | ||
| _definition.StoredItem = storedItemComponent; | ||
| } |
There was a problem hiding this comment.
Avoid leaking a hidden StoredItem placeholder per builder.
Both constructors allocate a GameObject, add StoredItem, and mark it DontDestroyOnLoad, but nothing tears that placeholder down when WithStoredItem() replaces it or after Build() finishes. Every QualityItemDefinitionBuilder instance therefore leaves one hidden Unity object alive for the rest of the session. Create the placeholder lazily only when you actually need the fallback, or destroy it as soon as a real stored item is assigned.
Also applies to: 80-87
🤖 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 `@S1API/Items/QualityItemDefinitionBuilder.cs` around lines 62 - 69, The
builder currently always allocates a hidden GameObject placeholder
(_storedItemPlaceholder) with a StoredItem component and calls
DontDestroyOnLoad, leaking one hidden Unity object per
QualityItemDefinitionBuilder; change this by creating the placeholder lazily
only when _definition.StoredItem is requested and null (defer creation to the
accessor/ensure method) or, if WithStoredItem(...) or Build() assigns a real
stored item, immediately destroy and null out _storedItemPlaceholder (call
Object.Destroy and clear references) so no placeholder persists; update
QualityItemDefinitionBuilder's WithStoredItem, Build, and any
getter/ensureStoredItem helper to either lazy-create the placeholder or to
teardown _storedItemPlaceholder when replaced.
Comment on lines
+306
to
+333
| private void CopyPropertiesFrom(S1ItemFramework.QualityItemDefinition source) | ||
| { | ||
| if (source == null) return; | ||
|
|
||
| // Basic ItemDefinition properties | ||
| _definition.Name = source.Name; | ||
| _definition.Description = source.Description; | ||
| _definition.Category = source.Category; | ||
| _definition.StackLimit = source.StackLimit; | ||
| _definition.AvailableInDemo = source.AvailableInDemo; | ||
| _definition.UsableInFilters = source.UsableInFilters; | ||
| _definition.Icon = source.Icon; | ||
| _definition.legalStatus = source.legalStatus; | ||
| _definition.PickpocketDifficultyMultiplier = source.PickpocketDifficultyMultiplier; | ||
| _definition.CombatUtility = source.CombatUtility; | ||
|
|
||
| // StorableItemDefinition properties | ||
| _definition.BasePurchasePrice = source.BasePurchasePrice; | ||
| _definition.ResellMultiplier = source.ResellMultiplier; | ||
| _definition.ShopCategories = source.ShopCategories; | ||
| _definition.RequiresLevelToPurchase = source.RequiresLevelToPurchase; | ||
| _definition.RequiredRank = source.RequiredRank; | ||
| _definition.StoredItem = source.StoredItem != null ? source.StoredItem : _definition.StoredItem; | ||
| _definition.StationItem = source.StationItem; | ||
| _definition.Equippable = source.Equippable; | ||
| _definition.DefaultQuality = source.DefaultQuality; | ||
| _definition.CustomItemUI = source.CustomItemUI; | ||
| } |
There was a problem hiding this comment.
Clone builders currently lose the source item ID.
CopyPropertiesFrom carries over the display and gameplay fields but never assigns source.ID. That means QualityItemCreator.CloneFrom(...).Build() can register a definition with an empty ID unless the caller reruns WithBasicInfo, which breaks the advertised preconfigured clone flow. At minimum, Build() should reject blank IDs; ideally the clone path should also offer a narrow way to set just the new ID before registration.
🤖 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 `@S1API/Items/QualityItemDefinitionBuilder.cs` around lines 306 - 333,
CopyPropertiesFrom currently copies all fields except the item identifier so
cloned definitions end up with a blank ID; update CopyPropertiesFrom to copy
source.ID into _definition.ID (or the appropriate identifier property) and add a
validation in Build (the method called after
QualityItemCreator.CloneFrom(...).Build()) to reject or throw when
_definition.ID is null/empty; additionally expose or ensure WithBasicInfo/WithID
(or add a narrow WithID method) is available on the builder so callers can set a
new ID for clones before registration. Ensure references to CopyPropertiesFrom,
QualityItemCreator.CloneFrom, Build, WithBasicInfo/WithID and the _definition.ID
field are used when making these changes.
Comment on lines
+61
to
+70
| _definition.RequiresLevelToPurchase = false; | ||
| _definition.RequiredRank = new S1Levelling.FullRank(S1Levelling.ERank.Street_Rat, 1); | ||
|
|
||
| // Provide a minimal StoredItem placeholder so the field is never null in tooling/inspectors. | ||
| _storedItemPlaceholder = new GameObject("S1API_DefaultStoredItem"); | ||
| _storedItemPlaceholder.SetActive(false); | ||
| _storedItemPlaceholder.hideFlags = HideFlags.HideAndDontSave; | ||
| Object.DontDestroyOnLoad(_storedItemPlaceholder); | ||
| var storedItemComponent = _storedItemPlaceholder.AddComponent<S1Storage.StoredItem>(); | ||
| _definition.StoredItem = storedItemComponent; |
There was a problem hiding this comment.
Avoid leaking a hidden StoredItem placeholder per builder.
These constructors allocate a hidden StoredItem placeholder and mark it DontDestroyOnLoad, but the builder never destroys it when a real stored item is assigned or after Build() completes. Every StorableItemDefinitionBuilder instance therefore leaks one Unity object for the remainder of the session. Create the placeholder lazily, or tear it down once it is no longer the active StoredItem.
Also applies to: 82-88
🤖 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 `@S1API/Items/StorableItemDefinitionBuilder.cs` around lines 61 - 70,
StorableItemDefinitionBuilder is creating a hidden _storedItemPlaceholder
GameObject in the constructor and calling Object.DontDestroyOnLoad, which leaks
one Unity object per builder; change this to either create the placeholder
lazily (only when _definition.StoredItem is read and null) or ensure you destroy
the placeholder as soon as a real stored item is assigned or after Build()
completes. Specifically, update the StorableItemDefinitionBuilder
constructor/fields so _storedItemPlaceholder is null initially, add a helper
(e.g., EnsureStoredItemPlaceholder()) that creates the GameObject and
S1Storage.StoredItem only when needed, and modify any setter or Build() method
that sets _definition.StoredItem to destroy (Object.DestroyImmediate or Destroy)
and null out _storedItemPlaceholder if a real stored item is provided so the
temporary GameObject is not kept with DontDestroyOnLoad. Ensure references to
_storedItemPlaceholder and _definition.StoredItem are updated consistently
(constructor, Build(), and any SetStoredItem methods) to avoid leaks.
Comment on lines
+294
to
+323
| /// <summary> | ||
| /// Copies all properties from a source StorableItemDefinition to the current definition. | ||
| /// </summary> | ||
| /// <param name="source">The source definition to copy properties from.</param> | ||
| private void CopyPropertiesFrom(S1ItemFramework.StorableItemDefinition source) | ||
| { | ||
| if (source == null) return; | ||
|
|
||
| // Basic ItemDefinition properties | ||
| _definition.Name = source.Name; | ||
| _definition.Description = source.Description; | ||
| _definition.Category = source.Category; | ||
| _definition.StackLimit = source.StackLimit; | ||
| _definition.AvailableInDemo = source.AvailableInDemo; | ||
| _definition.UsableInFilters = source.UsableInFilters; | ||
| _definition.Icon = source.Icon; | ||
| _definition.legalStatus = source.legalStatus; | ||
| _definition.PickpocketDifficultyMultiplier = source.PickpocketDifficultyMultiplier; | ||
| _definition.CombatUtility = source.CombatUtility; | ||
|
|
||
| // StorableItemDefinition properties | ||
| _definition.BasePurchasePrice = source.BasePurchasePrice; | ||
| _definition.ResellMultiplier = source.ResellMultiplier; | ||
| _definition.ShopCategories = source.ShopCategories; | ||
| _definition.RequiresLevelToPurchase = source.RequiresLevelToPurchase; | ||
| _definition.RequiredRank = source.RequiredRank; | ||
| _definition.StoredItem = source.StoredItem != null ? source.StoredItem : _definition.StoredItem; | ||
| _definition.StationItem = source.StationItem; | ||
| _definition.Equippable = source.Equippable; | ||
| } |
There was a problem hiding this comment.
Clone builders currently lose the source item ID.
CopyPropertiesFrom copies the source metadata and economics, but not source.ID. That leaves ItemCreator.CloneFrom(...).Build() able to register a definition without an ID unless callers reconstruct the basic info manually. Please guard Build() against blank IDs and give the clone flow an explicit way to set the new ID before registration.
🤖 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 `@S1API/Items/StorableItemDefinitionBuilder.cs` around lines 294 - 323,
CopyPropertiesFrom currently omits the source ID so clones can end up with a
blank ID; update CopyPropertiesFrom to copy source.ID into _definition.ID, add a
public SetID(string id) method on StorableItemDefinitionBuilder so callers
(e.g., ItemCreator.CloneFrom(...)) can explicitly set a new ID, and add
validation in StorableItemDefinitionBuilder.Build() to guard against empty IDs
(throw an ArgumentException or similar) so registration cannot proceed with a
blank ID.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds builder options to customize temperature and its tolerance when building a station recipe.
Adds a custom quality calculation method for determining the quality of the resulting recipe product -
Absolute, allowing for invariant quality, determined only by the definition, rather than quality of ingredients.Release Notes
ChemistryStationRecipeTemperatureclass to encapsulate cook temperature and tolerance with computed inclusive boundsTemperatureproperty toChemistryStationRecipeto expose recipe temperature requirementsQualityCalculationMethodenum withAdditive(default) andAbsolutemodes for determining product qualityQualityCalculationMethodproperty toChemistryStationRecipeto specify which quality calculation mode is usedChemistryStationRecipeBuilderwithWithTemperature(float cookTemperature, float tolerance)method for customizing recipe cook temperatureChemistryStationRecipeBuilderwithWithCalculationMethod(QualityCalculationMethod method)method to set quality calculation behaviorStationRecipe.CalculateQualityto supportAbsolutequality calculation mode, which makes product quality invariant and determined solely by recipe definitionChemistryStationPatches.csto properly select between Il2Cpp and Mono versions of framework typesLines Changed by Author