Skip to content

Conversation

@donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Apr 4, 2023

Changes in this PR:

  • Texture: Replace .encoding with .colorSpace
  • WebGLRenderTarget: Replace .encoding with .colorSpace

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 not THREE.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 assigned THREE.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:

@donmccurdy donmccurdy added this to the r152 milestone Apr 4, 2023
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 4, 2023

@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?

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize Gzipped Diff from dev
627.8 kB 156.1 kB +846 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize Gzipped Diff from dev
419.4 kB 102.1 kB +697 B

export { arrayMin, arrayMax, arrayNeedsUint32, getTypedArray, createElementNS };
const _cache = {};

function warnOnce( message ) {
Copy link
Collaborator Author

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.

@WestLangley
Copy link
Collaborator

@WestLangley How do you feel about the choice of defaults here?

I think it is the correct choice.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 5, 2023

I thought it might be helpful to see that the E2E tests continue to pass before examples are updated.

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!

@donmccurdy
Copy link
Collaborator Author

@Mugen87 ok – tests are green! ✅

@Mugen87 Mugen87 merged commit 88c9537 into mrdoob:dev Apr 6, 2023
@donmccurdy donmccurdy deleted the feat/texture-colorSpace branch April 6, 2023 12:37
@Methuselah96 Methuselah96 mentioned this pull request Apr 28, 2023
38 tasks
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Jun 7, 2023
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Jun 7, 2023
Specifying color spaces are no longer required starting from r152

See: mrdoob/three.js#25771
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Jun 7, 2023
`texture.encoding` has been renamed to `texture.colorSpace` in r152
This commit adds compat codes to compensate

See: mrdoob/three.js#25771
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.

3 participants