Skip to content

Fix correctness bugs in TextureManager and image worker#11

Merged
chiefcll merged 1 commit into
mainfrom
fix/texture-manager-bugs
May 11, 2026
Merged

Fix correctness bugs in TextureManager and image worker#11
chiefcll merged 1 commit into
mainfrom
fix/texture-manager-bugs

Conversation

@chiefcll
Copy link
Copy Markdown
Contributor

Summary

Reviewed CoreTextureManager, ImageTexture, and ImageWorker for bugs and lower-risk issues, and fixed them in this PR. Larger optimizations (in-flight dedup, parallel getTextureData in processSome, lazy resolveDefaults on cache miss, replacing XHR with fetch+AbortController) are intentionally not included here and tracked separately.

What changed

ImageWorker

  • Fixed a dead/tautological supportsOptionsCreateImageBitmap === false && supportsOptionsCreateImageBitmap === false branch that caused modern browsers to silently fall through to the no-options createImageBitmap(blob) path. The fallback now requires both options and full support to be false.
  • Worker now resolves with the computed withAlphaChannel instead of echoing the raw input — previously the worker path returned null/undefined for PNGs while the main-thread path returned true, leading to inconsistent GL uploads.
  • withAlphaChannel now treats null the same as undefined, matching the main thread's nullish-coalesce behavior.
  • getImage now rejects when no worker is available instead of leaving the promise hanging.
  • Wired worker.onerror / worker.onmessageerror: a crashed worker now rejects all outstanding pending requests rather than hanging the entire load pipeline.
  • Worker-script blob URL is revoked after workers spawn.
  • let minLoad = 99Infinity so we always pick the genuinely least-loaded worker.
  • Removed unused imageWorkersEnabled field.

ImageTexture

  • Object URL created from a Blob in loadImageFallback is now revoked on both onload and onerror, fixing a per-image leak on the non-createImageBitmap fallback path.
  • makeCacheKey now inserts delimiters between sx/sy/sw/sh so values like (sx=1, sy=23) and (sx=12, sy=3) no longer collide.
  • loadImage computes isBase64Image(src) once.

CoreTextureManager

  • New TEXTURE_UPLOAD_FAILED error code; upload failures no longer masquerade as TEXTURE_DATA_NULL, and the underlying error message is propagated.
  • Removed misleading coreContext !== null check around loadCtxTexture() (return type is non-nullable; the check shadowed an actual NPE risk that didn't exist).
  • processSome skips failed/freed textures pulled from the queue instead of wasting a fetch/decode on dead entries.
  • enqueueUploadTexture rejects failed/freed textures so the queue never holds dead entries in the first place.
  • When initialization completes, any textures enqueued pre-init are drained immediately instead of sitting until the next frame tick happens to call processSome.
  • Fixed typo in the no-image-worker warning string.

TextureError

  • Added TEXTURE_UPLOAD_FAILED to the enum and default-messages map.

Why this matters

The two most impactful items: the duplicate-variable bug was silently disabling the optimal createImageBitmap code path on every modern browser, and the worker-crash / no-worker cases could hang load promises indefinitely. The rest are smaller correctness fixes and resource leaks.

Test plan

  • Existing visual-regression tests pass
  • Existing unit tests pass (pnpm test)
  • Manually verify image loading on a representative scene
  • Verify failed-image case surfaces TEXTURE_UPLOAD_FAILED with a meaningful message instead of TEXTURE_DATA_NULL
  • Verify a deliberately bad URL no longer hangs (rejects via worker error path)

🤖 Generated with Claude Code

- ImageWorker: fix duplicate `supportsOptionsCreateImageBitmap === false`
  check that caused modern browsers to fall through to the no-options
  createImageBitmap branch.
- ImageWorker: return computed `withAlphaChannel` instead of raw input so
  the worker path matches the main thread.
- ImageWorker: treat `null` like `undefined` so withAlphaChannel falls
  back to mime detection, matching the main thread's nullish coalesce.
- ImageWorker: reject (instead of hanging) when no worker is available
  and wire `onerror` / `onmessageerror` so a crashed worker rejects all
  outstanding requests.
- ImageWorker: revoke the worker-script blob URL after workers spawn;
  replace `minLoad = 99` with `Infinity`; drop the unused
  `imageWorkersEnabled` field.
- ImageTexture: revoke the object URL created from a Blob in the
  non-createImageBitmap fallback path on both success and error.
- ImageTexture: add delimiters between sx/sy/sw/sh in `makeCacheKey` to
  prevent cache-key collisions (e.g. sx=1,sy=23 vs sx=12,sy=3).
- ImageTexture: compute `isBase64Image(src)` once in `loadImage`.
- CoreTextureManager: introduce `TEXTURE_UPLOAD_FAILED` error code and
  use it for upload failures (previously misreported as
  `TEXTURE_DATA_NULL`); propagate the underlying message.
- CoreTextureManager: drop the unreachable `coreContext !== null` check
  around `loadCtxTexture()` (return type is non-nullable).
- CoreTextureManager: skip `failed`/`freed` textures pulled from the
  upload queue in `processSome`, and reject them in
  `enqueueUploadTexture` so the queue never holds dead entries.
- CoreTextureManager: drain `uploadTextureQueue` when initialization
  completes so textures enqueued before init don't sit waiting for a
  frame tick.
- CoreTextureManager: fix typo in the no-image-worker warning.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@chiefcll chiefcll merged commit f2953af into main May 11, 2026
1 check passed
@chiefcll chiefcll deleted the fix/texture-manager-bugs branch May 11, 2026 01:13
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