Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Textures from useTexture don’t receive options applied in useEffect #902

Closed
FarazzShaikh opened this issue May 13, 2022 · 3 comments · Fixed by #903
Closed

Textures from useTexture don’t receive options applied in useEffect #902

FarazzShaikh opened this issue May 13, 2022 · 3 comments · Fixed by #903
Labels
bug Something isn't working released

Comments

@FarazzShaikh
Copy link
Member

FarazzShaikh commented May 13, 2022

  • three version: 0.140.2
  • @react-three/fiber version: 8.0.7
  • @react-three/drei version: 9.7.1
  • node version:
  • npm (or yarn) version:

Problem description:

Textures from useTexture don’t receive the repeat property when it’s set in a useEffect. It does work when set in a useLayoutEffect.

This is probably due to the internal useEffect here

useEffect(() => {
overriding the setting.

Relevant code:

Suggested solution:

Not sure, maybe to change the internal useEffect to a useLayoutEffect

@github-actions
Copy link

🎉 This issue has been resolved in version 9.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@drcmda
Copy link
Member

drcmda commented Oct 2, 2022

i bumped into this today @FarazzShaikh i think we should revert. this isn't about useEffect, but just threejs not being able to set certain properties without needsUpdate, which isn't a bug but expected behaviour as useEffect runs after render. i think we shouldn't mask that with lifecycles.

the onload imo leads to bad coding practices and i'm sure pretty much all loaders have cases like that, everything that's setting buffers or shader defines must have needsUpdate in three if it runs after the object has been rendered out once.

✅ this works (this is how you'd do it in vanilla, too, when the object has been rendered already)

  useEffect(() => {
    tex.wrapS = tex.wrapT = RepeatWrapping
    tex.repeat.setScalar(2)
    tex.needsUpdate = true
  }, [tex])

✅ this works (this runs before render, so no need to needsUpdate)

  useLayoutEffect(() => {
    tex.wrapS = tex.wrapT = RepeatWrapping
    tex.repeat.setScalar(2)
  }, [tex])

✅ this works

const text = useTexture(url)
tex.wrapS = tex.wrapT = RepeatWrapping
tex.repeat.setScalar(2)

✅ this works

const text = useTexture(url)
useMemo(() => {
  tex.wrapS = tex.wrapT = RepeatWrapping
  tex.repeat.setScalar(2)
}, [url])

@FarazzShaikh
Copy link
Member Author

Huh yeah, useLayoutEffect would be the right place to put the code. I was just unaware of the use of that hook at the time.

We can revert this, I don’t see many people using this anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants