Listpatch Updates#1258
Merged
Merged
Conversation
Added 'went' source example to the creatingMods documentation.
…files of subfiles Added new source examples for 'prty', 'sstm', and 'slct'.
Removed several source examples from the creatingMods.md documentation.
📝 WalkthroughWalkthroughThis pull request adds comprehensive support for patching additional Kingdom Hearts II system data structures. It introduces custom binary serialization for LVUP data, creates new table definitions for SLCT/WENT/PRTY/SSTM data, extends the patcher processor to support these new formats, adds test coverage, and updates documentation with new listpatch examples. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 7
🧹 Nitpick comments (5)
OpenKh.Kh2/Slct.cs (1)
1-8: Unused imports can be removed.
OpenKh.Common,OpenKh.Kh2.Extensions,OpenKh.Kh2.Utils, andSystem.Linqappear to be unused in this file.♻️ Proposed fix
-using OpenKh.Common; -using OpenKh.Kh2.Extensions; -using OpenKh.Kh2.Utils; -using System; using System.Collections.Generic; using System.IO; -using System.Linq; using Xe.BinaryMapper;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Kh2/Slct.cs` around lines 1 - 8, The file contains unused using directives—remove the unnecessary imports OpenKh.Common, OpenKh.Kh2.Extensions, OpenKh.Kh2.Utils, and System.Linq from the top of Slct.cs and keep only the required usings (e.g., System, System.Collections.Generic, System.IO, Xe.BinaryMapper); update any code references if removal reveals missing types and rebuild to ensure no compilation errors in the Slct class or its methods.OpenKh.Kh2/SystemData/Pref/Prty.cs (1)
126-138: Consider removing commented-out code.Similar to
Sstm.cs, the commented-out Read/Write methods appear to be dead code. Consider removing them if the functionality has been replaced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Kh2/SystemData/Pref/Prty.cs` around lines 126 - 138, Remove the dead commented-out Read/Write block inside the Prty class: delete the multi-line commented methods that call BaseTableOffsetWithPaddings<Prty>.ReadWithOffsets and WriteWithOffsets so the file matches the cleaned Sstm.cs style; before removal, search for any references to these methods (Prty.Read / Prty.Write or BaseTableOffsetWithPaddings<Prty>) to ensure they are not relied upon or, if needed, replace with the new implementation rather than leaving commented code.docs/tool/GUI.ModsManager/listpatch_information.md (1)
39-42: Add language identifiers to fenced code blocks.Multiple code blocks throughout this documentation lack language specifiers (flagged by markdownlint MD040). Adding
yamlas the language identifier will enable proper syntax highlighting for mod authors.📝 Example fix
-``` +```yaml 2: ItemId: 347Apply this pattern to all code blocks in the document. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/tool/GUI.ModsManager/listpatch_information.mdaround lines 39 - 42, The
fenced code blocks in listpatch_information.md (e.g., the block containing the
snippet "2:\n ItemId: 347") lack a language tag causing markdownlint MD040;
update every triple-backtick block to include the yaml identifier (change ``` towith proper YAML highlighting.OpenKh.Kh2/SystemData/Pref/Sstm.cs (1)
123-133: Consider removing commented-out code.The commented-out
SstmPatchclass and associated Read/Write methods appear to be dead code. If this functionality is no longer needed, consider removing it entirely rather than leaving it commented out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Kh2/SystemData/Pref/Sstm.cs` around lines 123 - 133, Remove the dead commented-out block containing the SstmPatch class and the commented Read/Write wrappers (SstmPatch, Read(Stream) => BaseTableSstm<Sstm>.Read, Write(Stream, IEnumerable<Sstm>) => BaseTableSstm<Sstm>.Write); if you still need that functionality either fully implement SstmPatch and restore the Read/Write methods with working logic or delete the commented code to keep the file clean, then build and run tests to ensure no references remain to SstmPatch or those methods.OpenKh.Kh2/SystemData/Went.cs (1)
47-48: Consider extracting magic number 70 to a constant.The header size of 70 entries is hardcoded in multiple places. A named constant would improve readability and maintainability.
♻️ Proposed refactor
public class Went { + private const int HeaderEntryCount = 70; + public List<uint> Offsets { get; set; } public List<WentSet> Sets { get; set; } // ... public static Went Read(Stream stream) { // ... - for (int i = 0; i < 70; i++) + for (int i = 0; i < HeaderEntryCount; i++) went.Offsets.Add(ReadUInt32(stream));Also applies to: 82-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Kh2/SystemData/Went.cs` around lines 47 - 48, Replace the hardcoded magic number 70 with a named constant in the Went class: declare a private const int (e.g., OffsetCount = 70) near the top of the Went class and use that constant wherever the loop uses 70 (the for loops that call went.Offsets.Add(ReadUInt32(stream)) and the other occurrences around the 82-83 region) so all header-size references use the named constant for clarity and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tool/GUI.ModsManager/listpatch_information.md`:
- Around line 5-6: Remove the empty link in the Table of Contents header by
replacing the markdown "[Table of Contents]()" with either plain text "Table of
Contents" or a valid anchor like "[Table of Contents](`#table-of-contents`)";
update the line containing the "[Table of Contents]()" header in
listpatch_information.md accordingly so markdownlint MD042 is resolved.
- Around line 183-184: The example uses two inconsistent property names: change
the incorrect FlagNegationForWorld occurrence to NegationFlagForWorld so it
matches the Memt class property and the earlier example usage; update the
example entry that currently uses FlagNegationForWorld to use
NegationFlagForWorld to ensure consistency with the Memt class and the prior
NegationFlagForWorld line.
In `@OpenKh.Kh2/Battle/Lvup.cs`:
- Around line 111-119: WriteCharacter computes ch.NumLevels using ch.Levels but
then iterates ch.Levels without a null check, risking a NullReferenceException;
update WriteCharacter (the ch.NumLevels assignment, WriteInt32 call and the
foreach that calls BinaryMapping.WriteObject) to safely handle a null Levels by
either iterating over ch.Levels only when it is not null (e.g., if (ch.Levels !=
null) foreach ...) or by iterating over a safe empty sequence (e.g., ch.Levels
?? Enumerable.Empty<LevelType>()), so NumLevels remains correct and
BinaryMapping.WriteObject is only called for existing levels.
In `@OpenKh.Kh2/SystemData/Went.cs`:
- Around line 96-111: The loop over uniqueOffsets uses Sets.Find(x =>
x.OriginalOffset == oldOffset) which can return null, then dereferences set
(set.SetSize, set.WeaponIds) causing a NullReferenceException; update the loop
in the method that writes WENT entries to null-check the result of Sets.Find
(the local variable set) and handle the missing case—either skip this offset,
log/throw a descriptive exception referencing the bad OriginalOffset, or create
a default Set object—before mutating set.SetSize, updating offsetMap and calling
WriteUInt32; ensure subsequent uses of set.WeaponIds are safe after the
null-handling change.
In `@OpenKh.Patcher/PatcherProcessor.cs`:
- Around line 871-893: The sstm branch reads and mutates a Kh2.SystemData.Sstm
instance but forgets to reset the stream before writing, so the
sstm.Write(stream) will append instead of overwrite; update the case handling
(around the deserializer.Deserialize and sstm variable) to call
stream.SetLength(0) and stream.Position = 0 (same reset used in the went case)
immediately before invoking sstm.Write(stream) to ensure the stream is truncated
and writing starts at the beginning.
- Around line 811-823: Replace the direct index access to characterMap with a
TryGetValue check to validate character.Key before using it: call
characterMap.TryGetValue(character.Key, out var mapIndex) and if it returns
false throw a descriptive exception naming the invalid character key and the
YAML source; use mapIndex - 1 to compute charIndex and proceed as before. Also
remove the redundant assignment to characterData.NumLevels (since
Lvup.WriteSingleCharacter recalculates it), and keep the loop that ensures
Levels has enough entries and assigns level.Value to
characterData.Levels[levelIndex].
- Around line 895-928: The prty case handler must reset the stream position
before writing and validate dictionary/list accesses to avoid runtime
exceptions: after reading and applying changes to the Kh2.SystemData.PrtyFile
(via Kh2.SystemData.PrtyFile.Read and the deserialized mod), set the stream
position (and truncate if necessary) before calling file.Write(stream); and
before using file.Offsets[realIndex] and file.UniqueEntries[offset] ensure the
keys/indices exist (use bounds checks or ContainsKey/TryGetValue on file.Offsets
and file.UniqueEntries) and throw a clear Exception mentioning the character key
or missing offset/entry when validation fails instead of letting
IndexOutOfRange/KeyNotFound propagate; keep the existing use of
Kh2.SystemData.Prty.CharacterMap.TryGetValue to resolve character -> index.
---
Nitpick comments:
In `@docs/tool/GUI.ModsManager/listpatch_information.md`:
- Around line 39-42: The fenced code blocks in listpatch_information.md (e.g.,
the block containing the snippet "2:\n ItemId: 347") lack a language tag
causing markdownlint MD040; update every triple-backtick block to include the
yaml identifier (change ``` to ```yaml) so all examples (including the shown "2:
ItemId: 347" block) render with proper YAML highlighting.
In `@OpenKh.Kh2/Slct.cs`:
- Around line 1-8: The file contains unused using directives—remove the
unnecessary imports OpenKh.Common, OpenKh.Kh2.Extensions, OpenKh.Kh2.Utils, and
System.Linq from the top of Slct.cs and keep only the required usings (e.g.,
System, System.Collections.Generic, System.IO, Xe.BinaryMapper); update any code
references if removal reveals missing types and rebuild to ensure no compilation
errors in the Slct class or its methods.
In `@OpenKh.Kh2/SystemData/Pref/Prty.cs`:
- Around line 126-138: Remove the dead commented-out Read/Write block inside the
Prty class: delete the multi-line commented methods that call
BaseTableOffsetWithPaddings<Prty>.ReadWithOffsets and WriteWithOffsets so the
file matches the cleaned Sstm.cs style; before removal, search for any
references to these methods (Prty.Read / Prty.Write or
BaseTableOffsetWithPaddings<Prty>) to ensure they are not relied upon or, if
needed, replace with the new implementation rather than leaving commented code.
In `@OpenKh.Kh2/SystemData/Pref/Sstm.cs`:
- Around line 123-133: Remove the dead commented-out block containing the
SstmPatch class and the commented Read/Write wrappers (SstmPatch, Read(Stream)
=> BaseTableSstm<Sstm>.Read, Write(Stream, IEnumerable<Sstm>) =>
BaseTableSstm<Sstm>.Write); if you still need that functionality either fully
implement SstmPatch and restore the Read/Write methods with working logic or
delete the commented code to keep the file clean, then build and run tests to
ensure no references remain to SstmPatch or those methods.
In `@OpenKh.Kh2/SystemData/Went.cs`:
- Around line 47-48: Replace the hardcoded magic number 70 with a named constant
in the Went class: declare a private const int (e.g., OffsetCount = 70) near the
top of the Went class and use that constant wherever the loop uses 70 (the for
loops that call went.Offsets.Add(ReadUInt32(stream)) and the other occurrences
around the 82-83 region) so all header-size references use the named constant
for clarity and maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d142ff25-35ea-4545-9477-b7f3cb2cdc90
📒 Files selected for processing (9)
OpenKh.Kh2/Battle/Lvup.csOpenKh.Kh2/Slct.csOpenKh.Kh2/SystemData/Pref/Prty.csOpenKh.Kh2/SystemData/Pref/Sstm.csOpenKh.Kh2/SystemData/Went.csOpenKh.Patcher/PatcherProcessor.csOpenKh.Tests/Patcher/PatcherTests.csdocs/tool/GUI.ModsManager/creatingMods.mddocs/tool/GUI.ModsManager/listpatch_information.md
Comment on lines
+5
to
+6
|
|
||
| # [Table of Contents]() |
There was a problem hiding this comment.
Empty link in Table of Contents header.
The Table of Contents header has an empty link [Table of Contents]() which is flagged by markdownlint (MD042). Either add a valid anchor or remove the link syntax.
📝 Proposed fix
-# [Table of Contents]()
+# Table of Contents📝 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
| # [Table of Contents]() | |
| # Table of Contents |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 5-5: No empty links
(MD042, no-empty-links)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tool/GUI.ModsManager/listpatch_information.md` around lines 5 - 6,
Remove the empty link in the Table of Contents header by replacing the markdown
"[Table of Contents]()" with either plain text "Table of Contents" or a valid
anchor like "[Table of Contents](`#table-of-contents`)"; update the line
containing the "[Table of Contents]()" header in listpatch_information.md
accordingly so markdownlint MD042 is resolved.
Comment on lines
+183
to
+184
| NegationFlagForWorld: Twilight Town #Specify world name | ||
| CheckArea: 2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the correct property name in the Memt class
rg -n "FlagNegationForWorld|NegationFlagForWorld" --type=csRepository: OpenKH/OpenKh
Length of output: 654
🏁 Script executed:
sed -n '180,200p' docs/tool/GUI.ModsManager/listpatch_information.mdRepository: OpenKH/OpenKh
Length of output: 791
Inconsistent property name in MEMT example.
Line 183 uses NegationFlagForWorld but line 194 uses FlagNegationForWorld. Line 194 should be NegationFlagForWorld to match the actual property name in the Memt class and be consistent with line 183.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tool/GUI.ModsManager/listpatch_information.md` around lines 183 - 184,
The example uses two inconsistent property names: change the incorrect
FlagNegationForWorld occurrence to NegationFlagForWorld so it matches the Memt
class property and the earlier example usage; update the example entry that
currently uses FlagNegationForWorld to use NegationFlagForWorld to ensure
consistency with the Memt class and the prior NegationFlagForWorld line.
Comment on lines
+111
to
+119
| private static void WriteCharacter(Stream stream, PlayableCharacter ch) | ||
| { | ||
| ch.NumLevels = ch.Levels?.Count ?? 0; | ||
|
|
||
| WriteInt32(stream, ch.NumLevels); | ||
|
|
||
| foreach (var level in ch.Levels) | ||
| BinaryMapping.WriteObject(stream, level); | ||
| } |
There was a problem hiding this comment.
Potential NullReferenceException if ch.Levels is null.
Line 113 handles null with ch.Levels?.Count ?? 0, but line 117 iterates ch.Levels directly without a null check, which will throw if Levels is null.
🐛 Proposed fix
private static void WriteCharacter(Stream stream, PlayableCharacter ch)
{
ch.NumLevels = ch.Levels?.Count ?? 0;
WriteInt32(stream, ch.NumLevels);
- foreach (var level in ch.Levels)
+ if (ch.Levels != null)
+ {
+ foreach (var level in ch.Levels)
BinaryMapping.WriteObject(stream, level);
+ }
}📝 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
| private static void WriteCharacter(Stream stream, PlayableCharacter ch) | |
| { | |
| ch.NumLevels = ch.Levels?.Count ?? 0; | |
| WriteInt32(stream, ch.NumLevels); | |
| foreach (var level in ch.Levels) | |
| BinaryMapping.WriteObject(stream, level); | |
| } | |
| private static void WriteCharacter(Stream stream, PlayableCharacter ch) | |
| { | |
| ch.NumLevels = ch.Levels?.Count ?? 0; | |
| WriteInt32(stream, ch.NumLevels); | |
| if (ch.Levels != null) | |
| { | |
| foreach (var level in ch.Levels) | |
| BinaryMapping.WriteObject(stream, level); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Kh2/Battle/Lvup.cs` around lines 111 - 119, WriteCharacter computes
ch.NumLevels using ch.Levels but then iterates ch.Levels without a null check,
risking a NullReferenceException; update WriteCharacter (the ch.NumLevels
assignment, WriteInt32 call and the foreach that calls
BinaryMapping.WriteObject) to safely handle a null Levels by either iterating
over ch.Levels only when it is not null (e.g., if (ch.Levels != null) foreach
...) or by iterating over a safe empty sequence (e.g., ch.Levels ??
Enumerable.Empty<LevelType>()), so NumLevels remains correct and
BinaryMapping.WriteObject is only called for existing levels.
Comment on lines
+96
to
+111
| foreach (var oldOffset in uniqueOffsets) | ||
| { | ||
| var set = Sets.Find(x => x.OriginalOffset == oldOffset); | ||
|
|
||
| long currentPos = stream.Position; | ||
| uint newOffset = (uint)((currentPos - fileStart) / 4); | ||
|
|
||
| offsetMap[oldOffset] = newOffset; | ||
|
|
||
| set.SetSize = (uint)(set.WeaponIds.Count + 1); | ||
|
|
||
| WriteUInt32(stream, set.SetSize); | ||
|
|
||
| foreach (var id in set.WeaponIds) | ||
| WriteUInt32(stream, id); | ||
| } |
There was a problem hiding this comment.
Potential NullReferenceException if set not found.
Sets.Find() on line 98 can return null if no matching OriginalOffset is found. Line 105 then accesses set.SetSize without a null check.
🐛 Proposed fix
foreach (var oldOffset in uniqueOffsets)
{
var set = Sets.Find(x => x.OriginalOffset == oldOffset);
+ if (set == null)
+ throw new InvalidOperationException($"WentSet not found for offset {oldOffset}");
long currentPos = stream.Position;📝 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 oldOffset in uniqueOffsets) | |
| { | |
| var set = Sets.Find(x => x.OriginalOffset == oldOffset); | |
| long currentPos = stream.Position; | |
| uint newOffset = (uint)((currentPos - fileStart) / 4); | |
| offsetMap[oldOffset] = newOffset; | |
| set.SetSize = (uint)(set.WeaponIds.Count + 1); | |
| WriteUInt32(stream, set.SetSize); | |
| foreach (var id in set.WeaponIds) | |
| WriteUInt32(stream, id); | |
| } | |
| foreach (var oldOffset in uniqueOffsets) | |
| { | |
| var set = Sets.Find(x => x.OriginalOffset == oldOffset); | |
| if (set == null) | |
| throw new InvalidOperationException($"WentSet not found for offset {oldOffset}"); | |
| long currentPos = stream.Position; | |
| uint newOffset = (uint)((currentPos - fileStart) / 4); | |
| offsetMap[oldOffset] = newOffset; | |
| set.SetSize = (uint)(set.WeaponIds.Count + 1); | |
| WriteUInt32(stream, set.SetSize); | |
| foreach (var id in set.WeaponIds) | |
| WriteUInt32(stream, id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Kh2/SystemData/Went.cs` around lines 96 - 111, The loop over
uniqueOffsets uses Sets.Find(x => x.OriginalOffset == oldOffset) which can
return null, then dereferences set (set.SetSize, set.WeaponIds) causing a
NullReferenceException; update the loop in the method that writes WENT entries
to null-check the result of Sets.Find (the local variable set) and handle the
missing case—either skip this offset, log/throw a descriptive exception
referencing the bad OriginalOffset, or create a default Set object—before
mutating set.SetSize, updating offsetMap and calling WriteUInt32; ensure
subsequent uses of set.WeaponIds are safe after the null-handling change.
Comment on lines
+811
to
+823
| var charIndex = characterMap[character.Key] - 1; | ||
| var characterData = levelList.Characters[charIndex]; | ||
|
|
||
| foreach (var level in character.Value) | ||
| { | ||
| levelList.Characters[characterMap[character.Key] - 1].Levels[level.Key - 1] = moddedLevels[character.Key][level.Key]; | ||
| int levelIndex = level.Key - 1; | ||
| while (characterData.Levels.Count <= levelIndex) | ||
| { | ||
| characterData.Levels.Add(new Kh2.Battle.Lvup.PlayableCharacter.Level()); | ||
| } | ||
| characterData.Levels[levelIndex] = level.Value; | ||
| } | ||
| characterData.NumLevels = characterData.Levels.Count; |
There was a problem hiding this comment.
Missing error handling for unknown character names.
The code directly accesses characterMap[character.Key] without validation, which will throw a KeyNotFoundException with an unhelpful message if an invalid character name is provided in the YAML. Other new cases (e.g., went at line 839, prty at line 903) use TryGetValue with descriptive exceptions.
Additionally, line 823 setting NumLevels is redundant since Lvup.WriteSingleCharacter already recalculates it from Levels.Count (per the context snippet from Lvup.cs).
Suggested improvement for consistency
foreach (var character in moddedLevels)
{
- var charIndex = characterMap[character.Key] - 1;
+ if (!characterMap.TryGetValue(character.Key, out byte charId))
+ throw new Exception($"Invalid character name: {character.Key}");
+ var charIndex = charId - 1;
var characterData = levelList.Characters[charIndex];
foreach (var level in character.Value)
{
int levelIndex = level.Key - 1;
while (characterData.Levels.Count <= levelIndex)
{
characterData.Levels.Add(new Kh2.Battle.Lvup.PlayableCharacter.Level());
}
characterData.Levels[levelIndex] = level.Value;
}
- characterData.NumLevels = characterData.Levels.Count;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Patcher/PatcherProcessor.cs` around lines 811 - 823, Replace the
direct index access to characterMap with a TryGetValue check to validate
character.Key before using it: call characterMap.TryGetValue(character.Key, out
var mapIndex) and if it returns false throw a descriptive exception naming the
invalid character key and the YAML source; use mapIndex - 1 to compute charIndex
and proceed as before. Also remove the redundant assignment to
characterData.NumLevels (since Lvup.WriteSingleCharacter recalculates it), and
keep the loop that ensures Levels has enough entries and assigns level.Value to
characterData.Levels[levelIndex].
Comment on lines
+871
to
+893
| case "sstm": | ||
| { | ||
| var sstm = Kh2.SystemData.Sstm.Read(stream); | ||
|
|
||
| var mod = deserializer.Deserialize<Dictionary<string, object>>(sourceText); | ||
|
|
||
| foreach (var entry in mod) | ||
| { | ||
| var prop = typeof(Kh2.SystemData.Sstm).GetProperty(entry.Key); | ||
| if (prop == null) | ||
| throw new Exception($"Invalid SSTM field: {entry.Key}"); | ||
|
|
||
| var converted = Convert.ChangeType( | ||
| entry.Value, | ||
| Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType | ||
| ); | ||
|
|
||
| prop.SetValue(sstm, converted); | ||
| } | ||
|
|
||
| sstm.Write(stream); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Missing stream position reset before write.
Unlike the went case (lines 864-865) which correctly resets the stream with SetLength(0) and Position = 0 before writing, the sstm case writes directly without resetting the stream position. This will append data instead of overwriting, potentially corrupting the output.
Proposed fix
prop.SetValue(sstm, converted);
}
+ stream.SetLength(0);
+ stream.Position = 0;
sstm.Write(stream);
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Patcher/PatcherProcessor.cs` around lines 871 - 893, The sstm branch
reads and mutates a Kh2.SystemData.Sstm instance but forgets to reset the stream
before writing, so the sstm.Write(stream) will append instead of overwrite;
update the case handling (around the deserializer.Deserialize and sstm variable)
to call stream.SetLength(0) and stream.Position = 0 (same reset used in the went
case) immediately before invoking sstm.Write(stream) to ensure the stream is
truncated and writing starts at the beginning.
Comment on lines
+895
to
+928
| case "prty": | ||
| { | ||
| var file = Kh2.SystemData.PrtyFile.Read(stream); | ||
|
|
||
| var mod = deserializer.Deserialize<Dictionary<string, Dictionary<string, object>>>(sourceText); | ||
|
|
||
| foreach (var character in mod) | ||
| { | ||
| if (!Kh2.SystemData.Prty.CharacterMap.TryGetValue(character.Key, out int index)) | ||
| throw new Exception($"Invalid PRTY character: {character.Key}"); | ||
|
|
||
| int realIndex = index + 1; | ||
| int offset = file.Offsets[realIndex]; | ||
|
|
||
| var entry = file.UniqueEntries[offset]; | ||
|
|
||
| foreach (var field in character.Value) | ||
| { | ||
| var prop = typeof(Kh2.SystemData.Prty).GetProperty(field.Key); | ||
| if (prop == null) | ||
| throw new Exception($"Invalid PRTY field: {field.Key}"); | ||
|
|
||
| var converted = Convert.ChangeType( | ||
| field.Value, | ||
| Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType | ||
| ); | ||
|
|
||
| prop.SetValue(entry, converted); | ||
| } | ||
| } | ||
|
|
||
| file.Write(stream); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Missing stream position reset and potential dictionary access issues.
-
Stream reset missing: Same issue as
sstm-file.Write(stream)at line 926 doesn't reset the stream position first. -
Unvalidated dictionary access: Lines 907 and 909 access
file.Offsets[realIndex]andfile.UniqueEntries[offset]without checking if the keys exist, which could throwIndexOutOfRangeExceptionorKeyNotFoundExceptionif the PRTY file structure doesn't match expectations.
Proposed fix
if (!Kh2.SystemData.Prty.CharacterMap.TryGetValue(character.Key, out int index))
throw new Exception($"Invalid PRTY character: {character.Key}");
int realIndex = index + 1;
+ if (realIndex >= file.Offsets.Count)
+ throw new Exception($"Character index {realIndex} out of bounds for PRTY offsets");
+
int offset = file.Offsets[realIndex];
+ if (!file.UniqueEntries.TryGetValue(offset, out var entry))
+ throw new Exception($"No PRTY entry found at offset {offset}");
- var entry = file.UniqueEntries[offset];
foreach (var field in character.Value)
{
// ... existing code ...
}
}
+ stream.SetLength(0);
+ stream.Position = 0;
file.Write(stream);
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Patcher/PatcherProcessor.cs` around lines 895 - 928, The prty case
handler must reset the stream position before writing and validate
dictionary/list accesses to avoid runtime exceptions: after reading and applying
changes to the Kh2.SystemData.PrtyFile (via Kh2.SystemData.PrtyFile.Read and the
deserialized mod), set the stream position (and truncate if necessary) before
calling file.Write(stream); and before using file.Offsets[realIndex] and
file.UniqueEntries[offset] ensure the keys/indices exist (use bounds checks or
ContainsKey/TryGetValue on file.Offsets and file.UniqueEntries) and throw a
clear Exception mentioning the character key or missing offset/entry when
validation fails instead of letting IndexOutOfRange/KeyNotFound propagate; keep
the existing use of Kh2.SystemData.Prty.CharacterMap.TryGetValue to resolve
character -> index.
shananas
approved these changes
Apr 30, 2026
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.
The following updates have been made for Listpatching:
Lvup has been updated to no longer be hardcoded to 99 entries. Entries are able to be added as specified, and the pointers in the header will be adjusted to reflect this. Mods from prior versions should remain unaffected by this change,.
Went has been added as a listpatchable field, which should improve compatibility with mods that add multiple weapons..
The other pref files Prty and Sstm have been added as valid listpatches, in addition to 14mission.bar's 'slct' file.
Tests have been added for each listpatch.
Due to how large the listpatch section has grown over time, I ended up separating all of the listpatch examples out into their own document, which should make the creatingMods page easier to browse through.
Summary by CodeRabbit
New Features
Tests
Documentation