pmndrs / react-three-fiber

🇨🇭 A React renderer for Three.js
https://docs.pmnd.rs/react-three-fiber
MIT License
27.6k stars 1.6k forks source link

Setting maps on a material don't copy over the existing colorspace of that texture. #3386

Open pushmatrix opened 3 weeks ago

pushmatrix commented 3 weeks ago

Problem

When setting a roughnessMap on a standard material, the existing colorspace used by that texture is not applied.

Ex:

const roughness = useTexture("roughness.png");

return(
  <>
   <mesh>
        <planeGeometry />
        <meshStandardMaterial roughnessMap={roughness} />
    </mesh>
  </>
)

useTexture will load it as a THREE.NoColorSpace, which is correct for roughness. However once it gets passed to<meshStandardMaterial> it switches to be an srgb texture.

This can be fixed if I specify roughnessMap-colorSpace={NoColorSpace}, but is non-obvious.

I believe the issue is that is copying the texture, but isn't copying over the color space.

Demo

Here I create a THREE.MeshStandard material and avoid using the r3f by setting material property directly on <mesh>. This works correctly Live Demo

Here I use a <meshStandardProperty> and pass in the roughnessMap. This is incorrect lighting, and you'll notice in the console how the colorspace changes once its applied to the r3f material. Live Demo

Code: https://codesandbox.io/p/sandbox/3t97t5

Additional Info

Unsure if this also applies to other maps and materials, but I assume it would.

krispya commented 3 weeks ago

I can confirm the issue has to do with the prop copy since it works when passed in as an args: https://codesandbox.io/p/sandbox/texture-issue-forked-jtsrcg?file=%2Fsrc%2Fmodel.js

pushmatrix commented 3 weeks ago

FWIW, doing a texture copy in threejs seems to preserve it

const roughness = useTexture(
    "roughness.png"
  );

  const tex = new Texture();
  tex.copy(roughness);

 // These are both NoColorSpace
  console.log(roughness.colorSpace, tex.colorSpace);

So is console.log(roughness.clone().colorSpace)

CodyJasonBennett commented 3 weeks ago

See https://github.com/pmndrs/react-three-fiber/issues/3370#issuecomment-2401056417. This was overzealous behavior for color textures that we're now trying to correct with our next major. Three's API for this is not as welcome as it could be.

pushmatrix commented 3 weeks ago

Here's an additional demo where I'm not using useTexture. Just a straight up new Texture() that has NoColorSpace as its default colorspace. But as soon as its used as in a prop, it switches to srgb

https://codesandbox.io/p/sandbox/texture-issue-forked-jvfmy8