Fixes Texture count bug, adds optional skeleton replace option and re… #1242
Conversation
📝 WalkthroughWalkthroughThis pull request extends the KH2 MDLX Editor with three main features: optional collision bone resetting during model import, user control over skeleton retention via UI checkbox, and individual texture replacement capability. Changes span import logic, view models, UI bindings, and stream handling during MDLX file building. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.csOpenKh.Tools.Kh2MdlxEditor/ViewModels/Importer_VM.csOpenKh.Tools.Kh2MdlxEditor/ViewModels/Main2_VM.csOpenKh.Tools.Kh2MdlxEditor/ViewModels/TextureFile_VM.csOpenKh.Tools.Kh2MdlxEditor/Views/Importer_Window.xamlOpenKh.Tools.Kh2MdlxEditor/Views/Importer_Window.xaml.csOpenKh.Tools.Kh2MdlxEditor/Views/Main2_Window.xaml.csOpenKh.Tools.Kh2MdlxEditor/Views/TextureFile_Control.xamlOpenKh.Tools.Kh2MdlxEditor/Views/TextureFile_Control.xaml.cs
🧰 Additional context used
🧬 Code graph analysis (7)
OpenKh.Tools.Kh2MdlxEditor/ViewModels/Importer_VM.cs (1)
OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.cs (1)
MdlxEditorImporter(13-307)
OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.cs (4)
OpenKh.AssimpUtils/Kh2MdlxAssimp.cs (7)
Dictionary(137-149)Dictionary(514-534)Assimp(16-135)Assimp(151-272)Assimp(496-510)VifMesh(274-368)Kh2MdlxAssimp(11-535)OpenKh.Kh2/Models/ModelCommon.cs (4)
List(237-251)Matrix4x4(253-274)ModelCommon(10-332)Bone(125-169)OpenKh.Kh2/Models/VIF/VifUtils.cs (2)
VifUtils(5-120)calcBaseAddress(102-119)OpenKh.Kh2/Models/VIF/VifMesh.cs (2)
VifMesh(11-20)VifMesh(16-19)
OpenKh.Tools.Kh2MdlxEditor/ViewModels/Main2_VM.cs (1)
OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.cs (1)
MdlxEditorImporter(13-307)
OpenKh.Tools.Kh2MdlxEditor/ViewModels/TextureFile_VM.cs (2)
OpenKh.Kh2/ModelTexture.cs (13)
ModelTexture(14-977)ModelTexture(416-479)ModelTexture(557-561)ModelTexture(563-694)ModelTexture(932-933)Texture(29-90)Texture(37-50)PixelFormat(873-884)GetData(71-89)GetClut(58-69)Write(320-334)Write(710-823)Build(481-488)OpenKh.Tools.Kh2MdlxEditor/Utils/ImageUtils.cs (5)
ModelTexture(41-52)ModelTexture(54-57)ImageUtils(15-137)Imgd(17-39)Imgd(65-108)
OpenKh.Tools.Kh2MdlxEditor/Views/Main2_Window.xaml.cs (1)
OpenKh.Tools.Kh2MdlxEditor/Views/TextureFile_Control.xaml.cs (4)
TextureFile_Control(19-190)TextureFile_Control(25-28)TextureFile_Control(30-35)TextureFile_Control(38-51)
OpenKh.Tools.Kh2MdlxEditor/Views/Importer_Window.xaml.cs (1)
OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.cs (1)
MdlxEditorImporter(13-307)
OpenKh.Tools.Kh2MdlxEditor/Views/TextureFile_Control.xaml.cs (3)
OpenKh.Tools.Kh2MdlxEditor/ViewModels/Main2_VM.cs (3)
Main2_VM(10-120)Main2_VM(25-25)Main2_VM(26-29)OpenKh.Kh2/ModelTexture.cs (5)
ModelTexture(14-977)ModelTexture(416-479)ModelTexture(557-561)ModelTexture(563-694)ModelTexture(932-933)OpenKh.Tools.Kh2MdlxEditor/ViewModels/TextureFile_VM.cs (4)
TextureFile_VM(11-109)TextureFile_VM(18-18)TextureFile_VM(19-24)replaceTextureAt(39-108)
⏰ 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 (22)
OpenKh.Tools.Kh2MdlxEditor/Views/TextureFile_Control.xaml (1)
43-43: LGTM! Context menu item properly wired.The new "Replace Texture" menu item follows the established pattern and correctly binds to the Texture_Replace handler implemented in the code-behind.
OpenKh.Tools.Kh2MdlxEditor/Views/Importer_Window.xaml.cs (2)
1-1: LGTM! Required namespace import.The
OpenKh.Tools.Kh2MdlxEditor.Utilsnamespace is needed to accessMdlxEditorImporterstatic flags.
76-78: LGTM! Follows established pattern for import configuration.The static flag pattern is consistent with the existing
TRIANGLE_INVERSEflag inMdlxEditorImporter. The flags are set immediately before use, minimizing risk.OpenKh.Tools.Kh2MdlxEditor/Views/Importer_Window.xaml (1)
20-23: LGTM! UI correctly exposes both import options.The horizontal StackPanel layout with proper spacing and TwoWay binding aligns with the new
KeepSkeletonfeature. Both options are now accessible to users.OpenKh.Tools.Kh2MdlxEditor/ViewModels/Importer_VM.cs (3)
31-31: LGTM! New property follows established pattern.The
KeepSkeletonproperty mirrors the existingKeepShadowproperty structure and provides user control over skeleton retention during import.
42-42: LGTM! Safe default value.Defaulting to
truepreserves the original skeleton by default, which is the safer option and prevents unexpected behavior changes for existing workflows.
120-120: LGTM! Properly propagates flag to importer.The
KeepSkeletonvalue is correctly assigned to the staticKEEP_ORIGINAL_SKELETONflag before model import, ensuring the user's choice is respected.OpenKh.Tools.Kh2MdlxEditor/Views/Main2_Window.xaml.cs (1)
111-121: LGTM! Clean refactoring with proper null guards.The extracted
LoadTextureControlhelper method:
- Adds appropriate null checks before creating the control
- Passes
mainVMto enable texture replacement integration- Uses a self-referential callback pattern to enable UI refresh after texture replacement
This design cleanly supports the new texture replacement feature while maintaining good separation of concerns.
OpenKh.Tools.Kh2MdlxEditor/ViewModels/Main2_VM.cs (2)
90-90: LGTM! Critical fix for stream positioning.Resetting the stream position to 0 after writing is essential to ensure subsequent reads (e.g., by
Bar.Writein line 170 of Main2_Window.xaml.cs) can successfully read the data from the beginning of the stream. Without these resets, the streams would be positioned at EOF, causing empty or corrupted data to be written to the output file.Also applies to: 95-95, 99-99
117-118: LGTM! Enables collision bone reset during skeleton import.Passing
CollisionFiletoreplaceMeshModelSkeletalallows the importer to reset collision bone references when a new skeleton is imported, preventing crashes from invalid bone indices as described in the PR objectives.OpenKh.Tools.Kh2MdlxEditor/ViewModels/TextureFile_VM.cs (3)
3-7: LGTM! Required using directives.The additional using directives support the new texture replacement functionality.
13-13: LGTM! Callback pattern enables external notification.The callback mechanism allows the parent
Main2_VMto be notified when a texture is replaced, ensuring the view model state stays synchronized.Also applies to: 19-19, 23-23
38-108: LGTM! Comprehensive texture replacement implementation.The
replaceTextureAtmethod:
- Validates the index range (lines 41-44)
- Rebuilds the entire texture list to ensure consistency (lines 50-71)
- Correctly preserves footer data (lines 74-82)
- Uses a write/read cycle to normalize the
ModelTextureformat (lines 94-97)- Notifies the parent via callback (line 100)
- Updates the observable collection for UI refresh (lines 103-107)
The full rebuild approach, while not minimal, ensures proper formatting and consistency across all textures. The
swizzled: falseparameter on line 67 is correct since textures fromModelTexture.Imagesare already in unswizzled format.OpenKh.Tools.Kh2MdlxEditor/Views/TextureFile_Control.xaml.cs (4)
22-23: LGTM! Fields support the new texture replacement feature.The private fields enable integration with
Main2_VMand support external refresh callbacks after texture replacement.
37-51: LGTM! Constructor cleanly integrates with parent VM.The new constructor:
- Accepts
Main2_VMand an optional refresh callback- Wires a callback to update
Main2_VM.TextureFilewhen a texture is replaced (lines 43-48)- Enables bidirectional synchronization between the control and the parent view model
55-55: LGTM! Forces WPF data binding refresh.Setting
DataContext = nullbefore reassigning forces WPF to recognize the change and refresh all bindings, ensuring the UI reflects the updated texture data.
109-141: LGTM! Well-structured texture replacement handler.The
Texture_Replacemethod:
- Guards against no selection (line 112)
- Filters to PNG files only (line 119)
- Properly handles exceptions with detailed error messages (lines 134-138)
- Invokes the refresh callback to update the UI (line 129)
- Provides user feedback with success/error MessageBox dialogs
The implementation is robust and user-friendly.
OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.cs (5)
1-9: LGTM! Necessary import for LINQ functionality.The addition of
System.Linqis required for the.Distinct()method used on line 79 to fix the texture count bug.
69-69: LGTM! Backward-compatible signature change.The optional
collisionFileparameter enables the collision bone reset feature when importing a new skeleton, while maintaining backward compatibility for existing callers.
79-79: LGTM! Correct fix for the texture count bug.The calculation properly counts distinct texture indices from the
materialToTexturedictionary, which correctly handles cases where multiple materials share the same texture. The null-coalescing operator provides a safe fallback.
89-99: LGTM! Proper collision bone reset logic.The logic correctly resets collision bones when importing a new skeleton, preventing crashes from invalid bone index references. The conditional ensures this only executes when a new skeleton is imported and collision data is present.
139-149: LGTM! Well-implemented helper method.The method properly handles null checks and clearly implements the collision bone reset functionality. The comment accurately describes its purpose of preventing crashes from invalid bone indices.
| List<DmaVifPacket> dmaVifPackets = VifProcessor.vifMeshToDmaVifPackets(vifMesh); | ||
|
|
||
| ModelSkeletal.SkeletalGroup group = VifProcessor.getSkeletalGroup(dmaVifPackets, (uint)mesh.MaterialIndex, baseAddress); | ||
| group.Header.TextureIndex = (uint)materialToTexture[mesh.MaterialIndex]; |
There was a problem hiding this comment.
Consider adding null check for consistency.
Line 79 uses a null-conditional operator for materialToTexture, but this line directly accesses the dictionary without null checking. If materialToTexture is null (e.g., if createModelTexture wasn't called first), this will throw a NullReferenceException.
🔎 Suggested fix for consistency
- group.Header.TextureIndex = (uint)materialToTexture[mesh.MaterialIndex];
+ group.Header.TextureIndex = (uint)(materialToTexture?[mesh.MaterialIndex] ?? mesh.MaterialIndex);Or add an explicit null check at the start of the method if materialToTexture being populated is a precondition.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In OpenKh.Tools.Kh2MdlxEditor/Utils/MdlxEditorImporter.cs around line 115, the
code accesses materialToTexture directly which can be null (causing
NullReferenceException); either add an explicit null check at the start of the
method and throw a clear ArgumentNullException if materialToTexture is required,
or guard this access by using the null-conditional operator and a safe fallback
(e.g., set TextureIndex to a default value) so the code does not dereference a
null dictionary.
2 participants
Fixes Texture count bug.
adds optional skeleton replace option.
Adds replace Texture option.
If Skeleton is imported, reset Bones to 0, otherwise crash if trying to display collisions on invalid bone indicates.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.