unfinished work on animation optmization #3
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looking at this PR, I can see it's implementing an optimization for animation loading - moving from eager loading to lazy loading with async file operations. Let me break down what makes sense and what doesn't:
What Makes Sense ✅
- Lazy Loading Strategy: The core idea of scanning file paths at startup but only loading textures when needed is solid for performance
- Async Loading: Using UnityWebRequest for async texture loading prevents blocking the main thread
- Loading State Management: The
_currentlyLoadingHashSet prevents duplicate load operations - Memory Optimization: Only keeping sprites in memory when actually needed
What Doesn't Make Sense Anymore ❌
-
UnityWebRequest for Local Files: This is the biggest issue. UnityWebRequest is meant for web requests, not local file loading. For local files, you should use
File.ReadAllBytesAsync()or similar. -
Broken Async Pattern: The
LoadFolderAsync()method is called synchronously fromTryGetAnimation(), which defeats the purpose of making it async. You need a coroutine runner or proper async/await. -
Race Conditions: The loading state checking has potential race conditions between the lock checks and the actual loading.
-
Missing Error Handling: What happens if loading fails? The ghost card just... doesn't animate? No fallback mechanism.
-
Performance Metrics During Async: Measuring memory usage during async operations is unreliable since other operations might be happening concurrently.
Additional Issues
- The cleanup in
CardUISetGhostCardUI.cschanged from cleaning all animators to just one, which could leave orphaned components - No cache eviction strategy - memory will just grow indefinitely
- The coroutine needs to be started somewhere (like
MonoBehaviour.StartCoroutine())
This looks like a half-finished refactor that got the right idea but didn't complete the implementation properly. The lazy loading concept is good, but the execution needs work to be production-ready.
WILL the cache grow indefinitely? or just until every animated card has been seen once by the player? missing error handling: what else WOULD the fallback be, if not just displaying the static image, which is what it currently does? The rest sounds correct. |
There was a problem hiding this comment.
Great questions! Let me clarify those points:
Cache Growth Analysis
You're absolutely right - the cache won't grow indefinitely in practice. It will only grow until:
- Every animated card type has been encountered (bounded by the number of monster types with animations)
- Each card has been seen in both white and black ghost variants (if applicable)
So the maximum cache size is number_of_animated_monsters × 2 × frames_per_animation. This is actually quite reasonable for a game cache. I was wrong to call this "indefinite growth" - it's bounded growth with a reasonable upper limit.
Error Handling / Fallback
You're spot on here too. The current fallback behavior is exactly what you'd want:
// Try to get animation frames
Sprite[] frames;
if (!Plugin.animated_ghost_cache.TryGetAnimation(data.MonsterType, isBlackGhost, out frames))
{
// TryGetAnimation will start async load if needed
return; // Use default ghost icon while loading ← This IS the fallback!
}If loading fails or hasn't completed yet, it just uses the default static ghost sprite that the game would normally show. That's actually perfect fallback behavior - no need for anything more complex.
So the Real Issues Are:
- UnityWebRequest for local files - should use
File.ReadAllBytesAsync() - Broken async pattern - need proper coroutine startup
- Potential race conditions in loading state management
- Changed cleanup logic from multiple animators to single animator
Thanks for the reality check on those two points - you're absolutely correct that those aren't actually problems!
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
core/AnimatedGhostCache.cs (2)
81-93:UnityWebRequestis overkill for local file I/O.As noted in the PR discussion,
UnityWebRequestis designed for HTTP requests. For local files, usingFile.ReadAllBytes(orFile.ReadAllBytesAsyncwith proper async handling) plusTexture2D.LoadImageis simpler and avoids HTTP overhead.🔎 Simplified local file loading approach
- UnityWebRequest request = null; - try - { - request = UnityWebRequest.Get(Path.GetFullPath(framePath)); - request.downloadHandler = new DownloadHandlerTexture(); - } - catch (Exception ex) - { - Plugin.Logger.LogError($"Error creating web request for frame {i + 1} for {monsterType}: {ex.Message}"); - continue; - } - - yield return request.SendWebRequest(); - - try - { - if (request.result != UnityWebRequest.Result.Success) - { - Plugin.Logger.LogError($"Failed to load frame {i + 1} for {monsterType}: {request.error}"); - continue; - } - - var texture = ((DownloadHandlerTexture)request.downloadHandler).texture; + Texture2D texture = null; + try + { + byte[] fileData = File.ReadAllBytes(framePath); + texture = new Texture2D(2, 2); + if (!texture.LoadImage(fileData)) + { + Plugin.Logger.LogError($"Failed to decode frame {i + 1} for {monsterType}"); + continue; + }Note: If you need true async to avoid blocking, consider yielding every N frames or using a background thread for file I/O.
246-261:LogPerformanceSummaryis defined but never called.This method appears to be dead code. Either integrate it into the loading workflow (e.g., call it after all animations are loaded) or remove it.
Patches/CardUISetGhostCardUI.cs (1)
36-38: Redundant null check forghostCard.
ghostCardis already checked on line 17, and this code path is only reached if it was non-null at that point. The check on line 38 is redundant.if (frames != null && frames.Length > 0) { - if (ghostCard == null) return;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Patches/CardUISetGhostCardUI.cs(1 hunks)Plugin.cs(1 hunks)core/AnimatedGhostCache.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Patches/CardUISetGhostCardUI.cs (3)
Patches/CardUISetCardPatch.cs (1)
Postfix(43-46)core/GhostCardAnimator.cs (2)
GhostCardAnimatedRenderer(8-124)StopAnimation(40-49)core/AnimatedGhostCache.cs (1)
TryGetAnimation(157-173)
Plugin.cs (1)
core/AnimatedGhostCache.cs (1)
ScanAnimationFolders(175-224)
🔇 Additional comments (3)
core/AnimatedGhostCache.cs (1)
45-67: Potential race condition: state accessed outside the lock.The lock protects the checks on lines 45-67, but
_animationFilePathsis accessed on line 69, and_animatedGhostCards/_folderMetricsare modified on lines 126/134, all outside the lock. If called from multiple threads, this could cause issues.If all access is guaranteed from Unity's main thread (typical for coroutines), this is acceptable. However, the lock would then be unnecessary.
Consider either:
- Removing the lock if single-threaded
- Or extending lock protection to cover all shared state access if multi-threaded access is possible
Also applies to: 126-126
Plugin.cs (1)
38-38: LGTM!The change from eager loading to scanning paths at startup aligns with the lazy-loading strategy. This defers texture loading until animations are actually needed.
Patches/CardUISetGhostCardUI.cs (1)
14-25: Cleanup logic looks correct.The change from scanning all children to using
GetComponentis more efficient and targeted. Stopping the animation before destruction is appropriate.
|
|
||
| var frames = new List<Sprite>(); | ||
| var stopwatch = Stopwatch.StartNew(); | ||
| var initialMemory = GetCurrentMemoryUsage(); |
There was a problem hiding this comment.
Memory measurement during async operations is unreliable.
Capturing Profiler.GetTotalAllocatedMemoryLong() before and after an async loading loop measures total process allocations, not just what this load consumed. Other systems allocate memory during the yield statements, skewing the metric.
Consider either:
- Measuring texture memory directly via
Texture2D.GetRawTextureData().Lengthper frame - Or acknowledging this metric is approximate for logging purposes only
Also applies to: 130-132
| public bool TryGetAnimation(EMonsterType monsterType, bool isBlackGhost, out Sprite[] frames) | ||
| { | ||
| var key = (monsterType, isBlackGhost); | ||
| if (_animatedGhostCards.TryGetValue(key, out frames)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var frames = new List<Sprite>(); | ||
| foreach (var framePath in frameFiles) | ||
| { | ||
| byte[] fileData = File.ReadAllBytes(framePath); | ||
| Texture2D texture = new Texture2D(2, 2); | ||
| if (texture.LoadImage(fileData)) | ||
| { | ||
| var sprite = Sprite.Create(texture, | ||
| new Rect(0, 0, texture.width, texture.height), | ||
| new Vector2(0.5f, 0.5f)); | ||
| frames.Add(sprite); | ||
| } | ||
| } | ||
| // If we have paths but haven't loaded yet, start loading | ||
| if (_animationFilePaths.ContainsKey(key)) | ||
| { | ||
| LoadFolderAsync(monsterType, isBlackGhost); | ||
| } | ||
|
|
||
| if (frames.Count > 0) | ||
| { | ||
| _animatedGhostCards[(monsterType, isBlackGhost)] = frames.ToArray(); | ||
|
|
||
| stopwatch.Stop(); | ||
| var finalMemory = GetCurrentMemoryUsage(); | ||
| var memoryUsed = finalMemory - initialMemory; | ||
| frames = null; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Broken async pattern: LoadFolderAsync is never started as a coroutine.
On line 168, LoadFolderAsync returns an IEnumerator but is called without StartCoroutine. This creates an iterator that is never executed—the async load never actually runs. Unity coroutines require being started via a MonoBehaviour.StartCoroutine().
🔎 Proposed approach
You need a MonoBehaviour reference to start coroutines. Consider:
- Pass a
MonoBehaviourreference toTryGetAnimationor store one in the cache - Or use an async/await pattern with
UniTaskinstead of coroutines - Or implement a coroutine runner singleton
Example fix if you have a MonoBehaviour reference:
- public bool TryGetAnimation(EMonsterType monsterType, bool isBlackGhost, out Sprite[] frames)
+ public bool TryGetAnimation(EMonsterType monsterType, bool isBlackGhost, out Sprite[] frames, MonoBehaviour runner = null)
{
var key = (monsterType, isBlackGhost);
if (_animatedGhostCards.TryGetValue(key, out frames))
{
return true;
}
// If we have paths but haven't loaded yet, start loading
- if (_animationFilePaths.ContainsKey(key))
+ if (_animationFilePaths.ContainsKey(key) && runner != null)
{
- LoadFolderAsync(monsterType, isBlackGhost);
+ runner.StartCoroutine(LoadFolderAsync(monsterType, isBlackGhost));
}
frames = null;
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public bool TryGetAnimation(EMonsterType monsterType, bool isBlackGhost, out Sprite[] frames) | |
| { | |
| var key = (monsterType, isBlackGhost); | |
| if (_animatedGhostCards.TryGetValue(key, out frames)) | |
| { | |
| return true; | |
| } | |
| var frames = new List<Sprite>(); | |
| foreach (var framePath in frameFiles) | |
| { | |
| byte[] fileData = File.ReadAllBytes(framePath); | |
| Texture2D texture = new Texture2D(2, 2); | |
| if (texture.LoadImage(fileData)) | |
| { | |
| var sprite = Sprite.Create(texture, | |
| new Rect(0, 0, texture.width, texture.height), | |
| new Vector2(0.5f, 0.5f)); | |
| frames.Add(sprite); | |
| } | |
| } | |
| // If we have paths but haven't loaded yet, start loading | |
| if (_animationFilePaths.ContainsKey(key)) | |
| { | |
| LoadFolderAsync(monsterType, isBlackGhost); | |
| } | |
| if (frames.Count > 0) | |
| { | |
| _animatedGhostCards[(monsterType, isBlackGhost)] = frames.ToArray(); | |
| stopwatch.Stop(); | |
| var finalMemory = GetCurrentMemoryUsage(); | |
| var memoryUsed = finalMemory - initialMemory; | |
| frames = null; | |
| return false; | |
| } | |
| public bool TryGetAnimation(EMonsterType monsterType, bool isBlackGhost, out Sprite[] frames, MonoBehaviour runner = null) | |
| { | |
| var key = (monsterType, isBlackGhost); | |
| if (_animatedGhostCards.TryGetValue(key, out frames)) | |
| { | |
| return true; | |
| } | |
| // If we have paths but haven't loaded yet, start loading | |
| if (_animationFilePaths.ContainsKey(key) && runner != null) | |
| { | |
| runner.StartCoroutine(LoadFolderAsync(monsterType, isBlackGhost)); | |
| } | |
| frames = null; | |
| return false; | |
| } |
| var frameFiles = Directory.GetFiles(dir, "*.png") | ||
| .OrderBy(f => int.Parse(Path.GetFileNameWithoutExtension(f))) | ||
| .ToArray(); |
There was a problem hiding this comment.
int.Parse can throw if filenames are not numeric.
If any PNG file in the directory has a non-numeric name (e.g., frame_01.png instead of 1.png), this will throw a FormatException.
🔎 Proposed fix with TryParse
var frameFiles = Directory.GetFiles(dir, "*.png")
- .OrderBy(f => int.Parse(Path.GetFileNameWithoutExtension(f)))
+ .Select(f => new { Path = f, Name = Path.GetFileNameWithoutExtension(f) })
+ .Where(x => int.TryParse(x.Name, out _))
+ .OrderBy(x => int.Parse(x.Name))
+ .Select(x => x.Path)
.ToArray();Or use a natural sort that handles non-numeric names gracefully.
| // Try to get animation frames | ||
| Sprite[] frames; | ||
| if (!Plugin.animated_ghost_cache.TryGetAnimation(data.MonsterType, isBlackGhost, out frames)) | ||
| { | ||
| // TryGetAnimation will start async load if needed | ||
| return; // Use default ghost icon while loading | ||
| } |
There was a problem hiding this comment.
Async loading won't actually trigger due to broken coroutine pattern.
This code relies on TryGetAnimation to "start async load if needed" (line 31), but as noted in AnimatedGhostCache.cs, the LoadFolderAsync coroutine is never actually started—it's called without StartCoroutine.
To fix, pass the MonoBehaviour (__instance or ghostCard) to start the coroutine:
- if (!Plugin.animated_ghost_cache.TryGetAnimation(data.MonsterType, isBlackGhost, out frames))
+ if (!Plugin.animated_ghost_cache.TryGetAnimation(data.MonsterType, isBlackGhost, out frames, __instance))This requires corresponding changes in TryGetAnimation to accept and use the MonoBehaviour for StartCoroutine.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Patches/CardUISetGhostCardUI.cs around lines 27 to 33, the call to
Plugin.animated_ghost_cache.TryGetAnimation(...) relies on that method to kick
off an async coroutine, but since TryGetAnimation currently never has a
MonoBehaviour to call StartCoroutine on, the async load is never started; modify
the call to pass a MonoBehaviour (e.g., __instance or ghostCard) into
TryGetAnimation and update TryGetAnimation’s signature to accept that
MonoBehaviour and call StartCoroutine(LoadFolderAsync(...)) on it when needed so
the async loading actually begins.
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.