Skip to content

fix: pass mapped textures to onLoad callback in useTexture#2696

Open
MichaelangJason wants to merge 1 commit intopmndrs:masterfrom
MichaelangJason:fix/use-texture-onload-record-type
Open

fix: pass mapped textures to onLoad callback in useTexture#2696
MichaelangJason wants to merge 1 commit intopmndrs:masterfrom
MichaelangJason:fix/use-texture-onload-record-type

Conversation

@MichaelangJason
Copy link
Copy Markdown

Why

onLoad callback for useTexture is typed to accept MappedTextureType<Url>, which includes TextureRecord<Url>. However, onLoad is called with the raw output of useLoader, which is cast to MappedTextureType<Url> but will never actually be a TextureRecord<Url> (only _Texture[] or _Texture, respectively, TextureArray<Url> and SingleTexture<Url>). This means an onLoad callback expecting a TextureRecord<Url> would receive an array instead of a keyed object.

What

This PR moves the useMemo call before the useLayoutEffect, and passes the correctly structured mappedTextures to the onLoad call.

I noticed that the original useLayoutEffect didn't include textures in its dependency list. I assumed this was intentional to avoid re-firing onLoad unnecessarily, so I kept the same pattern and didn't include mappedTextures in the dependency list either.

Checklist

  • Ready to be merged

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 17, 2026

@MichaelangJason is attempting to deploy a commit to the Poimandres Team on Vercel.

A member of the Team first needs to authorize it.

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Feb 17, 2026

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Copy Markdown

@travisbreaks travisbreaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct — onLoad was receiving raw loader output instead of the mapped texture record. Moving useMemo above useLayoutEffect ensures mappedTextures is computed before the callback fires.

One note: the useLayoutEffect dependency array is [onLoad] but doesn't include mappedTextures. If the textures change (e.g., dynamic URL swap), onLoad won't re-fire with the new mapped textures unless the callback reference also changes. This matches the existing behavior before this PR, so not a regression — but worth noting for anyone expecting onLoad to fire on texture updates.

The fix itself is clean and minimal. LGTM on the core change.

@MichaelangJason
Copy link
Copy Markdown
Author

MichaelangJason commented Apr 4, 2026

The fix is correct — onLoad was receiving raw loader output instead of the mapped texture record. Moving useMemo above useLayoutEffect ensures mappedTextures is computed before the callback fires.

One note: the useLayoutEffect dependency array is [onLoad] but doesn't include mappedTextures. If the textures change (e.g., dynamic URL swap), onLoad won't re-fire with the new mapped textures unless the callback reference also changes. This matches the existing behavior before this PR, so not a regression — but worth noting for anyone expecting onLoad to fire on texture updates.

The fix itself is clean and minimal. LGTM on the core change.

@travisbreaks Thank you so much for the code review! Sorry for the late reply.

@MichaelangJason
Copy link
Copy Markdown
Author

MichaelangJason commented Apr 4, 2026

Hi @DennisSmolek !

Just following up on this PR whenever you have a chance to take a look. I’d really appreciate any feedback on whether these changes address the issue correctly, and specifically whether ⁠mappedTextures should be included in the ⁠useLayoutEffect dependency lists.

I noticed the project seems to be focused on v11, so I understand if this PR is lower priority at the moment. I took a quick look at the v11 code, and it seems these changes could likely be applied there as well without modification. If that would be more useful, I’d be happy to open a separate PR for v11 or retarget this one to it.

Since this is my first contribution to an open-source project, I’d also appreciate any guidance on the best next step.

Thanks for your time!

@MichaelangJason MichaelangJason force-pushed the fix/use-texture-onload-record-type branch from 9ff40e1 to fd093de Compare April 4, 2026 04:07
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.

2 participants