Fixed animated group textures not animating#1124
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a compiler-side issue where animated textures applied as sub-areas (via group texturing tools using ParentArea) were not being recognized as animated, causing them to compile as static textures instead.
Changes:
- Detects textures with a non-zero
ParentAreaand attempts to match them against reference animated textures using reconstructed “full-frame” UVs. - When a match is found, synthesizes a sub-area
AnimatedTextureSet, generates lookup entries for it, and retriesAddTextureso the sub-area can be treated as animated. - Implements the same logic in both the classic TR compiler texinfo manager and the TombEngine texinfo manager.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| TombLib/TombLib/LevelData/Compilers/Util/TexInfoManager.cs | Adds sub-area animated-texture detection + synthetic sub-animation lookup generation for classic/TR compilers. |
| TombLib/TombLib/LevelData/Compilers/TombEngine/TombEngineTexInfoManager.cs | Mirrors the sub-area animated-texture handling for TombEngine texture compilation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR is now complete and ready for review. |
| GenerateAnimTexture(refTex, refQuad, destination, isForTriangle); | ||
| var result = AddTexture(texture, destination, isForTriangle, blendMode); | ||
| { | ||
| result.Animated = true; |
There was a problem hiding this comment.
Not really understanding where this flag is now checked, or why it was removed. Please describe.
There was a problem hiding this comment.
The Animated flag is consumed when polygons are emitted (Result.CreateTombEnginePolygon3/4 set polygon.Animated = Animated). In the sub-area retry path, forcing result.Animated = true was removed so animation is now determined by the recursive AddTexture(...) match result instead of being forced, preventing incorrect animated marking on fallback paths. Commit: f875ab2.
| } | ||
| } | ||
|
|
||
| // Check if this is a sub-area of an animated texture (e.g. applied via group texturing tools). |
There was a problem hiding this comment.
Can we move this to a helper method to avoid clutter in the parent function?
There was a problem hiding this comment.
Done. I moved the sub-area animated-texture block out of AddTexture(...) into a dedicated helper method (TryAddSubAreaAnimatedTexture(...)) to reduce clutter and keep the parent function focused. Commit: f875ab2.
| /// <param name="rect">The rectangle to normalize.</param> | ||
| /// <param name="margin">The quantization step used by animated texture lookup comparisons.</param> | ||
| /// <returns>A rectangle snapped to the lookup grid defined by <paramref name="margin"/>.</returns> | ||
| public static Rectangle2 NormalizeLookupRectangle(Rectangle2 rect, float margin) => new( |
There was a problem hiding this comment.
Do we need this to be public?
There was a problem hiding this comment.
Good point. I reduced visibility in AnimatedTextureLookupUtility: members used only within the type are now private, and cross-type helpers are internal (no unnecessary public). Commit: f875ab2.
| /// <param name="first">The first texture to compare.</param> | ||
| /// <param name="second">The second texture to compare.</param> | ||
| /// <returns><see langword="true"/> when both textures resolve to the same identity; otherwise <see langword="false"/>.</returns> | ||
| public static bool AreEquivalentTextures(Texture first, Texture second) |
There was a problem hiding this comment.
Updated accordingly: methods in AnimatedTextureLookupUtility are no longer unnecessarily public; they’re now private/internal based on actual usage. Commit: f875ab2.
| /// </summary> | ||
| /// <param name="texture">The texture whose identity hash should be computed.</param> | ||
| /// <returns>A hash code suitable for deduplication keys.</returns> | ||
| public static int GetTextureIdentityHash(Texture texture) |
There was a problem hiding this comment.
As above, and for all other public methods that are not used outside the class.
There was a problem hiding this comment.
Applied across the utility: non-external methods were tightened to private/internal so only required surface area remains exposed. Commit: f875ab2.
Agent-Logs-Url: https://github.com/TombEngine/Tomb-Editor/sessions/6a79d356-e4fc-43a2-b28b-5d0ccded6cf4 Co-authored-by: Nickelony <20436882+Nickelony@users.noreply.github.com>
MontyTRC89
left a comment
There was a problem hiding this comment.
To test for performance with big levels
This pull request introduces robust support for animated texture sub-areas - enabling tools that apply textures to only a portion of an animated frame (such as group texturing) to work seamlessly. The changes include a new utility for matching and generating sub-area animation sets, cache key structures for deduplication, and integration with both the general and TombEngine-specific texture managers. Additionally, some redundant code related to resetting texture parent areas was removed.
Animated texture sub-area support:
AnimatedTextureLookupUtilitywith methods for matching, quantizing, and generating sub-area animation sets, as well as aSubAreaKeystruct for deduplicating sub-area lookups. (TombLib/TombLib/LevelData/Compilers/AnimatedTextureLookupUtility.cs)TexInfoManagerandTombEngineTexInfoManager, including logic to handle only compatible animation types and prevent redundant processing via_processedSubAreas. (TombLib/TombLib/LevelData/Compilers/Util/TexInfoManager.cs,TombLib/TombLib/LevelData/Compilers/TombEngine/TombEngineTexInfoManager.cs) [1] [2] [3] [4] [5]isForRoomparameter, ensuring correct scoping of generated lookups. (TombLib/TombLib/LevelData/Compilers/Util/TexInfoManager.cs) [1] [2]Code cleanup and bug fixes:
texture.ParentAreabefore applying textures, preventing accidental loss of parent area information needed for sub-area matching. (TombEditor/EditorActions.cs) [1] [2]Dependency management:
usingdirective for the new utility inTexInfoManager. (TombLib/TombLib/LevelData/Compilers/Util/TexInfoManager.cs)