Conversation
coconutbird
commented
Jan 31, 2026
coconutbird
commented
- Fix project file relative pathing
- Support direct .bnk editing
- More
There was a problem hiding this comment.
Pull request overview
This PR introduces a significant architectural refactoring to support direct BNK file editing alongside PCK package files. It implements a unified IAudioFile interface that abstracts over both file types, enabling commands and batch operations to work seamlessly with either format.
Changes:
- Introduced
IAudioFileandIAudioFileFactoryabstractions to unify PCK and BNK file handling - Created
BnkFilewrapper class enabling standalone BNK files to be edited directly - Refactored batch project system with improved relative path handling (input files relative to game directory, action source files relative to project file)
- Updated all CLI commands to use the new unified audio file factory
- Added comprehensive test coverage for new BNK file functionality
- Added Native AOT/trimming support attributes (
DynamicallyAccessedMembers) - Changed batch project file extension from
.batchprojto.json - Added GitHub Actions CI/CD workflows
- Enabled NuGet package lock files for reproducible builds
- Updated .NET SDK version to 10.0.100
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| PckTool.Abstractions/IAudioFile.cs | New unified interface for PCK and BNK files |
| PckTool.Abstractions/IAudioFileFactory.cs | New factory interface for creating audio file instances |
| PckTool.Core/WWise/AudioFileFactory.cs | Implementation of audio file factory with automatic type detection |
| PckTool.Core/WWise/Bnk/BnkFile.cs | New wrapper class for standalone BNK files implementing IAudioFile |
| PckTool.Core/WWise/Bnk/Collections/* | Moved collection classes to dedicated Collections namespace |
| PckTool.Core/WWise/Bnk/Entries/BnkFileSoundBankEntry.cs | Entry wrapper for BNK soundbank in standalone files |
| PckTool.Core/WWise/Pck/PckFile.cs | Updated to implement IAudioFile alongside existing interfaces |
| PckTool.Core/Services/Batch/BatchProjectExecutor.cs | Refactored to use IAudioFileFactory and improved path resolution |
| PckTool.Core/Services/Batch/BatchProject.cs | Enhanced validation with separate game directory handling |
| PckTool.Core/Services/PackageBrowser.cs | Updated to use IAudioFileFactory for unified file loading |
| PckTool/Services/ServiceProvider.cs | Replaced PckFileFactory with AudioFileFactory |
| PckTool/Commands/*.cs | All commands updated to use IAudioFileFactory and added AOT attributes |
| PckTool/Program.cs | Added comprehensive command examples and aliases |
| tests/PckTool.Core.Tests/BnkFileTests.cs | New comprehensive tests for BNK file operations |
| tests/PckTool.Core.Tests/SoundBankTests.cs | Tests moved from other files and organized |
| tests/PckTool.Core.Tests/PckFileTests.cs | PCK-specific tests reorganized |
| global.json | SDK version updated to 10.0.100 |
| .github/workflows/* | New CI/CD workflows for build, test, and release |
| Directory.Build.props | Enabled package lock files globally |
| **/packages.lock.json | New lock files for reproducible builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void ReplaceWith(byte[] data) | ||
| { | ||
| // For standalone BNK, we need to re-parse the data into the soundbank | ||
| // This is a bit unusual but maintains consistency with the interface | ||
| var newBank = SoundBank.Parse(data); | ||
|
|
||
| if (newBank is null) | ||
| { | ||
| throw new InvalidDataException("Failed to parse replacement BNK data."); | ||
| } | ||
|
|
||
| // Copy properties from new bank to existing bank | ||
| // Note: This is a simplified approach - in practice you might want to | ||
| // replace the entire soundbank reference | ||
| _soundBank.Media.Clear(); | ||
|
|
||
| foreach (var kvp in newBank.Media) | ||
| { | ||
| _soundBank.Media.Add(kvp.Key, kvp.Value); | ||
| } | ||
|
|
||
| _soundBank.Items.Clear(); | ||
| _soundBank.Items.AddRange(newBank.Items); | ||
|
|
||
| _parent.MarkModified(); |
There was a problem hiding this comment.
The ReplaceWith method only copies Media and Items from the new bank but doesn't copy other important properties like Id, LanguageId, Version, ProjectId, and FeedbackInBank. This could lead to inconsistencies where the soundbank's metadata doesn't match its actual content.
Consider copying all properties from newBank to _soundBank, or document why only Media and Items should be replaced while preserving the original metadata.
There was a problem hiding this comment.
This is intentional. We're replacing the soundbank's content (Media and Items), not its identity. The metadata (Id, LanguageId, etc.) should remain unchanged so the soundbank stays in the same slot within the parent file. Added a clarifying comment to the code.
| /// <inheritdoc /> | ||
| public uint Size => (uint) _soundBank.ToByteArray().Length; |
There was a problem hiding this comment.
The Size property calls ToByteArray() on every access, which serializes the entire soundbank. This could be a performance issue if Size is accessed frequently (e.g., in loops or UI binding).
Consider caching the size value and invalidating it when the soundbank is modified, or computing the size differently without full serialization.
There was a problem hiding this comment.
For a CLI tool, Size is typically accessed once per entry during listing/display operations, not in tight loops. The overhead is negligible in this context. If this becomes a hot path in the future, we can add caching then.
| { | ||
| // Dispose all loaded files | ||
| foreach (var pck in loadedFiles.Values) | ||
| foreach (var audioFile in loadedFiles.Values) |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
There was a problem hiding this comment.
The dictionary is built up incrementally during execution, so a using statement isn't practical here. The try/finally pattern is appropriate for this use case where we're managing a collection of disposables that are created at different points in the method.
| { | ||
| // Arrange | ||
| var tempPath = Path.GetTempFileName(); | ||
| PckFile? loaded = null; |
There was a problem hiding this comment.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
There was a problem hiding this comment.
This is test code cleaning up a temp file. The try/finally is clear and appropriate here - using a using statement for a file path string wouldn't make sense, and wrapping the temp file in a disposable helper would be overkill for a test.
