-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
Texture+WebGLRenderTarget: Replace .encoding with .colorSpace #25771
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
Conversation
|
@Mugen87 I've made backwards-compatible updates to property names here, but haven't updated examples. I'm happy to do that in this PR, or another, as you prefer. I thought it might be helpful to see that the E2E tests continue to pass before examples are updated. @WestLangley How do you feel about the choice of defaults here? |
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
| export { arrayMin, arrayMax, arrayNeedsUint32, getTypedArray, createElementNS }; | ||
| const _cache = {}; | ||
|
|
||
| function warnOnce( message ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opted to add this warnOnce( message ) helper, at least temporarily. Because texture.encoding is read on every frame, it gets very noise otherwise.
I think it is the correct choice. |
Good idea! Since the tests pass, I think we can use this PR to update the rest of the codebase accordingly. In this way, the task is logically bundled in one PR. BTW: What has been implemented so far looks already great! |
|
@Mugen87 ok – tests are green! ✅ |
It's no longer required starting from r152 See: mrdoob/three.js#25771
Specifying color spaces are no longer required starting from r152 See: mrdoob/three.js#25771
`texture.encoding` has been renamed to `texture.colorSpace` in r152 This commit adds compat codes to compensate See: mrdoob/three.js#25771
Changes in this PR:
The .encoding property will continue to work, with a deprecation warning, for the next ~10 releases. The default value of texture.colorSpace is
THREE.NoColorSpace | '', and notTHREE.LinearSRGBColorSpace | 'srgb-linear', because relatively few textures are Linear-sRGB in typical usage, and NoColorSpace is a more appropriate choice for non-color data like normal and roughness maps. Color textures should be manually assignedTHREE.SRGBColorSpace | 'srgb'.Whether WebGLRenderTarget should default to THREE.NoColorSpace or THREE.LinearSRGBColorSpace is a good question for thought, but in practice it makes no difference — neither is handled any differently by the renderer at this time.
Remaining changes for r152, in next PRs:
Related issue: