fix: keep a path-tracked texture published across UpdateTexture's recreate [patch] - #451
Merged
matt-edmondson merged 1 commit intoSep 24, 2026
Conversation
…reate [patch] UpdateTexture's delete-and-recreate fallback routed through DeleteTexture, which drops every cache entry pointing at the deleted handle - including the entry GetOrLoadTexture published for that path - and then never restored it. The texture stayed live on the GPU while TryGetTexture reported it missing, the next GetOrLoadTexture uploaded a duplicate, and CleanupAllTextures, which walks Textures.Keys, could no longer reach either handle to free it. The fallback now captures the keys the instance is published under, by reference equality so only its own entries come back, and restores them after the recreate. Capture, delete, recreate and republish run inside one Invoker.Invoke for the same reason GetOrLoadTexture publishes inside its own: between the delete and the republish the path is absent from the cache, so a concurrent load would otherwise miss and upload a second GPU texture. Fixes #450 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjdYTWFr5EVfbmSobYcw3G
|
matt-edmondson
deleted the
claude/imguiapp-450-updatetexture-cache-republish
branch
September 24, 2026 00:47
This was referenced Sep 24, 2026
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.



Fixes #450.
ImGuiApp.UpdateTexturefalls back to delete-and-recreate when the size changes or the backend declines an in-place update. That fallback went throughDeleteTexture(nint), which drops every cache entry whose handle matches:For a texture that came from
GetOrLoadTexture, that is the entry published under its path.UpdateTexturethen mutatedtextureInfoin place with the fresh handle and never restored the entry, so the texture stayed live on the GPU while the cache no longer knew about it.The change
The fallback captures the keys this instance is published under, deletes, recreates, then restores them.
Two decisions worth a look:
Reference equality, not a handle comparison. Only the entry this exact
ImGuiAppTextureInfois published under should come back. Matching onTextureIdwould republish an entry that a different info object happened to share a handle with, which is the same class of mistake as the over-broad removal inDeleteTexturethat caused this.The fallback now runs inside one
Invoker.Invoke. This is load-bearing rather than tidying: between the delete and the republish the path is absent from the cache, so a concurrentGetOrLoadTexturefor it would miss, re-decode the file and upload a second GPU texture — reintroducing the very leak by another route.GetOrLoadTexturealready publishes inside its ownInvokefor exactly this reason, and its comment spells the hazard out. Invoke bodies never overlap, so nothing can observe the gap. NestedInvokeruns inline on the invoker thread and is already exercised today (GetOrLoadTexture's body callsUploadTextureRGBA, which invokes again). The span copy is hoisted out of the lambda since aReadOnlySpan<byte>cannot be captured; it is the sameToArray()the recreate path always made.The
<remarks>onUpdateTexturenow states that a path-tracked texture stays published across the fallback, since that is now part of the contract.The other option the issue offered
The issue also proposed throwing when
UpdateTextureis handed a path-tracked texture. Not taken: hot-reloading a file-backed image at a new resolution is a reasonable thing to do and nothing in the public API discourages it, so making the two APIs compose is better than forbidding the combination. No public signature changes either way.Tests
Three added to
ImGuiAppTests, next to the existingUpdateTexture_*cases. Verified by stashing theImGuiApp.cschange and re-running — two fail against the old implementation:UpdateTexture_OnAPathTrackedTexture_KeepsItPublishedInTheCacheTryGetTexturereturnsfalsefor a texture that is loaded and in useUpdateTexture_OnAPathTrackedTexture_DoesNotUploadADuplicateOnTheNextLoadGetOrLoadTexturereturns a different instance, having uploaded a second GPU textureUpdateTexture_OnAnUntrackedTexture_StaysOutOfTheCacheCreateTexturetexture still never enters the path-keyed cacheThe existing
UpdateTexture_WithDifferentSize_RecreatesTextureandUpdateTexture_WhenBackendDeclines_FallsBackToRecreatemissed this because both drive aCreateTexture-sourced texture, which was never in the cache to be evicted.The second test also asserts the path is still among
Textures.Keys, which is whatCleanupAllTexturesiterates — that is the property that makes the handle freeable. It asserts on the keys rather than callingCleanupAllTexturesbecause that method early-returns whenglis null, and these tests register a renderer backend without a GL context.ImGui.App.Tests: 449 passed, 0 failed, 0 skipped. Whole solution builds with 0 warnings, 0 errors.ImGuiAppDemo.UITestscould not be run here:examples/ImGuiAppDemo/icon.pngis a Git LFS pointer andgit-lfsis not installed in this container, so the harness fails to decode it duringSetUpand every test cascades from that. It is unrelated to this diff — nothing here touches the demo or image decoding — and CI fetches LFS.🤖 Generated with Claude Code
https://claude.ai/code/session_01AjdYTWFr5EVfbmSobYcw3G
Generated by Claude Code