Cherry picked Linux fixes and improvements from pr #1096 and #1199 #1204
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes convert OpenKh.Bbs.Scd to instance-based parsing with media extraction, add an output mode to OpenKh.Egs extraction, introduce new readers/models for KH1 CVBL and KH2 BGM/BOP (plus an empty WD class), and tweak a docs link. Public APIs are added/modified accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Scd as Scd.Read(stream)
participant Stream
participant Entries as StreamHeaders/Files
participant Media as MediaFiles
Caller->>Scd: Read(Stream)
Scd->>Stream: Read FileHeader, tableOffsetHeader
loop For each sound offset
Scd->>Stream: Seek and read entry header
Scd->>Entries: Store StreamHeader
Scd->>Stream: Read bytes [off .. next)
alt Codec == 6 (OGG)
Scd->>Scd: Extract key, decrypt slice
Scd->>Media: Store decrypted OGG
else Codec == 12 (MSADPCM)
Scd->>Scd: Build WAV-like container
Scd->>Media: Store ADPCM container
else Other
Scd->>Media: Store null
end
Scd->>Entries: Store raw entry bytes
end
Scd-->>Caller: Scd instance (FileHeader, StreamHeaders, StreamFiles, MediaFiles)
sequenceDiagram
autonumber
participant User
participant Egs as EgsTools.Extract
participant FS as File System
User->>Egs: Extract(inputHed, outputDir, doNotExtractAgain, outputMode)
alt outputMode == SeparateRoot
Egs->>FS: Originals -> outputDir/ORIGINAL_FILES_FOLDER_NAME/...
Egs->>FS: Remastered -> outputDir/REMASTERED_FILES_FOLDER_NAME/...
else outputMode == Adjacent
Egs->>FS: Originals -> outputDir/<file>
Egs->>FS: Remastered -> outputDir/<file>.remastered.d/...
end
Egs-->>User: Done
sequenceDiagram
autonumber
participant Loader as Cvbl(stream, mdlsJoints)
participant Stream
participant Joints as MdlsJointsToDictionary
participant Sub as Submeshes
Loader->>Stream: Read CvblHeader
alt HasUnknownEntries valid
Loader->>Stream: Read MeshEntries
Loader->>Joints: Build joint matrices
loop Each Mesh
Loader->>Stream: Read subsections
alt Vertex format 8/9/10
Loader->>Loader: Read vertices via BinaryMapping
Loader->>Loader: Skin to global via joint weights
Loader->>Sub: Append vertices/faces, joint slots
else Unknown
note over Loader: Stop parsing this mesh
end
end
else Invalid
note over Loader: Early return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 (15)
OpenKh.Kh2/Wd.cs (1)
3-6: Clarify the purpose of this empty placeholder class.This class has no members, methods, or documentation. Empty classes can indicate incomplete work or future placeholders, but they add no value to the codebase in their current state.
Consider one of the following:
- If this is a work-in-progress placeholder for future implementation, add a TODO comment explaining the intended functionality and timeline.
- If this class is not yet needed, defer adding it until the implementation is ready.
- If you have partial implementation details, I can help scaffold the class structure.
Would you like me to search the codebase to understand how Wd files are typically structured in Kingdom Hearts data formats?
OpenKh.Kh2/Bop.cs (2)
56-58: Clarify magic numbers and improve documentation.Line 56 mentions "the number 8" in a comment, but the actual value read is discarded. Line 58 uses a magic number
0x68(104 bytes) to calculate entry count without explanation.Apply this diff to improve code clarity:
- stream.ReadUInt32(); //the number 8 + var header = stream.ReadUInt32(); // Header value (expected: 8) var fileSize = stream.ReadUInt32(); - var count = (int)(fileSize / 0x68); + const int BopEntrySize = 0x68; // 104 bytes per BopEntry (25 fields × 4 bytes) + var count = (int)(fileSize / BopEntrySize);Optionally, consider validating the header value if it's always expected to be 8.
52-63: Consider adding stream validation.The Read method doesn't validate that the stream has sufficient data or that the calculated count is reasonable. Malformed data could cause exceptions or excessive memory allocation.
Consider adding basic validation:
public static Bop Read(Stream stream) { var bop = new Bop(); var header = stream.ReadUInt32(); var fileSize = stream.ReadUInt32(); + + if (fileSize % 0x68 != 0) + { + throw new InvalidDataException($"File size {fileSize} is not a multiple of BopEntry size (0x68)"); + } + const int BopEntrySize = 0x68; var count = (int)(fileSize / BopEntrySize); bop.Entries = Enumerable.Range(0, count).Select(_ => BinaryMapping.ReadObject<BopEntry>(stream)).ToList(); return bop; }OpenKh.Egs/EgsTools.cs (1)
89-93: Clear implementation of output modes.The switch expressions cleanly implement the two output strategies:
- SeparateRoot: Maintains the existing folder structure (
original/andremastered/)- Adjacent: Places files directly in the output directory with a
.remastered.dsuffix for HD assetsThe logic is straightforward and the default case correctly falls back to SeparateRoot.
Consider adding XML documentation to the
Extractmethod and theExtractFileOutputModeenum to help users understand when to use each mode. For example:/// <summary> /// Specifies how extracted files should be organized in the output directory. /// </summary> public enum ExtractFileOutputMode { /// <summary> /// Extract original files to 'original/' and remastered assets to 'remastered/' subdirectories. /// </summary> SeparateRoot, /// <summary> /// Extract original files directly to the output directory and remastered assets to '{filename}.remastered.d/' subdirectories. /// </summary> Adjacent, }Also applies to: 105-109
OpenKh.Kh2/Bgm.cs (4)
29-29: Consider using a property instead of a public field.Line 29 declares
Tracksas a public field, while all other class members use properties with[Data]attributes. This inconsistency could cause issues with serialization frameworks or code that expects uniform property access.Apply this diff to make Tracks a property:
- public List<byte[]> Tracks = new(); + public List<byte[]> Tracks { get; set; } = new();
31-34: Remove or utilize the unused BgmTrack class.The
BgmTracknested class is defined but never instantiated or used. TheReadmethod populatesTracksasList<byte[]>directly, bypassing this class entirely.Consider one of the following:
- Option 1: Remove the unused class if it serves no purpose.
- Option 2: Use
BgmTrackinstances in theTrackslist if structured track data is needed:- public List<byte[]> Tracks { get; set; } = new(); + public List<BgmTrack> Tracks { get; set; } = new(); public class BgmTrack { - [Data] public byte[] Raw { get; set; } + public byte[] Raw { get; set; } } public static Bgm Read(Stream stream) { var bgm = BinaryMapping.ReadObject<Bgm>(stream); - bgm.Tracks = Enumerable.Range(0, bgm.TrackCount).Select(_ => stream.ReadBytes((int)stream.ReadUInt32())).ToList(); + bgm.Tracks = Enumerable.Range(0, bgm.TrackCount) + .Select(_ => new BgmTrack { Raw = stream.ReadBytes((int)stream.ReadUInt32()) }) + .ToList(); return bgm; }
36-43: Consider adding validation for track data.The
Readmethod doesn't validate that the stream contains sufficient data for all tracks or thatTrackCountis reasonable. Malformed BGM files could cause out-of-memory exceptions or unexpected behavior.Consider adding basic validation:
public static Bgm Read(Stream stream) { var bgm = BinaryMapping.ReadObject<Bgm>(stream); + + if (bgm.TrackCount > 256) // Reasonable upper limit + { + throw new InvalidDataException($"TrackCount {bgm.TrackCount} exceeds reasonable limit"); + } bgm.Tracks = Enumerable.Range(0, bgm.TrackCount).Select(_ => stream.ReadBytes((int)stream.ReadUInt32())).ToList(); return bgm; }
83-118: Consider simplifying the switch statement.The
ArgumentByteReadCountmethod uses a large switch statement with multiple cases returning the same value. While functional, this could be simplified for better maintainability.Consider using a switch expression with pattern matching for cleaner code:
public static int ArgumentByteReadCount(this BgmTrackCommandType command) { - switch (command) + return command switch { - case BgmTrackCommandType.SetTempo: - case BgmTrackCommandType.Unk0: - case BgmTrackCommandType.Unk1: - case BgmTrackCommandType.NoteOnPreviousVelocity: - case BgmTrackCommandType.NoteOnPreviousKey: - case BgmTrackCommandType.NoteOff: - case BgmTrackCommandType.ProgramChange: - case BgmTrackCommandType.Volume: - case BgmTrackCommandType.Expression: - case BgmTrackCommandType.Pan: - case BgmTrackCommandType.Unk3: - case BgmTrackCommandType.Unk4: - case BgmTrackCommandType.Unk5: - case BgmTrackCommandType.Unk6: - case BgmTrackCommandType.SustainPedal: - case BgmTrackCommandType.Unk7: - case BgmTrackCommandType.Unk12: - case BgmTrackCommandType.Portamento: - return 1; - case BgmTrackCommandType.TimeSignature: - case BgmTrackCommandType.NoteOn: - case BgmTrackCommandType.Unk2: - case BgmTrackCommandType.Unk9: - return 2; - case BgmTrackCommandType.Unk8: - case BgmTrackCommandType.Unk10: - case BgmTrackCommandType.Unk11: - return 3; - default: - return 0; - } + BgmTrackCommandType.SetTempo or + BgmTrackCommandType.Unk0 or + BgmTrackCommandType.Unk1 or + BgmTrackCommandType.NoteOnPreviousVelocity or + BgmTrackCommandType.NoteOnPreviousKey or + BgmTrackCommandType.NoteOff or + BgmTrackCommandType.ProgramChange or + BgmTrackCommandType.Volume or + BgmTrackCommandType.Expression or + BgmTrackCommandType.Pan or + BgmTrackCommandType.Unk3 or + BgmTrackCommandType.Unk4 or + BgmTrackCommandType.Unk5 or + BgmTrackCommandType.Unk6 or + BgmTrackCommandType.SustainPedal or + BgmTrackCommandType.Unk7 or + BgmTrackCommandType.Unk12 or + BgmTrackCommandType.Portamento => 1, + + BgmTrackCommandType.TimeSignature or + BgmTrackCommandType.NoteOn or + BgmTrackCommandType.Unk2 or + BgmTrackCommandType.Unk9 => 2, + + BgmTrackCommandType.Unk8 or + BgmTrackCommandType.Unk10 or + BgmTrackCommandType.Unk11 => 3, + + _ => 0 + }; }OpenKh.Bbs/Scd.cs (4)
65-69: Downlevel C# compatibility for collection/array expressions.
[]collection expressions may require C# 12. If the repo targets older language versions, compilation will fail. Prefer explicit constructors.- public List<StreamHeader> StreamHeaders = []; - public List<byte[]> StreamFiles = []; - public List<byte[]> MediaFiles = []; //... + public List<StreamHeader> StreamHeaders = new List<StreamHeader>(); + public List<byte[]> StreamFiles = new List<byte[]>(); + public List<byte[]> MediaFiles = new List<byte[]>(); //...
157-160: Avoid silent no-op write.Empty
Writecan mislead callers. Throw until implemented.- public static void Write(Scd scd, Stream stream) - { - - } + public static void Write(Scd scd, Stream stream) + { + throw new NotImplementedException("Scd.Write is not implemented yet."); + }
66-66: Naming consistency.
tableOffsetHeader(field-like, camelCase) vsFileHeader(PascalCase). Use consistent public API naming.- public TableOffsetHeader tableOffsetHeader = new(); + public TableOffsetHeader TableOffsetHeader = new();And update usages accordingly.
12-15: Unused constants.
MagicCode,FileVersion,SSCFVersionare not validated. Consider validating on read or remove.OpenKh.Kh1/Cvbl.cs (3)
63-75: C# 12 array literals[]may not compile on older targets.
get => [JointSlotId];,Weights => [Weight];, etc. require collection expressions. Use explicit array creation for broader compatibility.- get => [JointSlotId]; + get => new ushort[] { JointSlotId }; @@ - public float[] Weights => [Weight]; + public float[] Weights => new float[] { Weight }; @@ - get => [JointSlotId]; + get => new ushort[] { JointSlotId }; @@ - public float[] Weights => [1]; + public float[] Weights => new float[] { 1f };Also applies to: 72-73, 144-151, 151-152
206-215: Avoid full stream duplication when possible.
ReadAllBytes()thennew MemoryStream(file)doubles memory. Ifstream.CanSeek, operate directly onstreamor copy once.- var file = stream.ReadAllBytes(); - var str = new MemoryStream(file); + var str = stream.CanSeek ? stream : new MemoryStream(stream.ReadAllBytes()); + str.Position = 0;
254-262: Magic numbers: document or centralize subsection layout.
+8,+24offsets obscure structure. Define constants or structs for subsection headers to prevent drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
OpenKh.Bbs/Scd.cs(3 hunks)OpenKh.Egs/EgsTools.cs(3 hunks)OpenKh.Kh1/Cvbl.cs(1 hunks)OpenKh.Kh2/Bgm.cs(1 hunks)OpenKh.Kh2/Bop.cs(1 hunks)OpenKh.Kh2/Wd.cs(1 hunks)docs/kh2/file/model.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
OpenKh.Egs/EgsTools.cs (1)
OpenKh.Command.IdxImg/EpicGamesAssets.cs (6)
ExtractCommand(33-72)Egs(59-59)Egs(67-67)OpenKh(17-112)Command(21-110)OnExecute(48-71)
OpenKh.Kh2/Bop.cs (1)
OpenKh.Common/StreamExtensions.cs (1)
StreamExtensions(11-333)
OpenKh.Kh1/Cvbl.cs (1)
OpenKh.Kh1/Mdls.cs (1)
MdlsJoint(144-182)
OpenKh.Bbs/Scd.cs (2)
OpenKh.Egs/EgsTools.cs (2)
List(452-626)List(632-645)OpenKh.Bbs/PAtk.cs (1)
List(50-55)
⏰ 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 (6)
docs/kh2/file/model.md (1)
64-64: LGTM! Documentation improvement.Converting the plain text label to a Markdown link improves documentation navigability, allowing readers to quickly access the UVSC UV scroll reference.
OpenKh.Kh2/Bop.cs (1)
12-45: LGTM! Well-structured data class.The BopEntry class is well-organized with clear field names for transformation data (position, rotation, scale) and bounding values. The use of BinaryMapping attributes for serialization is consistent with the codebase pattern.
OpenKh.Egs/EgsTools.cs (2)
71-75: LGTM! Clear enum for output modes.The enum provides two distinct output strategies with descriptive names. This is a clean abstraction for the feature.
77-77: Backward compatibility verified. All existing calls supply three arguments and will continue to default toExtractFileOutputMode.SeparateRoot, so no changes are needed.OpenKh.Kh2/Bgm.cs (2)
13-28: LGTM! Well-structured data model.The Bgm class properties are clearly named and appropriately typed with BinaryMapping attributes for deserialization. The use of Reserved fields for unknown data is a good practice.
46-79: LGTM! Comprehensive command type enumeration.The enum provides a well-documented set of MIDI-like commands for BGM tracks. The use of explicit byte values and descriptive names (with "Unk" for unknown/reverse-engineered commands) is appropriate for binary format parsing.
| for (var index = 0; index < soundOffsets.Count; index++) | ||
| { | ||
| var off = soundOffsets[index]; | ||
| var next = (int)(index == soundOffsets.Count - 1 ? stream.Length : soundOffsets[index + 1] - off); | ||
| stream.Seek(off, SeekOrigin.Begin); | ||
| var streamInfo = BinaryMapping.ReadObject<StreamHeader>(stream); | ||
|
|
||
| byte[] st = stream.ReadBytes((int)streamInfo.StreamSize); | ||
| StreamFiles.Add(st); | ||
|
|
||
| // Convert to VAG Streams. | ||
| /*VAGHeader vagHeader = new VAGHeader(); | ||
| vagHeader.ChannelCount = (byte)streamInfo.ChannelCount; | ||
| vagHeader.SamplingFrequency = (byte)streamInfo.SampleRate; | ||
| vagHeader.Version = 3; | ||
| vagHeader.Magic = 0x70474156; | ||
| vagHeader.Name = p.ToString();*/ | ||
| scd.StreamHeaders.Add(streamInfo); | ||
|
|
||
| var st = stream.ReadBytes(next - 0x20); | ||
| scd.StreamFiles.Add(st); |
There was a problem hiding this comment.
Incorrect last-stream size computation (can overread).
For the last entry, next is set to stream.Length instead of (stream.Length - off). This can yield a huge next - 0x20, causing overreads and truncated buffers.
Apply:
- var next = (int)(index == soundOffsets.Count - 1 ? stream.Length : soundOffsets[index + 1] - off);
+ var next = (int)(index == soundOffsets.Count - 1
+ ? (stream.Length - off)
+ : (soundOffsets[index + 1] - off));
+ if (next < 0x20) throw new InvalidDataException($"Stream block too small at offset 0x{off:X}.");📝 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.
| for (var index = 0; index < soundOffsets.Count; index++) | |
| { | |
| var off = soundOffsets[index]; | |
| var next = (int)(index == soundOffsets.Count - 1 ? stream.Length : soundOffsets[index + 1] - off); | |
| stream.Seek(off, SeekOrigin.Begin); | |
| var streamInfo = BinaryMapping.ReadObject<StreamHeader>(stream); | |
| byte[] st = stream.ReadBytes((int)streamInfo.StreamSize); | |
| StreamFiles.Add(st); | |
| // Convert to VAG Streams. | |
| /*VAGHeader vagHeader = new VAGHeader(); | |
| vagHeader.ChannelCount = (byte)streamInfo.ChannelCount; | |
| vagHeader.SamplingFrequency = (byte)streamInfo.SampleRate; | |
| vagHeader.Version = 3; | |
| vagHeader.Magic = 0x70474156; | |
| vagHeader.Name = p.ToString();*/ | |
| scd.StreamHeaders.Add(streamInfo); | |
| var st = stream.ReadBytes(next - 0x20); | |
| scd.StreamFiles.Add(st); | |
| for (var index = 0; index < soundOffsets.Count; index++) | |
| { | |
| var off = soundOffsets[index]; | |
| var next = (int)(index == soundOffsets.Count - 1 | |
| ? (stream.Length - off) | |
| : (soundOffsets[index + 1] - off)); | |
| if (next < 0x20) throw new InvalidDataException($"Stream block too small at offset 0x{off:X}."); | |
| stream.Seek(off, SeekOrigin.Begin); | |
| var streamInfo = BinaryMapping.ReadObject<StreamHeader>(stream); | |
| scd.StreamHeaders.Add(streamInfo); | |
| var st = stream.ReadBytes(next - 0x20); | |
| scd.StreamFiles.Add(st); |
🤖 Prompt for AI Agents
In OpenKh.Bbs/Scd.cs around lines 86 to 95, the computation of `next` for the
last stream uses `stream.Length` (an absolute position) instead of the remaining
size, which can make `next - 0x20` huge and cause overreads; change the
calculation so `next` is the size of the stream chunk: for the last entry set
`next = (int)(stream.Length - off)` otherwise `next = (int)(soundOffsets[index +
1] - off)`, and then ensure the read length (e.g., `next - 0x20`) is clamped to
>= 0 and does not exceed `stream.Length - stream.Position` before calling
`ReadBytes`.
| var st = stream.ReadBytes(next - 0x20); | ||
| scd.StreamFiles.Add(st); | ||
|
|
There was a problem hiding this comment.
Guard against negative/oversized payload read.
stream.ReadBytes(next - 0x20) assumes next >= 0x20. Add a check to avoid negative or oversized reads.
- var st = stream.ReadBytes(next - 0x20);
+ if (next < 0x20) throw new InvalidDataException("Invalid stream header size.");
+ var payloadSize = next - 0x20;
+ var st = stream.ReadBytes(payloadSize);
+ if (st.Length != payloadSize) throw new EndOfStreamException("Unexpected end of stream.");📝 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.
| var st = stream.ReadBytes(next - 0x20); | |
| scd.StreamFiles.Add(st); | |
| if (next < 0x20) | |
| throw new InvalidDataException("Invalid stream header size."); | |
| var payloadSize = next - 0x20; | |
| var st = stream.ReadBytes(payloadSize); | |
| if (st.Length != payloadSize) | |
| throw new EndOfStreamException("Unexpected end of stream."); | |
| scd.StreamFiles.Add(st); |
🤖 Prompt for AI Agents
In OpenKh.Bbs/Scd.cs around lines 94 to 96, the call stream.ReadBytes(next -
0x20) assumes next >= 0x20 and that the resulting length fits within the
remaining stream; validate before reading: compute length = next - 0x20, if
length < 0 or length > remainingBytes (e.g., stream.Length - stream.Position)
then handle as invalid data (throw a descriptive exception or clamp/skip as
appropriate for your error policy) and only call stream.ReadBytes(length) when
the length is valid; this prevents negative or oversized reads and avoids
corrupt/overflow reads.
| var extradataOffset = 0u; | ||
| if (streamInfo.AuxChunkCount > 0) extradataOffset += BitConverter.ToUInt32(st.Skip(0x04).Take(4).ToArray(), 0); | ||
|
|
||
| var encryptionKey = st[extradataOffset + 0x02]; | ||
| var seekTableSize = BitConverter.ToUInt32(st.Skip((int)extradataOffset + 0x10).Take(4).ToArray(), 0); | ||
| var vorbHeaderSize = BitConverter.ToUInt32(st.Skip((int)extradataOffset + 0x14).Take(4).ToArray(), 0); | ||
|
|
||
| var startOffset = extradataOffset + 0x20 + seekTableSize; | ||
|
|
||
| var decryptedFile = st.ToArray(); | ||
|
|
||
| var endPosition = startOffset + vorbHeaderSize; | ||
|
|
||
| for (var i = startOffset; i < endPosition; i++) decryptedFile[i] = (byte)(decryptedFile[i]^encryptionKey); | ||
|
|
||
| var oggSize = vorbHeaderSize + streamInfo.StreamSize; | ||
|
|
||
| scd.MediaFiles.Add(decryptedFile.Skip((int)startOffset).Take((int)oggSize).ToArray()); |
There was a problem hiding this comment.
Array index type mismatches (won’t compile) and missing bounds checks.
st[extradataOffset + 0x02]usesuint/longfor indexing; arrays requireint.- Loop variables
iinfor (var i = startOffset; ... )areuint; indexingdecryptedFile[i]won’t compile. - No guards for insufficient
stlength before slicing.
Use int indices and validate sizes:
- var extradataOffset = 0u;
- if (streamInfo.AuxChunkCount > 0) extradataOffset += BitConverter.ToUInt32(st.Skip(0x04).Take(4).ToArray(), 0);
-
- var encryptionKey = st[extradataOffset + 0x02];
- var seekTableSize = BitConverter.ToUInt32(st.Skip((int)extradataOffset + 0x10).Take(4).ToArray(), 0);
- var vorbHeaderSize = BitConverter.ToUInt32(st.Skip((int)extradataOffset + 0x14).Take(4).ToArray(), 0);
+ var extradataOffset = 0;
+ if (streamInfo.AuxChunkCount > 0)
+ extradataOffset = (int)BitConverter.ToUInt32(st.AsSpan(0x04, 4));
+
+ if (extradataOffset + 0x20 > st.Length)
+ throw new InvalidDataException("Extradata offset beyond stream data.");
+
+ var encryptionKey = st[extradataOffset + 0x02];
+ var seekTableSize = (int)BitConverter.ToUInt32(st.AsSpan(extradataOffset + 0x10, 4));
+ var vorbHeaderSize = (int)BitConverter.ToUInt32(st.AsSpan(extradataOffset + 0x14, 4));
@@
- var startOffset = extradataOffset + 0x20 + seekTableSize;
+ var startOffset = extradataOffset + 0x20 + seekTableSize;
@@
- var endPosition = startOffset + vorbHeaderSize;
-
- for (var i = startOffset; i < endPosition; i++) decryptedFile[i] = (byte)(decryptedFile[i]^encryptionKey);
+ var endPosition = startOffset + vorbHeaderSize;
+ if (endPosition > decryptedFile.Length)
+ throw new InvalidDataException("Vorb header extends past buffer.");
+ for (int i = startOffset; i < endPosition; i++)
+ decryptedFile[i] = (byte)(decryptedFile[i] ^ encryptionKey);
@@
- var oggSize = vorbHeaderSize + streamInfo.StreamSize;
-
- scd.MediaFiles.Add(decryptedFile.Skip((int)startOffset).Take((int)oggSize).ToArray());
+ var oggSize = vorbHeaderSize + (int)streamInfo.StreamSize;
+ if (startOffset + oggSize > decryptedFile.Length)
+ oggSize = decryptedFile.Length - startOffset;
+ scd.MediaFiles.Add(decryptedFile.AsSpan(startOffset, oggSize).ToArray());📝 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.
| var extradataOffset = 0u; | |
| if (streamInfo.AuxChunkCount > 0) extradataOffset += BitConverter.ToUInt32(st.Skip(0x04).Take(4).ToArray(), 0); | |
| var encryptionKey = st[extradataOffset + 0x02]; | |
| var seekTableSize = BitConverter.ToUInt32(st.Skip((int)extradataOffset + 0x10).Take(4).ToArray(), 0); | |
| var vorbHeaderSize = BitConverter.ToUInt32(st.Skip((int)extradataOffset + 0x14).Take(4).ToArray(), 0); | |
| var startOffset = extradataOffset + 0x20 + seekTableSize; | |
| var decryptedFile = st.ToArray(); | |
| var endPosition = startOffset + vorbHeaderSize; | |
| for (var i = startOffset; i < endPosition; i++) decryptedFile[i] = (byte)(decryptedFile[i]^encryptionKey); | |
| var oggSize = vorbHeaderSize + streamInfo.StreamSize; | |
| scd.MediaFiles.Add(decryptedFile.Skip((int)startOffset).Take((int)oggSize).ToArray()); | |
| var extradataOffset = 0; | |
| if (streamInfo.AuxChunkCount > 0) | |
| extradataOffset = (int)BitConverter.ToUInt32(st.AsSpan(0x04, 4)); | |
| // Ensure we have at least the fixed extradata header | |
| if (extradataOffset + 0x20 > st.Length) | |
| throw new InvalidDataException("Extradata offset beyond stream data."); | |
| var encryptionKey = st[extradataOffset + 0x02]; | |
| var seekTableSize = (int)BitConverter.ToUInt32(st.AsSpan(extradataOffset + 0x10, 4)); | |
| var vorbHeaderSize = (int)BitConverter.ToUInt32(st.AsSpan(extradataOffset + 0x14, 4)); | |
| var startOffset = extradataOffset + 0x20 + seekTableSize; | |
| var decryptedFile = st.ToArray(); | |
| var endPosition = startOffset + vorbHeaderSize; | |
| // Make sure the Vorbis header fits in the buffer | |
| if (endPosition > decryptedFile.Length) | |
| throw new InvalidDataException("Vorb header extends past buffer."); | |
| for (int i = startOffset; i < endPosition; i++) | |
| decryptedFile[i] = (byte)(decryptedFile[i] ^ encryptionKey); | |
| // Clamp Ogg data to the actual buffer length | |
| var oggSize = vorbHeaderSize + (int)streamInfo.StreamSize; | |
| if (startOffset + oggSize > decryptedFile.Length) | |
| oggSize = decryptedFile.Length - startOffset; | |
| scd.MediaFiles.Add(decryptedFile.AsSpan(startOffset, oggSize).ToArray()); |
🤖 Prompt for AI Agents
In OpenKh.Bbs/Scd.cs around lines 102 to 119, the code uses uint/long values to
index arrays and slices without validating buffer lengths which causes compile
errors and possible out-of-bounds reads; change all index variables
(extradataOffset, startOffset, endPosition, loop index i, oggSize,
seekTableSize, vorbHeaderSize) to int (or explicitly cast uint->int after
validation), perform explicit bounds checks before reading from st (ensure
st.Length >= extradataOffset + required header sizes and that extradataOffset +
0x02, +0x10, +0x14 are inside the buffer), validate startOffset + vorbHeaderSize
and startOffset + oggSize are within decryptedFile.Length before the XOR loop
and the final Slice/Take, and if checks fail throw a clear exception or return
early; then use an int loop for i from startOffset to endPosition and index
decryptedFile[i] safely.
| for (var i = 0u; i < joints.Count; i++) | ||
| { | ||
| var j = joints[(int)i]; | ||
|
|
||
| var scaleMatrix = Matrix4x4.CreateScale(new Vector3(j.ScaleX, j.ScaleY, j.ScaleZ)); | ||
|
|
||
| var rotationMatrixX = Matrix4x4.CreateRotationX(j.RotateX); | ||
| var rotationMatrixY = Matrix4x4.CreateRotationY(j.RotateY); | ||
| var rotationMatrixZ = Matrix4x4.CreateRotationZ(j.RotateZ); | ||
| var rotationMatrix = rotationMatrixX * rotationMatrixY * rotationMatrixZ; | ||
|
|
||
| var translationMatrix = Matrix4x4.CreateTranslation(new Vector3(j.TranslateX, j.TranslateY, j.TranslateZ)); | ||
|
|
||
| dict[i] = scaleMatrix * rotationMatrix * translationMatrix; | ||
| } | ||
| return dict; | ||
| } |
There was a problem hiding this comment.
Bone mapping likely incorrect: using loop index instead of joint’s declared Index.
Dictionary is keyed by i, ignoring MdlsJoint.Index. If the list isn’t strictly ordered by Index, vertices will use wrong transforms.
- for (var i = 0u; i < joints.Count; i++)
+ for (var i = 0u; i < joints.Count; i++)
{
var j = joints[(int)i];
@@
- dict[i] = scaleMatrix * rotationMatrix * translationMatrix;
+ var key = j.Index; // authoritative joint id
+ dict[key] = scaleMatrix * rotationMatrix * translationMatrix;
}Optionally assert uniqueness of j.Index.
📝 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.
| for (var i = 0u; i < joints.Count; i++) | |
| { | |
| var j = joints[(int)i]; | |
| var scaleMatrix = Matrix4x4.CreateScale(new Vector3(j.ScaleX, j.ScaleY, j.ScaleZ)); | |
| var rotationMatrixX = Matrix4x4.CreateRotationX(j.RotateX); | |
| var rotationMatrixY = Matrix4x4.CreateRotationY(j.RotateY); | |
| var rotationMatrixZ = Matrix4x4.CreateRotationZ(j.RotateZ); | |
| var rotationMatrix = rotationMatrixX * rotationMatrixY * rotationMatrixZ; | |
| var translationMatrix = Matrix4x4.CreateTranslation(new Vector3(j.TranslateX, j.TranslateY, j.TranslateZ)); | |
| dict[i] = scaleMatrix * rotationMatrix * translationMatrix; | |
| } | |
| return dict; | |
| } | |
| for (var i = 0u; i < joints.Count; i++) | |
| { | |
| var j = joints[(int)i]; | |
| var scaleMatrix = Matrix4x4.CreateScale(new Vector3(j.ScaleX, j.ScaleY, j.ScaleZ)); | |
| var rotationMatrixX = Matrix4x4.CreateRotationX(j.RotateX); | |
| var rotationMatrixY = Matrix4x4.CreateRotationY(j.RotateY); | |
| var rotationMatrixZ = Matrix4x4.CreateRotationZ(j.RotateZ); | |
| var rotationMatrix = rotationMatrixX * rotationMatrixY * rotationMatrixZ; | |
| var translationMatrix = Matrix4x4.CreateTranslation(new Vector3(j.TranslateX, j.TranslateY, j.TranslateZ)); | |
| var key = j.Index; // authoritative joint id | |
| dict[key] = scaleMatrix * rotationMatrix * translationMatrix; | |
| } | |
| return dict; |
🤖 Prompt for AI Agents
In OpenKh.Kh1/Cvbl.cs around lines 188 to 204 the dictionary is populated using
the loop variable i as the key which ignores the joint's declared Index and will
mis-map bones when joints are not ordered by Index; change the dictionary key to
use j.Index (properly cast to the dictionary key type) when assigning the
composed transform so entries align with the joint's actual index, and add a
check/assert to detect duplicate j.Index values (or throw) to ensure uniqueness.
| case 1: | ||
| mesh.Faces.Add([totalVertCount + i - 2, totalVertCount + i - 1, totalVertCount + i]); | ||
| break; | ||
| case 2: | ||
| mesh.Faces.Add([totalVertCount + i, totalVertCount + i - 1, totalVertCount + i - 2]); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Triangle index underflow on early vertices.
totalVertCount + i - 1/2 underflows for i < 2 (unsigned math), producing huge indices if Face is set.
- switch (vert.Face)
+ if (i >= 2)
{
- case 1:
- mesh.Faces.Add([totalVertCount + i - 2, totalVertCount + i - 1, totalVertCount + i]);
- break;
- case 2:
- mesh.Faces.Add([totalVertCount + i, totalVertCount + i - 1, totalVertCount + i - 2]);
- break;
+ switch (vert.Face)
+ {
+ case 1:
+ mesh.Faces.Add(new uint[] { totalVertCount + i - 2, totalVertCount + i - 1, totalVertCount + i });
+ break;
+ case 2:
+ mesh.Faces.Add(new uint[] { totalVertCount + i, totalVertCount + i - 1, totalVertCount + i - 2 });
+ 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.
| case 1: | |
| mesh.Faces.Add([totalVertCount + i - 2, totalVertCount + i - 1, totalVertCount + i]); | |
| break; | |
| case 2: | |
| mesh.Faces.Add([totalVertCount + i, totalVertCount + i - 1, totalVertCount + i - 2]); | |
| break; | |
| } | |
| // Only add faces once we have at least two previous vertices to index into | |
| if (i >= 2) | |
| { | |
| switch (vert.Face) | |
| { | |
| case 1: | |
| mesh.Faces.Add(new uint[] { | |
| totalVertCount + i - 2, | |
| totalVertCount + i - 1, | |
| totalVertCount + i | |
| }); | |
| break; | |
| case 2: | |
| mesh.Faces.Add(new uint[] { | |
| totalVertCount + i, | |
| totalVertCount + i - 1, | |
| totalVertCount + i - 2 | |
| }); | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In OpenKh.Kh1/Cvbl.cs around lines 289 to 295 the face index arithmetic uses
unsigned values so expressions like totalVertCount + i - 1 and - 2 will
underflow for i < 2; change the logic to ensure subtraction happens on a signed
integer or guard the branch by only executing when i >= 2 (or start the loop at
i = 2) so you never compute negative offsets, and construct the triangle indices
from validated/signed values before adding them to mesh.Faces.
| else if (subsectionType == 17) | ||
| { | ||
| var jointId = str.ReadUInt32(); | ||
| var jointSlotId = str.ReadUInt32(); | ||
|
|
||
| jointSlots[jointSlotId] = (jointId, jointSlots[jointSlotId].Item2); | ||
|
|
||
| switch (jointStyle) | ||
| { | ||
| case 8: | ||
| { | ||
| var data = new List<uint>(); | ||
| for (var i = 0; i < 28; i++) data.Add(str.ReadUInt32()); | ||
| extraJointData[jointSlotId] = data; | ||
|
|
||
| break; | ||
| } | ||
| case 9: | ||
| { | ||
| var data = new List<uint>(); | ||
| for (var i = 0; i < 28; i++) data.Add(str.ReadUInt32()); | ||
| extraJointData[jointSlotId] = data; | ||
|
|
||
| jointSlots[jointSlotId] = (jointId, new Matrix4x4( | ||
| str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), | ||
| str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), | ||
| str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), | ||
| str.ReadSingle(), str.ReadSingle(), str.ReadSingle(), str.ReadSingle() | ||
| )); | ||
| break; | ||
| } | ||
| case 10: | ||
| { | ||
| var data = new List<uint>(); | ||
| for (var i = 0; i < 16; i++) data.Add(str.ReadUInt32()); | ||
| extraJointData[jointSlotId] = data; | ||
| break; | ||
| } | ||
| default: | ||
| Console.WriteLine($"Warning: Unknown joint style: {jointStyle} @ {subsectionDataOffset}"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Subsection 17 parsing: length not validated.
Reads fixed counts (28/16 dwords and optionally a 4x4 matrix) without checking subsectionLength. Risk of overreads on malformed files.
Add checks to ensure reads do not exceed subsectionLength - 8 and str.Length.
🤖 Prompt for AI Agents
In OpenKh.Kh1/Cvbl.cs around lines 300 to 342, the Subsection 17 parser blindly
reads fixed counts of dwords (28 or 16) and an optional 4x4 matrix without
validating subsectionLength or remaining stream length; update the code to first
compute availableBytes = subsectionLength - 8 (accounting for the two UInt32s
already read) and ensure availableBytes and str.Length - str.Position are
sufficient before each read, returning or throwing a controlled error (or
skipping the subsection) on insufficient bytes; specifically, check that
availableBytes >= (count * 4) before reading the array and >= ((count * 4) +
16*4) before reading the matrix, and also verify str.Position + requiredBytes <=
str.Length to avoid stream overreads.
3 participants
This is a pull request going to merge changes in #1199.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores