Feature: Add Support for List Patching system03.bin "shop" Data#1200
Merged
Conversation
WalkthroughAdds a KH2 Shop binary serializer/deserializer, integrates a "shop" listpatch case into the patcher, adds tests for shop patching, and updates mods documentation with a shop listpatch example and TOC entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Modder
participant YAML as "ShopList.yml"
participant Patcher as "PatcherProcessor"
participant BAR as "03system.bin (entry stream)"
participant ShopIO as "OpenKh.Kh2.SystemData.Shop"
Modder->>Patcher: run listpatch ("shop")
Patcher->>BAR: open entry stream
Patcher->>ShopIO: Read(stream)
ShopIO-->>Patcher: Shop (header, Shop/Inventory/Product lists)
Patcher->>YAML: deserialize ShopHelper
Note over Patcher: compute inventoriesBaseOffset & productsBaseOffset
Patcher->>Patcher: map helpers -> entries, merge/pad lists\nrecompute offsets & valid products
Patcher->>ShopIO: Write(stream, Shop)
ShopIO-->>BAR: updated entry written
Patcher-->>Modder: patch complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (7)
OpenKh.Patcher/PatcherProcessor.cs (1)
998-1021: In-place edits only: document or enforce behavior.This path assumes fixed counts (no insertion/removal). If additions should be supported later, base offsets must be recomputed using new counts before writing, and lists resized accordingly. For now, either document “update-only” semantics in docs or add validations that reject attempts to expand beyond current counts.
docs/tool/GUI.ModsManager/creatingMods.md (3)
22-22: Fix list indentation and hard tab in TOC.List indentation is inconsistent and contains a hard tab. Align with surrounding items.
Apply this diff:
- * [shop](#shop-source-example) + * [shop](#shop-source-example)
314-315: Align “shop” bullet with the rest of the list.One extra leading space breaks markdownlint’s indentation rules.
Apply this diff:
- * `shop` +* `shop`
422-454: Add language to fenced code block and clarify binarc entry requirements for shop.
- Specify yaml for syntax highlighting.
- Add a short note so authors know they must target the “shop” subfile in 03system.bin with Bar entry type Unknown41.
Apply this diff:
-### `shop` Source Example -``` +### `shop` Source Example + +Note: In 03system.bin, the shop subfile is named "shop" and uses Bar entry type "Unknown41". In your asset, target 03system.bin with method: binarc and an entry: + - name: shop + - type: Unknown41 + - method: listpatch + +```yaml ShopEntryHelpers: - CommandArgument: 104 UnlockMenuFlag: 42 @@ - ProductIndex: 1 ItemID: 296</blockquote></details> <details> <summary>OpenKh.Kh2/SystemData/Shop.cs (3)</summary><blockquote> `64-65`: **Avoid magic numbers for record sizes; use the declared constants.** Hard-coded 8 and 2 make maintenance error-prone. Use InventoryEntrySize and ProductEntrySize for clarity and consistency. Apply this diff: ```diff - InventoryStartIndex = (uint)((InventoryOffset - InventoriesBaseOffset) / 8), + InventoryStartIndex = (uint)((InventoryOffset - InventoriesBaseOffset) / InventoryEntrySize), @@ - ProductStartIndex = (uint)((ProductOffset - ProductsBaseOffset) / 2) + ProductStartIndex = (uint)((ProductOffset - ProductsBaseOffset) / ProductEntrySize) @@ - InventoryOffset = (ushort)(InventoriesBaseOffset + InventoryStartIndex * 8), + InventoryOffset = (ushort)(InventoriesBaseOffset + InventoryStartIndex * InventoryEntrySize), @@ - ProductOffset = (ushort)(ProductsBaseOffset + ProductStartIndex * 2), + ProductOffset = (ushort)(ProductsBaseOffset + ProductStartIndex * ProductEntrySize),Also applies to: 83-84, 155-156, 171-172
195-206: Validate header MagicCode and FileType when reading.Add simple guards to fail fast on wrong streams and prevent bogus counts from cascading.
Apply this diff:
public static Shop Read(Stream stream) { if (!stream.CanRead || !stream.CanSeek) throw new InvalidDataException($"Read or seek must be supported."); ShopHeader header = ShopHeader.Read(stream); + if (header.MagicCode != MagicCode) + throw new InvalidDataException($"Invalid shop magic 0x{header.MagicCode:X8} (expected 0x{MagicCode:X8})."); + if (header.FileType != FileType) + throw new InvalidDataException($"Invalid shop file type 0x{header.FileType:X4} (expected 0x{FileType:X4})."); var shop = new Shop() { ShopEntries = ShopEntry.Read(stream, header.ShopEntryCount), InventoryEntries = InventoryEntry.Read(stream, header.InventoryEntryCount), ProductEntries = ProductEntry.Read(stream, header.ProductEntryCount), ValidProductEntries = ProductEntry.Read(stream, (int)((stream.Length - header.ValidProductsOffset) / 2)) }; return shop; }
187-194: Public fields vs properties in ShopHelper.Using public fields works with YamlDotNet, but properties are the convention elsewhere in SystemData types. Consider switching to auto-properties for consistency and future extensibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
OpenKh.Kh2/SystemData/Shop.cs(1 hunks)OpenKh.Patcher/PatcherProcessor.cs(1 hunks)OpenKh.Tests/Patcher/PatcherTests.cs(1 hunks)docs/tool/GUI.ModsManager/creatingMods.md(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
OpenKh.Patcher/PatcherProcessor.cs (1)
OpenKh.Kh2/SystemData/Shop.cs (8)
Shop(11-236)Shop(195-209)ShopHelper(187-193)Write(30-30)Write(68-68)Write(87-87)Write(101-101)Write(211-235)
OpenKh.Tests/Patcher/PatcherTests.cs (3)
OpenKh.Patcher/PatcherProcessor.cs (3)
PatcherProcessor(17-1522)Patch(67-72)Patch(87-340)OpenKh.Kh2/SystemData/Shop.cs (25)
List(67-67)List(86-86)List(100-100)Shop(11-236)Shop(195-209)ShopEntry(33-69)ShopEntry(142-157)InventoryEntry(71-88)InventoryEntry(167-173)ProductEntry(90-102)ProductEntry(181-184)Write(30-30)Write(68-68)Write(87-87)Write(101-101)Write(211-235)ShopHelper(187-193)ShopEntryHelper(50-65)ShopEntryHelper(109-158)ShopEntryHelper(125-125)ShopEntryHelper(126-141)InventoryEntryHelper(78-84)InventoryEntryHelper(160-174)ProductEntryHelper(94-98)ProductEntryHelper(176-185)OpenKh.Patcher/Metadata.cs (1)
AssetFile(94-193)
OpenKh.Kh2/SystemData/Shop.cs (1)
OpenKh.Kh2/BaseTable.cs (1)
BaseList(36-52)
🪛 LanguageTool
🪛 markdownlint-cli2 (0.17.2)
docs/tool/GUI.ModsManager/creatingMods.md
22-22: Inconsistent indentation for list items at the same level
Expected: 6; Actual: 3
(MD005, list-indent)
22-22: Unordered list indentation
Expected: 6; Actual: 3
(MD007, ul-indent)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
314-314: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: build
🔇 Additional comments (1)
OpenKh.Tests/Patcher/PatcherTests.cs (1)
999-1016: Use a consistent Bar.EntryType in tests and docs.The test correctly uses Type = "Unknown41" for the binarc entry containing "shop". Please ensure docs mention this (added in doc review), and keep this constant usage in future tests to avoid ambiguity.
Comment on lines
+998
to
+1021
| case "shop": | ||
| var shop = Kh2.SystemData.Shop.Read(stream); | ||
| var moddedShop = deserializer.Deserialize<Kh2.SystemData.Shop.ShopHelper>(sourceText); | ||
| ushort inventoriesBaseOffset = (ushort)(Kh2.SystemData.Shop.HeaderSize + shop.ShopEntries.Count * Kh2.SystemData.Shop.ShopEntrySize); | ||
| ushort productsBaseOffset = (ushort)(inventoriesBaseOffset + shop.InventoryEntries.Count * Kh2.SystemData.Shop.InventoryEntrySize); | ||
| foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers) | ||
| { | ||
| var entryIndex = shop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID); | ||
| shop.ShopEntries[entryIndex] = shopEntryHelper.ToShopEntry(inventoriesBaseOffset); | ||
| } | ||
| foreach (var inventoryEntryHelper in moddedShop.InventoryEntryHelpers) | ||
| { | ||
| shop.InventoryEntries[inventoryEntryHelper.InventoryIndex] = inventoryEntryHelper.ToInventoryEntry(productsBaseOffset); | ||
| } | ||
| foreach (var productEntryHelper in moddedShop.ProductEntryHelpers) | ||
| { | ||
| shop.ProductEntries[productEntryHelper.ProductIndex] = productEntryHelper.ToProductEntry(); | ||
| } | ||
| foreach (var productEntryHelper in moddedShop.ValidProductEntryHelpers) | ||
| { | ||
| shop.ValidProductEntries[productEntryHelper.ProductIndex] = productEntryHelper.ToProductEntry(); | ||
| } | ||
| Kh2.SystemData.Shop.Write(stream.SetPosition(0), shop); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the “shop” listpatch path against missing/null lists and out-of-range indices.
Current code assumes all helper lists are present and indices exist in the base data. This can crash with IndexOutOfRangeException or -1 index when a mod provides an unknown ShopID or indexes past current counts. Add null-guards and explicit range checks (throw a clear exception or skip with a warning) to make failures actionable.
Apply this diff:
case "shop":
- var shop = Kh2.SystemData.Shop.Read(stream);
- var moddedShop = deserializer.Deserialize<Kh2.SystemData.Shop.ShopHelper>(sourceText);
+ var shop = Kh2.SystemData.Shop.Read(stream);
+ var moddedShop = deserializer.Deserialize<Kh2.SystemData.Shop.ShopHelper>(sourceText);
ushort inventoriesBaseOffset = (ushort)(Kh2.SystemData.Shop.HeaderSize + shop.ShopEntries.Count * Kh2.SystemData.Shop.ShopEntrySize);
ushort productsBaseOffset = (ushort)(inventoriesBaseOffset + shop.InventoryEntries.Count * Kh2.SystemData.Shop.InventoryEntrySize);
- foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers)
- {
- var entryIndex = shop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID);
- shop.ShopEntries[entryIndex] = shopEntryHelper.ToShopEntry(inventoriesBaseOffset);
- }
- foreach (var inventoryEntryHelper in moddedShop.InventoryEntryHelpers)
- {
- shop.InventoryEntries[inventoryEntryHelper.InventoryIndex] = inventoryEntryHelper.ToInventoryEntry(productsBaseOffset);
- }
- foreach (var productEntryHelper in moddedShop.ProductEntryHelpers)
- {
- shop.ProductEntries[productEntryHelper.ProductIndex] = productEntryHelper.ToProductEntry();
- }
- foreach (var productEntryHelper in moddedShop.ValidProductEntryHelpers)
- {
- shop.ValidProductEntries[productEntryHelper.ProductIndex] = productEntryHelper.ToProductEntry();
- }
+ if (moddedShop?.ShopEntryHelpers != null)
+ {
+ foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers)
+ {
+ var entryIndex = shop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID);
+ if (entryIndex < 0)
+ throw new InvalidDataException($"Shop listpatch: ShopID {shopEntryHelper.ShopID} not found in existing ShopEntries.");
+ shop.ShopEntries[entryIndex] = shopEntryHelper.ToShopEntry(inventoriesBaseOffset);
+ }
+ }
+ if (moddedShop?.InventoryEntryHelpers != null)
+ {
+ foreach (var inventoryEntryHelper in moddedShop.InventoryEntryHelpers)
+ {
+ var idx = inventoryEntryHelper.InventoryIndex;
+ if (idx < 0 || idx >= shop.InventoryEntries.Count)
+ throw new IndexOutOfRangeException($"Shop listpatch: InventoryIndex {idx} out of range [0..{shop.InventoryEntries.Count - 1}].");
+ shop.InventoryEntries[idx] = inventoryEntryHelper.ToInventoryEntry(productsBaseOffset);
+ }
+ }
+ if (moddedShop?.ProductEntryHelpers != null)
+ {
+ foreach (var productEntryHelper in moddedShop.ProductEntryHelpers)
+ {
+ var idx = productEntryHelper.ProductIndex;
+ if (idx < 0 || idx >= shop.ProductEntries.Count)
+ throw new IndexOutOfRangeException($"Shop listpatch: ProductIndex {idx} out of range [0..{shop.ProductEntries.Count - 1}].");
+ shop.ProductEntries[idx] = productEntryHelper.ToProductEntry();
+ }
+ }
+ if (moddedShop?.ValidProductEntryHelpers != null)
+ {
+ foreach (var productEntryHelper in moddedShop.ValidProductEntryHelpers)
+ {
+ var idx = productEntryHelper.ProductIndex;
+ if (idx < 0 || idx >= shop.ValidProductEntries.Count)
+ throw new IndexOutOfRangeException($"Shop listpatch: ValidProductIndex {idx} out of range [0..{shop.ValidProductEntries.Count - 1}].");
+ shop.ValidProductEntries[idx] = productEntryHelper.ToProductEntry();
+ }
+ }
Kh2.SystemData.Shop.Write(stream.SetPosition(0), shop);
break;📝 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
| case "shop": | |
| var shop = Kh2.SystemData.Shop.Read(stream); | |
| var moddedShop = deserializer.Deserialize<Kh2.SystemData.Shop.ShopHelper>(sourceText); | |
| ushort inventoriesBaseOffset = (ushort)(Kh2.SystemData.Shop.HeaderSize + shop.ShopEntries.Count * Kh2.SystemData.Shop.ShopEntrySize); | |
| ushort productsBaseOffset = (ushort)(inventoriesBaseOffset + shop.InventoryEntries.Count * Kh2.SystemData.Shop.InventoryEntrySize); | |
| foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers) | |
| { | |
| var entryIndex = shop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID); | |
| shop.ShopEntries[entryIndex] = shopEntryHelper.ToShopEntry(inventoriesBaseOffset); | |
| } | |
| foreach (var inventoryEntryHelper in moddedShop.InventoryEntryHelpers) | |
| { | |
| shop.InventoryEntries[inventoryEntryHelper.InventoryIndex] = inventoryEntryHelper.ToInventoryEntry(productsBaseOffset); | |
| } | |
| foreach (var productEntryHelper in moddedShop.ProductEntryHelpers) | |
| { | |
| shop.ProductEntries[productEntryHelper.ProductIndex] = productEntryHelper.ToProductEntry(); | |
| } | |
| foreach (var productEntryHelper in moddedShop.ValidProductEntryHelpers) | |
| { | |
| shop.ValidProductEntries[productEntryHelper.ProductIndex] = productEntryHelper.ToProductEntry(); | |
| } | |
| Kh2.SystemData.Shop.Write(stream.SetPosition(0), shop); | |
| break; | |
| case "shop": | |
| var shop = Kh2.SystemData.Shop.Read(stream); | |
| var moddedShop = deserializer.Deserialize<Kh2.SystemData.Shop.ShopHelper>(sourceText); | |
| ushort inventoriesBaseOffset = (ushort)(Kh2.SystemData.Shop.HeaderSize + shop.ShopEntries.Count * Kh2.SystemData.Shop.ShopEntrySize); | |
| ushort productsBaseOffset = (ushort)(inventoriesBaseOffset + shop.InventoryEntries.Count * Kh2.SystemData.Shop.InventoryEntrySize); | |
| if (moddedShop?.ShopEntryHelpers != null) | |
| { | |
| foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers) | |
| { | |
| var entryIndex = shop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID); | |
| if (entryIndex < 0) | |
| throw new InvalidDataException($"Shop listpatch: ShopID {shopEntryHelper.ShopID} not found in existing ShopEntries."); | |
| shop.ShopEntries[entryIndex] = shopEntryHelper.ToShopEntry(inventoriesBaseOffset); | |
| } | |
| } | |
| if (moddedShop?.InventoryEntryHelpers != null) | |
| { | |
| foreach (var inventoryEntryHelper in moddedShop.InventoryEntryHelpers) | |
| { | |
| var idx = inventoryEntryHelper.InventoryIndex; | |
| if (idx < 0 || idx >= shop.InventoryEntries.Count) | |
| throw new IndexOutOfRangeException($"Shop listpatch: InventoryIndex {idx} out of range [0..{shop.InventoryEntries.Count - 1}]."); | |
| shop.InventoryEntries[idx] = inventoryEntryHelper.ToInventoryEntry(productsBaseOffset); | |
| } | |
| } | |
| if (moddedShop?.ProductEntryHelpers != null) | |
| { | |
| foreach (var productEntryHelper in moddedShop.ProductEntryHelpers) | |
| { | |
| var idx = productEntryHelper.ProductIndex; | |
| if (idx < 0 || idx >= shop.ProductEntries.Count) | |
| throw new IndexOutOfRangeException($"Shop listpatch: ProductIndex {idx} out of range [0..{shop.ProductEntries.Count - 1}]."); | |
| shop.ProductEntries[idx] = productEntryHelper.ToProductEntry(); | |
| } | |
| } | |
| if (moddedShop?.ValidProductEntryHelpers != null) | |
| { | |
| foreach (var productEntryHelper in moddedShop.ValidProductEntryHelpers) | |
| { | |
| var idx = productEntryHelper.ProductIndex; | |
| if (idx < 0 || idx >= shop.ValidProductEntries.Count) | |
| throw new IndexOutOfRangeException($"Shop listpatch: ValidProductIndex {idx} out of range [0..{shop.ValidProductEntries.Count - 1}]."); | |
| shop.ValidProductEntries[idx] = productEntryHelper.ToProductEntry(); | |
| } | |
| } | |
| Kh2.SystemData.Shop.Write(stream.SetPosition(0), shop); | |
| break; |
🤖 Prompt for AI Agents
In OpenKh.Patcher/PatcherProcessor.cs around lines 998 to 1021, the "shop" case
assumes moddedShop helper lists exist and that indices/ShopIDs map into the base
shop lists; this can produce -1 from FindIndex or IndexOutOfRangeException when
helpers are null or out-of-range. Add null-guards for each helper collection
(skip with a warning or log if null/empty), check FindIndex != -1 before using
entryIndex (throw a clear ArgumentException or log and continue if not found),
and verify that any index (InventoryIndex/ProductIndex) is within the target
list bounds before assignment (again throw or skip with a warning). Keep the
stream write unchanged but perform these validations before mutating shop
entries so failures are actionable and safe.
Comment on lines
+1078
to
+1094
| foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers) | ||
| { | ||
| int shopID = shopEntryHelper.ShopID; | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].CommandArgument, shopEntryHelper.CommandArgument); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].UnlockMenuFlag, shopEntryHelper.UnlockMenuFlag); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].NameID, shopEntryHelper.NameID); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].ShopKeeperEntityID, shopEntryHelper.ShopKeeperEntityID); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].PosX, shopEntryHelper.PosX); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].PosY, shopEntryHelper.PosY); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].PosZ, shopEntryHelper.PosZ); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].ExtraInventoryBitMask, shopEntryHelper.ExtraInventoryBitMask); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].SoundID, shopEntryHelper.SoundID); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].InventoryCount, shopEntryHelper.InventoryCount); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].ShopID, shopEntryHelper.ShopID); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].Unk19, shopEntryHelper.Unk19); | ||
| Assert.Equal(verifyShop.ShopEntries[shopID].InventoryOffset, (int)(inventoryEntriesBaseOffset + shopEntryHelper.InventoryStartIndex * OpenKh.Kh2.SystemData.Shop.InventoryEntrySize)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t assume ShopID equals list index; locate by ShopID when asserting.
The list order may not match ShopID. Resolve the entry index via FindIndex to make the test resilient.
Apply this diff:
- foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers)
- {
- int shopID = shopEntryHelper.ShopID;
- Assert.Equal(verifyShop.ShopEntries[shopID].CommandArgument, shopEntryHelper.CommandArgument);
- Assert.Equal(verifyShop.ShopEntries[shopID].UnlockMenuFlag, shopEntryHelper.UnlockMenuFlag);
- Assert.Equal(verifyShop.ShopEntries[shopID].NameID, shopEntryHelper.NameID);
- Assert.Equal(verifyShop.ShopEntries[shopID].ShopKeeperEntityID, shopEntryHelper.ShopKeeperEntityID);
- Assert.Equal(verifyShop.ShopEntries[shopID].PosX, shopEntryHelper.PosX);
- Assert.Equal(verifyShop.ShopEntries[shopID].PosY, shopEntryHelper.PosY);
- Assert.Equal(verifyShop.ShopEntries[shopID].PosZ, shopEntryHelper.PosZ);
- Assert.Equal(verifyShop.ShopEntries[shopID].ExtraInventoryBitMask, shopEntryHelper.ExtraInventoryBitMask);
- Assert.Equal(verifyShop.ShopEntries[shopID].SoundID, shopEntryHelper.SoundID);
- Assert.Equal(verifyShop.ShopEntries[shopID].InventoryCount, shopEntryHelper.InventoryCount);
- Assert.Equal(verifyShop.ShopEntries[shopID].ShopID, shopEntryHelper.ShopID);
- Assert.Equal(verifyShop.ShopEntries[shopID].Unk19, shopEntryHelper.Unk19);
- Assert.Equal(verifyShop.ShopEntries[shopID].InventoryOffset, (int)(inventoryEntriesBaseOffset + shopEntryHelper.InventoryStartIndex * OpenKh.Kh2.SystemData.Shop.InventoryEntrySize));
- }
+ foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers)
+ {
+ var idx = verifyShop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID);
+ Assert.True(idx >= 0, $"Patched ShopID {shopEntryHelper.ShopID} not found.");
+ Assert.Equal(verifyShop.ShopEntries[idx].CommandArgument, shopEntryHelper.CommandArgument);
+ Assert.Equal(verifyShop.ShopEntries[idx].UnlockMenuFlag, shopEntryHelper.UnlockMenuFlag);
+ Assert.Equal(verifyShop.ShopEntries[idx].NameID, shopEntryHelper.NameID);
+ Assert.Equal(verifyShop.ShopEntries[idx].ShopKeeperEntityID, shopEntryHelper.ShopKeeperEntityID);
+ Assert.Equal(verifyShop.ShopEntries[idx].PosX, shopEntryHelper.PosX);
+ Assert.Equal(verifyShop.ShopEntries[idx].PosY, shopEntryHelper.PosY);
+ Assert.Equal(verifyShop.ShopEntries[idx].PosZ, shopEntryHelper.PosZ);
+ Assert.Equal(verifyShop.ShopEntries[idx].ExtraInventoryBitMask, shopEntryHelper.ExtraInventoryBitMask);
+ Assert.Equal(verifyShop.ShopEntries[idx].SoundID, shopEntryHelper.SoundID);
+ Assert.Equal(verifyShop.ShopEntries[idx].InventoryCount, shopEntryHelper.InventoryCount);
+ Assert.Equal(verifyShop.ShopEntries[idx].ShopID, shopEntryHelper.ShopID);
+ Assert.Equal(verifyShop.ShopEntries[idx].Unk19, shopEntryHelper.Unk19);
+ Assert.Equal(
+ verifyShop.ShopEntries[idx].InventoryOffset,
+ (int)(inventoryEntriesBaseOffset + shopEntryHelper.InventoryStartIndex * OpenKh.Kh2.SystemData.Shop.InventoryEntrySize)
+ );
+ }📝 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
| foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers) | |
| { | |
| int shopID = shopEntryHelper.ShopID; | |
| Assert.Equal(verifyShop.ShopEntries[shopID].CommandArgument, shopEntryHelper.CommandArgument); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].UnlockMenuFlag, shopEntryHelper.UnlockMenuFlag); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].NameID, shopEntryHelper.NameID); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].ShopKeeperEntityID, shopEntryHelper.ShopKeeperEntityID); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].PosX, shopEntryHelper.PosX); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].PosY, shopEntryHelper.PosY); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].PosZ, shopEntryHelper.PosZ); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].ExtraInventoryBitMask, shopEntryHelper.ExtraInventoryBitMask); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].SoundID, shopEntryHelper.SoundID); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].InventoryCount, shopEntryHelper.InventoryCount); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].ShopID, shopEntryHelper.ShopID); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].Unk19, shopEntryHelper.Unk19); | |
| Assert.Equal(verifyShop.ShopEntries[shopID].InventoryOffset, (int)(inventoryEntriesBaseOffset + shopEntryHelper.InventoryStartIndex * OpenKh.Kh2.SystemData.Shop.InventoryEntrySize)); | |
| } | |
| foreach (var shopEntryHelper in moddedShop.ShopEntryHelpers) | |
| { | |
| var idx = verifyShop.ShopEntries.FindIndex(x => x.ShopID == shopEntryHelper.ShopID); | |
| Assert.True(idx >= 0, $"Patched ShopID {shopEntryHelper.ShopID} not found."); | |
| Assert.Equal(verifyShop.ShopEntries[idx].CommandArgument, shopEntryHelper.CommandArgument); | |
| Assert.Equal(verifyShop.ShopEntries[idx].UnlockMenuFlag, shopEntryHelper.UnlockMenuFlag); | |
| Assert.Equal(verifyShop.ShopEntries[idx].NameID, shopEntryHelper.NameID); | |
| Assert.Equal(verifyShop.ShopEntries[idx].ShopKeeperEntityID, shopEntryHelper.ShopKeeperEntityID); | |
| Assert.Equal(verifyShop.ShopEntries[idx].PosX, shopEntryHelper.PosX); | |
| Assert.Equal(verifyShop.ShopEntries[idx].PosY, shopEntryHelper.PosY); | |
| Assert.Equal(verifyShop.ShopEntries[idx].PosZ, shopEntryHelper.PosZ); | |
| Assert.Equal(verifyShop.ShopEntries[idx].ExtraInventoryBitMask, shopEntryHelper.ExtraInventoryBitMask); | |
| Assert.Equal(verifyShop.ShopEntries[idx].SoundID, shopEntryHelper.SoundID); | |
| Assert.Equal(verifyShop.ShopEntries[idx].InventoryCount, shopEntryHelper.InventoryCount); | |
| Assert.Equal(verifyShop.ShopEntries[idx].ShopID, shopEntryHelper.ShopID); | |
| Assert.Equal(verifyShop.ShopEntries[idx].Unk19, shopEntryHelper.Unk19); | |
| Assert.Equal( | |
| verifyShop.ShopEntries[idx].InventoryOffset, | |
| (int)(inventoryEntriesBaseOffset + shopEntryHelper.InventoryStartIndex * OpenKh.Kh2.SystemData.Shop.InventoryEntrySize) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In OpenKh.Tests/Patcher/PatcherTests.cs around lines 1078 to 1094, the test
assumes verifyShop.ShopEntries is indexed by ShopID; instead locate the matching
verifyShop entry by ShopID before asserting. For each shopEntryHelper, call
FindIndex (or equivalent) on verifyShop.ShopEntries to find the index where
entry.ShopID == shopEntryHelper.ShopID, assert the found index is not -1, then
use that index to access verifyShop.ShopEntries for all subsequent Assert.Equal
checks (including the InventoryOffset calculation).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
OpenKh.Kh2/SystemData/Shop.cs (3)
47-62: Verify offset calculation edge cases.The
ToShopEntryHelpermethod performs offset calculations that could potentially cause integer overflow or underflow if theInventoryOffsetis less thanInventoriesBaseOffset.public ShopEntryHelper ToShopEntryHelper(ushort InventoriesBaseOffset) => new ShopEntryHelper() { // ... other properties ... - InventoryStartIndex = (uint)((InventoryOffset - InventoriesBaseOffset) / InventoryEntrySize), + InventoryStartIndex = InventoryOffset >= InventoriesBaseOffset ? + (uint)((InventoryOffset - InventoriesBaseOffset) / InventoryEntrySize) : + throw new InvalidDataException($"InventoryOffset {InventoryOffset} is less than base offset {InventoriesBaseOffset}"), };
75-81: Similar offset calculation issue in InventoryEntryHelper.The same potential issue exists here with offset calculations.
public InventoryEntryHelper ToInventoryEntryHelper(int InventoryIndex, ushort ProductsBaseOffset) => new InventoryEntryHelper { InventoryIndex = InventoryIndex, UnlockEventID = UnlockEventID, ProductCount = ProductCount, - ProductStartIndex = (uint)((ProductOffset - ProductsBaseOffset) / ProductEntrySize) + ProductStartIndex = ProductOffset >= ProductsBaseOffset ? + (uint)((ProductOffset - ProductsBaseOffset) / ProductEntrySize) : + throw new InvalidDataException($"ProductOffset {ProductOffset} is less than base offset {ProductsBaseOffset}"), };
122-138: Consider simplifying constructor with object initializer.The verbose constructor with 13 parameters could be simplified by encouraging the use of object initializer syntax, which is more maintainable.
-public ShopEntryHelper(ushort commandArgument, ushort unlockMenuFlag, ushort nameID, ushort shopKeeperEntityID, short posX, short posY, short posZ, byte extraInventoryBitMask, byte soundID, ushort inventoryCount, byte shopID, byte unk19, uint inventoryStartIndex) -{ - CommandArgument = commandArgument; - UnlockMenuFlag = unlockMenuFlag; - NameID = nameID; - ShopKeeperEntityID = shopKeeperEntityID; - PosX = posX; - PosY = posY; - PosZ = posZ; - ExtraInventoryBitMask = extraInventoryBitMask; - SoundID = soundID; - InventoryCount = inventoryCount; - ShopID = shopID; - Unk19 = unk19; - InventoryStartIndex = inventoryStartIndex; -} +// Consider removing this constructor and using object initializer syntax instead +// This reduces maintenance burden as properties are added/removedOpenKh.Patcher/PatcherProcessor.cs (1)
999-1111: Comprehensive shop patching implementation with good error handling.The implementation handles:
- Safe deserialization with null checks
- Proper merging of shop entries by ShopID
- Dynamic expansion of collections with dummy entries when indices exceed current counts
- Proper offset recalculation after changes
- Index validation to prevent negative indices
This addresses the previous review concerns about null safety and bounds checking. The code now properly validates indices and expands collections as needed.
However, there are some performance considerations for large modifications:
+// Consider validating reasonable limits for expansion to prevent abuse if (prodIndex >= moddedProductEntryHelpers.Count) { + if (prodIndex > 10000) // reasonable upper limit + throw new InvalidDataException($"Shop listpatch: ProductIndex {prodIndex} exceeds reasonable limit."); int dummiesToAdd = prodIndex - moddedProductEntryHelpers.Count;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
OpenKh.Kh2/SystemData/Shop.cs(1 hunks)OpenKh.Patcher/PatcherProcessor.cs(2 hunks)docs/tool/GUI.ModsManager/creatingMods.md(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
OpenKh.Patcher/PatcherProcessor.cs (2)
OpenKh.Kh2/SystemData/Shop.cs (19)
Shop(8-239)Shop(192-212)ShopHelper(184-190)List(64-64)List(83-83)List(97-97)ShopEntryHelper(47-62)ShopEntryHelper(106-155)ShopEntryHelper(122-122)ShopEntryHelper(123-138)InventoryEntryHelper(75-81)InventoryEntryHelper(157-171)ProductEntryHelper(91-95)ProductEntryHelper(173-182)Write(27-27)Write(65-65)Write(84-84)Write(98-98)Write(214-238)OpenKh.Kh2/BaseTable.cs (8)
List(14-20)List(39-44)List(60-66)List(89-95)Write(22-33)Write(46-51)Write(68-79)Write(97-107)
OpenKh.Kh2/SystemData/Shop.cs (1)
OpenKh.Kh2/BaseTable.cs (1)
BaseList(36-52)
🪛 LanguageTool
🪛 markdownlint-cli2 (0.17.2)
docs/tool/GUI.ModsManager/creatingMods.md
314-314: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (11)
OpenKh.Kh2/SystemData/Shop.cs (2)
1-15: LGTM on basic structure and constants.The imports, namespace structure, constants, and magic code/file type definitions look correct. The constants use appropriate naming and the magic code TZSH (reversed) is typical for binary formats.
17-28: LGTM on ShopHeader structure.The header structure correctly maps the binary layout with appropriate data types and includes standard Read/Write methods using BinaryMapping.
docs/tool/GUI.ModsManager/creatingMods.md (3)
22-22: LGTM on documentation addition.The addition of the shop listpatch target to the table of contents is consistent with the existing format.
314-314: LGTM on shop addition to listpatch types.The shop entry is correctly added to the list of supported listpatch types in the documentation.
422-453: LGTM on comprehensive shop source example.The shop source example is well-structured and demonstrates all the helper types properly. The example includes:
- ShopEntryHelpers with all required fields
- InventoryEntryHelpers showing the relationship between inventory and products
- ProductEntryHelpers for regular products
- ValidProductEntryHelpers for valid products
The example data appears consistent and realistic.
OpenKh.Patcher/PatcherProcessor.cs (6)
1-3: LGTM on new imports.The new using directives for OpenKh.Bbs, OpenKh.Command.Bdxio.Models, and OpenKh.Command.Bdxio.Utils are appropriate for the added functionality.
14-14: LGTM on static import for Shop.The static import makes the Shop class members easily accessible in the shop patching code.
1005-1019: LGTM on ShopEntry merging logic.The shop entry merging correctly handles both updating existing entries (by ShopID) and adding new entries. The use of FindIndex to locate entries by ShopID is appropriate.
1034-1050: LGTM on inventory entry expansion logic.The dummy entry creation for inventory entries includes all required fields with appropriate default values (0 for counts and indices).
1064-1080: LGTM on product entry expansion logic.The product entry expansion follows the same pattern as inventory entries and properly creates dummy entries with default ItemID of 0.
1092-1108: LGTM on valid product entry expansion logic.The valid product entry handling mirrors the regular product entry logic appropriately.
Comment on lines
+192
to
+212
| public static Shop Read(Stream stream) | ||
| { | ||
| if (!stream.CanRead || !stream.CanSeek) | ||
| throw new InvalidDataException($"Read or seek must be supported."); | ||
|
|
||
| ShopHeader header = ShopHeader.Read(stream); | ||
|
|
||
| if (header.MagicCode != MagicCode) | ||
| throw new InvalidDataException($"Invalid shop magic 0x{header.MagicCode:X8} (expected 0x{MagicCode:X8})."); | ||
| if (header.FileType != FileType) | ||
| throw new InvalidDataException($"Invalid shop file type 0x{header.FileType:X4} (expected 0x{FileType:X4})."); | ||
|
|
||
| var shop = new Shop() | ||
| { | ||
| ShopEntries = ShopEntry.Read(stream, header.ShopEntryCount), | ||
| InventoryEntries = InventoryEntry.Read(stream, header.InventoryEntryCount), | ||
| ProductEntries = ProductEntry.Read(stream, header.ProductEntryCount), | ||
| ValidProductEntries = ProductEntry.Read(stream, (int)((stream.Length - header.ValidProductsOffset) / ProductEntrySize)) | ||
| }; | ||
| return shop; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for ValidProductEntries count calculation.
The calculation for ValidProductEntries count could be improved with validation to prevent potential integer overflow or negative counts.
public static Shop Read(Stream stream)
{
if (!stream.CanRead || !stream.CanSeek)
throw new InvalidDataException($"Read or seek must be supported.");
ShopHeader header = ShopHeader.Read(stream);
if (header.MagicCode != MagicCode)
throw new InvalidDataException($"Invalid shop magic 0x{header.MagicCode:X8} (expected 0x{MagicCode:X8}).");
if (header.FileType != FileType)
throw new InvalidDataException($"Invalid shop file type 0x{header.FileType:X4} (expected 0x{FileType:X4}).");
+ var validProductsLength = stream.Length - header.ValidProductsOffset;
+ if (validProductsLength < 0 || validProductsLength % ProductEntrySize != 0)
+ throw new InvalidDataException($"Invalid ValidProductsOffset: stream length {stream.Length}, offset {header.ValidProductsOffset}");
+
+ var validProductsCount = (int)(validProductsLength / ProductEntrySize);
var shop = new Shop()
{
ShopEntries = ShopEntry.Read(stream, header.ShopEntryCount),
InventoryEntries = InventoryEntry.Read(stream, header.InventoryEntryCount),
ProductEntries = ProductEntry.Read(stream, header.ProductEntryCount),
- ValidProductEntries = ProductEntry.Read(stream, (int)((stream.Length - header.ValidProductsOffset) / ProductEntrySize))
+ ValidProductEntries = ProductEntry.Read(stream, validProductsCount)
};
return shop;
}📝 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
| public static Shop Read(Stream stream) | |
| { | |
| if (!stream.CanRead || !stream.CanSeek) | |
| throw new InvalidDataException($"Read or seek must be supported."); | |
| ShopHeader header = ShopHeader.Read(stream); | |
| if (header.MagicCode != MagicCode) | |
| throw new InvalidDataException($"Invalid shop magic 0x{header.MagicCode:X8} (expected 0x{MagicCode:X8})."); | |
| if (header.FileType != FileType) | |
| throw new InvalidDataException($"Invalid shop file type 0x{header.FileType:X4} (expected 0x{FileType:X4})."); | |
| var shop = new Shop() | |
| { | |
| ShopEntries = ShopEntry.Read(stream, header.ShopEntryCount), | |
| InventoryEntries = InventoryEntry.Read(stream, header.InventoryEntryCount), | |
| ProductEntries = ProductEntry.Read(stream, header.ProductEntryCount), | |
| ValidProductEntries = ProductEntry.Read(stream, (int)((stream.Length - header.ValidProductsOffset) / ProductEntrySize)) | |
| }; | |
| return shop; | |
| } | |
| public static Shop Read(Stream stream) | |
| { | |
| if (!stream.CanRead || !stream.CanSeek) | |
| throw new InvalidDataException($"Read or seek must be supported."); | |
| ShopHeader header = ShopHeader.Read(stream); | |
| if (header.MagicCode != MagicCode) | |
| throw new InvalidDataException($"Invalid shop magic 0x{header.MagicCode:X8} (expected 0x{MagicCode:X8})."); | |
| if (header.FileType != FileType) | |
| throw new InvalidDataException($"Invalid shop file type 0x{header.FileType:X4} (expected 0x{FileType:X4})."); | |
| var validProductsLength = stream.Length - header.ValidProductsOffset; | |
| if (validProductsLength < 0 || validProductsLength % ProductEntrySize != 0) | |
| throw new InvalidDataException($"Invalid ValidProductsOffset: stream length {stream.Length}, offset {header.ValidProductsOffset}"); | |
| var validProductsCount = (int)(validProductsLength / ProductEntrySize); | |
| var shop = new Shop() | |
| { | |
| ShopEntries = ShopEntry.Read(stream, header.ShopEntryCount), | |
| InventoryEntries = InventoryEntry.Read(stream, header.InventoryEntryCount), | |
| ProductEntries = ProductEntry.Read(stream, header.ProductEntryCount), | |
| ValidProductEntries = ProductEntry.Read(stream, validProductsCount) | |
| }; | |
| return shop; | |
| } |
Merged
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
2 participants
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.
Does what the title says. Also updates the docs with an example of the shop listpatch YAML.
Summary by CodeRabbit
New Features
Tests
Documentation