feat: exclude specific textures from compression by GUID #96
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the ability to exclude specific textures from compression by selecting them in the inspector (internally compared via asset GUID), and surfaces the new skip reason throughout preview + tests.
Changes:
- Add
ExcludedTextureslist toTextureCompressorand inspector UI for managing it. - Pass excluded texture GUIDs into
TextureCollectorand skip matching textures with a newSkipReason.ExcludedTexture. - Extend preview/settings hashing and add/expand unit tests for excluded-texture behavior and GUID conversion.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/Editor/UI/PreviewGeneratorTests.cs | Adds settings-hash coverage for excluded texture list changes. |
| Tests/Editor/Core/Services/TextureCollectorTests.cs | Adds constructor/collection behavior tests for excluded texture GUIDs + GUID conversion helper tests. |
| Tests/Editor/Core/Models/TextureInfoTests.cs | Updates enum distinctness tests for the new skip reason. |
| Runtime/Components/TextureCompressor.cs | Introduces serialized ExcludedTextures list on the config component. |
| Editor/TextureCompressor/UI/Sections/FilterSection.cs | Adds inspector foldout UI to edit excluded textures list. |
| Editor/TextureCompressor/UI/Preview/PreviewSection.cs | Displays user-facing skip reason text for excluded textures. |
| Editor/TextureCompressor/UI/Preview/PreviewGenerator.cs | Converts excluded texture refs to GUIDs and includes them in collection + settings hash. |
| Editor/TextureCompressor/TextureCompressorEditor.cs | Wires the new excluded-textures UI section into the inspector. |
| Editor/TextureCompressor/Core/Services/TextureCompressorService.cs | Passes excluded texture GUIDs into TextureCollector for build/compress flow. |
| Editor/TextureCompressor/Core/Services/TextureCollector.cs | Adds excluded GUID set, constructor parameter, skip logic, and ToAssetGuids helper. |
| Editor/TextureCompressor/Core/Models/TextureInfo.cs | Adds SkipReason.ExcludedTexture. |
| CHANGELOG.md | Documents the new excluded textures list feature under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughAdds an inspector-driven exclusion list for textures: runtime config field for excluded textures, new SkipReason.ExcludedTexture, GUID-based exclusion in TextureCollector.EvaluateProcessability, editor UI and helper for managing exclusions, Preview hash updates to include excluded textures, and corresponding tests. Changes
Sequence DiagramsequenceDiagram
participant User as User/Inspector
participant Editor as TextureCompressorEditor
participant Filter as FilterSection
participant Config as TextureCompressor (Config)
participant Service as TextureCompressorService / PreviewGenerator
participant Collector as TextureCollector
participant Eval as EvaluateProcessability
User->>Editor: Open inspector / edit exclusions
Editor->>Filter: DrawExclusions(config, ref showSection)
Filter->>User: Render excluded textures (ObjectField) and paths
User->>Filter: Add/Remove/Modify Texture2D entry
Filter->>Config: Undo.RecordObject + update ExcludedTextures
Config->>Service: Start preview/compression
Service->>Collector: new TextureCollector(..., excludedTextures, frozenTextures)
Collector->>Eval: EvaluateProcessability(texture)
Eval->>Eval: resolve texture asset GUID
Eval-->>Collector: if GUID in excluded set => SkipReason.ExcludedTexture
Collector-->>Service: return textures marked/skipped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…r merge" This reverts commit 918a7ea.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/Editor/Core/Models/TextureInfoTests.cs (1)
167-181:⚠️ Potential issue | 🟡 MinorTest is missing
UnknownUncompressedPropertyenum value.The
SkipReason_AllValues_AreDistincttest doesn't include all enum members.UnknownUncompressedPropertyexists in the enum (line 29 inTextureInfo.cs) but is missing from this test array.🧪 Proposed fix to include all enum values
var values = new[] { SkipReason.None, SkipReason.TooSmall, SkipReason.FilteredByType, SkipReason.FrozenSkip, SkipReason.RuntimeGenerated, SkipReason.ExcludedPath, SkipReason.ExcludedTexture, + SkipReason.UnknownUncompressedProperty, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Editor/Core/Models/TextureInfoTests.cs` around lines 167 - 181, The SkipReason_AllValues_AreDistinct test is missing the enum member UnknownUncompressedProperty; update the values array in TextureInfoTests.SkipReason_AllValues_AreDistinct to include SkipReason.UnknownUncompressedProperty so the HashSet uniqueness assertion covers all members of the SkipReason enum (defined in TextureInfo.cs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Tests/Editor/Core/Models/TextureInfoTests.cs`:
- Around line 167-181: The SkipReason_AllValues_AreDistinct test is missing the
enum member UnknownUncompressedProperty; update the values array in
TextureInfoTests.SkipReason_AllValues_AreDistinct to include
SkipReason.UnknownUncompressedProperty so the HashSet uniqueness assertion
covers all members of the SkipReason enum (defined in TextureInfo.cs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81ac39b9-caaf-4e59-9f94-50c213a42a6f
📒 Files selected for processing (12)
CHANGELOG.mdEditor/TextureCompressor/Core/Models/TextureInfo.csEditor/TextureCompressor/Core/Services/TextureCollector.csEditor/TextureCompressor/Core/Services/TextureCompressorService.csEditor/TextureCompressor/TextureCompressorEditor.csEditor/TextureCompressor/UI/Preview/PreviewGenerator.csEditor/TextureCompressor/UI/Preview/PreviewSection.csEditor/TextureCompressor/UI/Sections/FilterSection.csRuntime/Components/TextureCompressor.csTests/Editor/Core/Models/TextureInfoTests.csTests/Editor/Core/Services/TextureCollectorTests.csTests/Editor/UI/PreviewGeneratorTests.cs
…ollector test calls
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Tests/Editor/Core/Services/TextureCollectorTests.cs (1)
1010-1126: Consider adding one integration test for replaced textures + excluded GUID.You already validate ObjectRegistry replacement behavior for
ExcludedPath/FrozenSkiplater; adding the same scenario for excluded GUID would close a likely regression gap for this feature path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Editor/Core/Services/TextureCollectorTests.cs` around lines 1010 - 1126, Add an integration test that verifies ObjectRegistry replacement is honored when a texture's GUID is in the excluded GUID list: create an original texture and a replacement texture, obtain the original's GUID, construct a TextureCollector with that GUID in its excluded-guids parameter, register the replacement via the ObjectRegistry replacement API for the original texture, assign the original to a Material, call TextureCollector.CollectFromMaterials (collectAll: true), and assert the collected dictionary contains the replacement Texture2D (not the original), that the replacement entry IsProcessed is false, and its SkipReason equals SkipReason.ExcludedTexture; reference TextureCollector, CollectFromMaterials, ObjectRegistry (the register-replacement method used elsewhere in tests), and SkipReason to locate the correct APIs.Editor/Common/UI/Controls/ExclusionListDrawer.cs (1)
27-38: Consider adding null guards for required parameters.The
undoTargetandlistparameters are critical for the method's operation. Passingnullforlistwould cause aNullReferenceExceptionat line 40 or 49, andnullforundoTargetwould cause issues withUndo.RecordObject. Adding early validation would provide clearer error messages.🛡️ Optional: Add parameter validation
) { + if (undoTarget == null) + throw new ArgumentNullException(nameof(undoTarget)); + if (list == null) + throw new ArgumentNullException(nameof(list)); + int count = list.Count;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Editor/Common/UI/Controls/ExclusionListDrawer.cs` around lines 27 - 38, The Draw<T> method should validate required inputs to avoid NullReferenceException and Undo.RecordObject failures: at the top of Draw, check that 'list' and 'undoTarget' are not null and throw ArgumentNullException (or ArgumentNullException(nameof(list)) / ArgumentNullException(nameof(undoTarget))) with clear messages if they are null; keep the rest of the logic unchanged so callers get a fast, descriptive failure rather than a later NRE when Draw uses 'list' or calls Undo.RecordObject on 'undoTarget'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Editor/Common/UI/Controls/ExclusionListDrawer.cs`:
- Around line 27-38: The Draw<T> method should validate required inputs to avoid
NullReferenceException and Undo.RecordObject failures: at the top of Draw, check
that 'list' and 'undoTarget' are not null and throw ArgumentNullException (or
ArgumentNullException(nameof(list)) / ArgumentNullException(nameof(undoTarget)))
with clear messages if they are null; keep the rest of the logic unchanged so
callers get a fast, descriptive failure rather than a later NRE when Draw uses
'list' or calls Undo.RecordObject on 'undoTarget'.
In `@Tests/Editor/Core/Services/TextureCollectorTests.cs`:
- Around line 1010-1126: Add an integration test that verifies ObjectRegistry
replacement is honored when a texture's GUID is in the excluded GUID list:
create an original texture and a replacement texture, obtain the original's
GUID, construct a TextureCollector with that GUID in its excluded-guids
parameter, register the replacement via the ObjectRegistry replacement API for
the original texture, assign the original to a Material, call
TextureCollector.CollectFromMaterials (collectAll: true), and assert the
collected dictionary contains the replacement Texture2D (not the original), that
the replacement entry IsProcessed is false, and its SkipReason equals
SkipReason.ExcludedTexture; reference TextureCollector, CollectFromMaterials,
ObjectRegistry (the register-replacement method used elsewhere in tests), and
SkipReason to locate the correct APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18720b76-92c0-480c-882b-b85dcd73cdb4
📒 Files selected for processing (4)
Editor/Common/UI/Controls/ExclusionListDrawer.csEditor/Common/UI/Controls/ExclusionListDrawer.cs.metaEditor/TextureCompressor/UI/Sections/FilterSection.csTests/Editor/Core/Services/TextureCollectorTests.cs
✅ Files skipped from review due to trivial changes (1)
- Editor/Common/UI/Controls/ExclusionListDrawer.cs.meta
…e-converted GUIDs
…nstead of pre-extracted skip GUIDs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/Editor/Core/Services/TextureCollectorTests.cs (1)
1162-1167: Align null-frozen test body with its stated intent.Line 1166 currently tests default optional-parameter behavior, not an explicit
nullfrozenTexturesargument. PassfrozenTextures: nullexplicitly to match the test name and avoid ambiguity.Proposed test adjustment
[Test] public void Constructor_WithNullFrozenTextures_DoesNotThrow() { Assert.DoesNotThrow(() => { - var collector = new TextureCollector(64, 0, true, true, true, true, true); + var collector = new TextureCollector( + 64, + 0, + true, + true, + true, + true, + true, + excludedPathPrefixes: null, + excludedTextures: null, + frozenTextures: null + ); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Editor/Core/Services/TextureCollectorTests.cs` around lines 1162 - 1167, The test Constructor_WithNullFrozenTextures_DoesNotThrow currently relies on default optional parameters instead of explicitly passing a null frozenTextures; update the test body to call the TextureCollector constructor with frozenTextures: null (e.g., new TextureCollector(64, 0, true, true, true, true, true, frozenTextures: null)) so the assertion truly verifies that passing a null frozenTextures to the TextureCollector constructor does not throw; keep the Assert.DoesNotThrow wrapper and refer to the TextureCollector constructor overload being tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Tests/Editor/Core/Services/TextureCollectorTests.cs`:
- Around line 1162-1167: The test
Constructor_WithNullFrozenTextures_DoesNotThrow currently relies on default
optional parameters instead of explicitly passing a null frozenTextures; update
the test body to call the TextureCollector constructor with frozenTextures: null
(e.g., new TextureCollector(64, 0, true, true, true, true, true, frozenTextures:
null)) so the assertion truly verifies that passing a null frozenTextures to the
TextureCollector constructor does not throw; keep the Assert.DoesNotThrow
wrapper and refer to the TextureCollector constructor overload being tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bac4ff46-806d-404f-8267-be0bd3358404
📒 Files selected for processing (7)
Editor/Common/UI/Controls/ExclusionListDrawer.csEditor/TextureCompressor/Core/Services/TextureCollector.csEditor/TextureCompressor/Core/Services/TextureCompressorService.csEditor/TextureCompressor/TextureCompressorEditor.csEditor/TextureCompressor/UI/Preview/PreviewGenerator.csEditor/TextureCompressor/UI/Sections/FilterSection.csTests/Editor/Core/Services/TextureCollectorTests.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- Editor/TextureCompressor/TextureCompressorEditor.cs
- Editor/TextureCompressor/UI/Preview/PreviewGenerator.cs
- Editor/TextureCompressor/Core/Services/TextureCompressorService.cs
- Editor/TextureCompressor/UI/Sections/FilterSection.cs
- Editor/Common/UI/Controls/ExclusionListDrawer.cs
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
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.
Summary by CodeRabbit
New Features
UI
Preview / UX
Documentation
Tests