Add strip path prefix while grouping by container, and add filter exclude mode#113
Add strip path prefix while grouping by container, and add filter exclude mode#113MejiroRina wants to merge 7 commits intoaelurum:AssetStudioModfrom
Conversation
- Added --filter-blacklist-mode flag
- Added --strip-path-prefix flag
sounds better
2443e0a to
0a763c5
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds two new features to AssetStudioCLI: a filter exclude mode that inverts filter logic to exclude matching assets rather than include them, and a strip path prefix option for cleaner export paths when grouping assets by container path.
Changes:
- Added
--filter-exclude-modeflag to invert filter behavior, excluding assets that match filter criteria instead of including them - Added
--strip-path-prefixoption to remove a specified prefix from container paths during export when using ContainerPath/ContainerPathFull grouping - Updated documentation in ReadMe.md and option descriptions in CLIOptions.cs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| AssetStudioCLI/Studio.cs | Implements filter exclusion logic and path prefix stripping with validation |
| AssetStudioCLI/Options/CLIOptions.cs | Adds new option declarations, initialization, parsing, and display logic for both features |
| AssetStudioCLI/ReadMe.md | Documents the new --filter-exclude-mode and --strip-path-prefix options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var toExportAssetDict = new ConcurrentDictionary<AssetItem, string>(); | ||
| var toParallelExportAssetDict = new ConcurrentDictionary<AssetItem, string>(); | ||
|
|
||
| if (CLIOptions.o_stripPathPrefix.Value != null) |
There was a problem hiding this comment.
The null check pattern here is inconsistent with the rest of the codebase. The default value for o_stripPathPrefix is an empty string (line 540 in CLIOptions.cs), so this will never be null unless explicitly set. Throughout the codebase, string options are checked with equality to empty string (e.g., o_assemblyPath.Value == "" at line 1280). Consider using !string.IsNullOrEmpty(CLIOptions.o_stripPathPrefix.Value) for consistency, or change the default value to null if that's the intended pattern.
| { | ||
| exportPath = Path.Combine(savePath, Path.GetDirectoryName(asset.Container)); | ||
| var containerPath = Path.GetDirectoryName(asset.Container); | ||
| if (CLIOptions.o_stripPathPrefix.Value != null) |
There was a problem hiding this comment.
Same null check pattern issue as lines 663 and 682. This should use !string.IsNullOrEmpty() for consistency with the rest of the codebase.
| { | ||
| continue; | ||
| } | ||
| if (!Path.Combine(Path.GetDirectoryName(containerPath), Path.GetFileName(containerPath)).StartsWith(CLIOptions.o_stripPathPrefix.Value)) |
There was a problem hiding this comment.
The validation logic using Path.Combine(Path.GetDirectoryName(containerPath), Path.GetFileName(containerPath)) is redundant and potentially problematic. This operation reconstructs the original path but may not handle edge cases correctly (e.g., paths with trailing slashes, root paths, or paths that are just filenames without directories). Consider using containerPath directly for the StartsWith check, as it's simpler and more reliable.
| if (!Path.Combine(Path.GetDirectoryName(containerPath), Path.GetFileName(containerPath)).StartsWith(CLIOptions.o_stripPathPrefix.Value)) | |
| if (!containerPath.StartsWith(CLIOptions.o_stripPathPrefix.Value)) |
AssetStudioCLI/Studio.cs
Outdated
| var excludeCount = assetsCount - filteredAssets.Count; | ||
| Logger.Info($"Excluding {excludeCount} asset(s) from export."); |
There was a problem hiding this comment.
The exclude count calculation is incorrect. The variable excludeCount is calculated as assetsCount - filteredAssets.Count, which represents the number of assets that DON'T match the filter (i.e., assets that will remain). However, when f_filterExcludeMode is enabled, filteredAssets gets inverted to exclude matching assets, so the actual number excluded is filteredAssets.Count (before the inversion). The message should say "Excluding {filteredAssets.Count} asset(s) that match the filter" to accurately reflect what's being excluded.
| var excludeCount = assetsCount - filteredAssets.Count; | |
| Logger.Info($"Excluding {excludeCount} asset(s) from export."); | |
| var excludeCount = filteredAssets.Count; | |
| Logger.Info($"Excluding {excludeCount} asset(s) that match the filter."); |
| { | ||
| exportPath = Path.Combine(savePath, Path.GetDirectoryName(asset.Container)); | ||
| var containerPath = Path.GetDirectoryName(asset.Container); | ||
| if (CLIOptions.o_stripPathPrefix.Value != null) |
There was a problem hiding this comment.
This Substring operation could throw an ArgumentOutOfRangeException if containerPath is shorter than the prefix length. While the validation loop above attempts to prevent this, it uses a different logic (Path.Combine with GetDirectoryName/GetFileName) that may not guarantee the same result. Add a length check here: if (containerPath.Length >= CLIOptions.o_stripPathPrefix.Value.Length) before calling Substring, or ensure the validation logic matches exactly.
| if (CLIOptions.o_stripPathPrefix.Value != null) | |
| if (CLIOptions.o_stripPathPrefix.Value != null && | |
| !string.IsNullOrEmpty(containerPath) && | |
| containerPath.Length >= CLIOptions.o_stripPathPrefix.Value.Length) |
| } | ||
| } | ||
|
|
||
| if (CLIOptions.o_stripPathPrefix.Value != null) |
There was a problem hiding this comment.
Same null check pattern issue as line 663. This should use !string.IsNullOrEmpty() for consistency with the rest of the codebase, or the default value should be changed to null.
AssetStudioCLI/Options/CLIOptions.cs
Outdated
| ( | ||
| optionDefaultValue: false, | ||
| optionName: "--filter-exclude-mode", | ||
| optionDescription: "(Flag) If specified, the filter options will work as a exclude\n" + |
There was a problem hiding this comment.
The description says "work as a exclude" which should be "work as an exclusion" (article "an" before vowel sound). This wording inconsistency also exists in the ReadMe.md where it correctly says "work as a exclusion" but should be "work as an exclusion".
| optionDescription: "(Flag) If specified, the filter options will work as a exclude\n" + | |
| optionDescription: "(Flag) If specified, the filter options will work as an exclusion\n" + |
AssetStudioCLI/ReadMe.md
Outdated
| --filter-with-regex (Flag) If specified, the filter options will handle the specified text | ||
| as a regular expression (doesn't apply to --filter-by-pathid) | ||
|
|
||
| --filter-exclude-mode (Flag) If specified, the filter options will work as a exclusion |
There was a problem hiding this comment.
The description says "work as a exclusion" which should be "work as an exclusion" (article "an" before vowel sound).
| --filter-exclude-mode (Flag) If specified, the filter options will work as a exclusion | |
| --filter-exclude-mode (Flag) If specified, the filter options will work as an exclusion |
| } | ||
| break; | ||
| case "--strip-path-prefix": | ||
| o_stripPathPrefix.Value = Path.Combine(Path.GetDirectoryName(value), Path.GetFileName(value)); |
There was a problem hiding this comment.
Using Path.Combine(Path.GetDirectoryName(value), Path.GetFileName(value)) to normalize the path is problematic. This approach may fail for edge cases: (1) If value is a root path like "C:", GetDirectoryName returns null; (2) If value has a trailing separator, it will be removed; (3) If value is just a filename with no directory, GetDirectoryName returns empty string and Combine will just return the filename. Consider using Path.GetFullPath(value) for proper path normalization, or simply use the value as-is and let the validation in Studio.cs handle the format checking.
| o_stripPathPrefix.Value = Path.Combine(Path.GetDirectoryName(value), Path.GetFileName(value)); | |
| o_stripPathPrefix.Value = Path.GetFullPath(value); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
48bd5bd to
3d5fb9d
Compare
No description provided.