Skip to content

fix(textures): reversibly free textures on cleanup so they reload in-place#87

Merged
chiefcll merged 2 commits into
mainfrom
fix/texture-cleanup-reversible-free
Jun 9, 2026
Merged

fix(textures): reversibly free textures on cleanup so they reload in-place#87
chiefcll merged 2 commits into
mainfrom
fix/texture-cleanup-reversible-free

Conversation

@chiefcll

@chiefcll chiefcll commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What

Texture memory cleanup now reversibly frees textures instead of destroying them, so a node that keeps its texture reference re-displays correctly when it scrolls back into the viewport.

Why (the bug)

LRU/idle cleanup routed reclamation through the terminal path: TextureMemoryManager.cleanup()destroyTexture()Texture.destroy() (which calls removeAllListeners()) + cache eviction.

A CoreNode only subscribes to its texture's loaded/freed events once, in loadTextureTask (on assignment/creation) — never on viewport re-entry. So when cleanup destroyed an off-screen texture the node still referenced:

  1. Node scrolls back in → setRenderableOwner(true)load() correctly re-triggers the reload.
  2. Texture reloads all the way to loaded.
  3. But its listeners were stripped by destroy(), so onTextureLoaded never runs → textureLoaded stays false → the node is never put back in the render list.

Result: "the texture is loaded but nothing is displayed" — a blank poster. It only self-corrected when the reactive layer happened to assign a new texture object (cache miss after eviction), which re-subscribed.

This was reported against an infinite/carousel virtual row: scroll down (textures freed under memory pressure), scroll back up, posters blank.

When it was introduced

The latent design smell — cleanup using the destroy path — dates to the initial release. It became an observable regression in #56 (844fec62, "release shader value cache and subtexture listeners on destroy"), which added removeAllListeners() to Texture.destroy(). That change was correct for genuine teardown but severed the last surviving link for nodes still holding the reference.

The fix

  • New TextureMemoryManager.freeTexture() — reversible reclamation that calls texture.free() (releases the GPU resource, sets state freed, reclaims tracked memory) while keeping the Texture object, its listeners, and its cache entry intact.
  • cleanup() now calls freeTexture() instead of destroyTexture(). The freed → reload machinery already existed (ctxTexture getter auto-reload + loadTexture's freed → loading transition); the only thing that was broken was the notification back to the node.
  • destroyTexture() is retained for genuine teardown.
  • Works for both backends — WebGL and Canvas2D free() both set the source to freed and reclaim memory.

Behavior note for reviewers

Freed textures are now kept in cache (correct LRU — re-requests reuse them and avoid duplicate downloads). If we ever want cache eviction under memory pressure, nodes would instead need to re-subscribe on viewport re-entry (more invasive). Flagging in case there was an intentional reason for the old eviction.

Tests

  • Unit (TextureMemoryManager.test.ts): cleanup takes the free() path (not destroy()), and reclaims the freed memory back to baseline.
  • Visual regression (examples/tests/texture-free-reload.ts + certified snapshot): load → move off-screen → force cleanup(true) → move back on-screen → assert the texture reloads and re-displays. The certified baseline was generated via the Docker --ci flow. Verified locally that the buggy destroy path produces a blank poster.

322 unit tests pass; pnpm build clean.

🤖 Generated with Claude Code

chiefcll and others added 2 commits June 9, 2026 14:48
…place

LRU/idle texture cleanup destroyed textures (Texture.destroy() ->
removeAllListeners() + cache eviction) instead of reversibly freeing them.
A CoreNode that kept its texture reference would lose its 'loaded'/'freed'
subscription, so when it scrolled back into the viewport the texture
reloaded all the way to 'loaded' but the node was never re-notified --
leaving the node blank ("texture is loaded but nothing is displayed").

The latent design issue (cleanup using the terminal destroy path) dates to
the initial release, but it only became an observable regression in #56,
which added removeAllListeners() to Texture.destroy() -- correct for genuine
teardown, but it severed the last surviving link for nodes still holding the
reference.

Fix: add TextureMemoryManager.freeTexture(), a reversible reclamation that
releases the GPU resource and transitions the source to 'freed' while keeping
the Texture object, its listeners, and its cache entry intact. cleanup() now
uses freeTexture() instead of destroyTexture(); the 'freed' -> reload
machinery already existed and now correctly re-notifies the node. Frees are
kept in cache so re-requests reuse them (and avoid duplicate downloads).
destroyTexture() is retained for genuine teardown.

- Unit tests: cleanup takes the free() path (not destroy()) and reclaims memory.
- Visual regression: texture-free-reload exercises load -> offscreen ->
  forced cleanup -> back onscreen -> assert reload + re-display.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@chiefcll chiefcll merged commit ebb2791 into main Jun 9, 2026
1 check passed
@chiefcll chiefcll deleted the fix/texture-cleanup-reversible-free branch June 9, 2026 19:02
chiefcll added a commit that referenced this pull request Jun 10, 2026
destroy() severs all listeners, so calling destroyTexture on a texture
that still has subscribers reintroduces the blank-poster bug from #87.
Restrict it to the one caller that proves the orphan precondition
(evictOrphanedTextures) and document freeTexture as the public,
reversible path for memory pressure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chiefcll added a commit that referenced this pull request Jun 10, 2026
* fix(textures): evict orphaned cached textures during cleanup

Since #87, cleanup() only ever reversibly frees textures, so nothing
evicts keyCache entries anymore — a slow leak of Texture JS objects in
long-running apps that cycle many unique image URLs.

Add an orphan-eviction sweep to TextureMemoryManager.cleanup(): a cached
texture is destroyed and evicted only when nothing can ever re-display
it — zero renderableOwners AND zero event listeners (every live
referencer, CoreNode.loadTextureTask and SubTexture, subscribes via
on()). This preserves the #87 invariant: a texture with any listener is
never destroyed, since destroy()'s removeAllListeners() would sever the
node's subscription and reintroduce the blank-poster bug.

Evictable states are 'freed', plus 'initial'/'failed' orphans once the
existing 2s startup grace period expires — the grace period covers the
same-frame window where a new node's subscription microtask hasn't
flushed yet. In-flight states are never evicted; 'loaded' orphans go
through the pressure-driven free path first.

Adds EventEmitter.hasListeners() as the liveness signal.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* refactor(textures): make destroyTexture private

destroy() severs all listeners, so calling destroyTexture on a texture
that still has subscribers reintroduces the blank-poster bug from #87.
Restrict it to the one caller that proves the orphan precondition
(evictOrphanedTextures) and document freeTexture as the public,
reversible path for memory pressure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant