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

Deprecate THREE.NoColorSpace, use 'null' instead #25899

Closed
wants to merge 2 commits into from

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Apr 21, 2023

Related:

While cleaning up docs, I'm also trying to mention the CSS Color Module Level 4 strings ("srgb", "srgb-linear") where appropriate, to clarify the mappings to CSS. I've left DisplayP3ColorSpace undocumented for now, as it's not ready for use.

@github-actions
Copy link

github-actions bot commented Apr 21, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
634.1 kB (157.2 kB) 634.1 kB (157.2 kB) +26 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
425.2 kB (103.1 kB) 425.2 kB (103.1 kB) +19 B

CodyJasonBennett added a commit to pmndrs/react-three-fiber that referenced this pull request Apr 21, 2023
@gkjohnson
Copy link
Collaborator

gkjohnson commented Apr 22, 2023

Can I ask why "null" is better or more clear than "NoColorSpace"? The .colorSpace field is basically set as an enum so it only makes sense to me to use another named enum to mean that no conversion is necessary. As a new user I'd also think I'd go to the color spaces enums page to see what "NoColorSpace" means or why it's useful - will "null" be on that page, as well?

Anyway not a strong feeling. Just surprised null seemed better to others.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 22, 2023

I have no preference here, but we need to decide very soon. Both options seem fine to me. /cc @WestLangley @mrdoob

...it only makes sense to me to use another named enum to mean that no conversion is necessary

I would push back a little on the idea that this means "no conversion is necessary" — it means the texture has not been assigned a color space. In some (TBD) contexts this could be interpreted as an error, and a warning would be logged.

As a new user I'd also think I'd go to the color spaces enums page to see what "NoColorSpace" means or why it's useful - will "null" be on that page, as well?

The preview in docs will appear as shown below. I don't think we would need to list null as a color space enum value, it is not a color space. .colorSpace = null indicates that the texture has not been assigned a color space, and I think that will be a familiar pattern to users.

Screenshot 2023-04-22 at 3 31 28 PM

That said, on the balance we generally do not use nullable enums in three.js... Counter-examples: NoToneMapping, CullFaceNone, NoBlending, NeverDepth, NeverStencilFunc.

I think @mrdoob's concern is that we may want the renderer's color space to be unspecified so we can auto-upgrade to Display P3 in the future. Using renderer.outputColorSpace = NoColorSpace does not make sense for that, and having both NoColorSpace = '' and nullable color space properties is a weird mix. I'd rather not do both. I would not anticipate our auto-upgrading users to Display P3 within the next year.

@gkjohnson
Copy link
Collaborator

I would push back a little on the idea that this means "no conversion is necessary" — it means the texture has not been assigned a color space.

Yes but when assigned appropriately it means do not perform a conversion since this is not color data which made sense to me.

That said, on the balance we generally do not use nullable enums in three.js... Counter-examples: NoToneMapping, CullFaceNone, NoBlending, NeverDepth, NeverStencilFunc.

Yes this is the bit that feels most strange and inconsistent to me - and why I didn't understand the "consistent" rationale for using null.

I think @mrdoob's concern is that we may want the renderer's color space to be unspecified so we can auto-upgrade to Display P3 in the future.

I don't know enough about these plans so I can't comment. Sorry for noise! Again both are usable so if everyone else is good with this it should be fine.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 23, 2023

Just want to comment that I prefer THREE.NoColorSpace instead of null. It's the more clear approach to me.

@mrdoob
Copy link
Owner

mrdoob commented Apr 25, 2023

Counter-examples: NoToneMapping, CullFaceNone, NoBlending, NeverDepth, NeverStencilFunc.

Interesting... I'll study this today 🤔

@mrdoob
Copy link
Owner

mrdoob commented Apr 26, 2023

Okay, lets keep NoColorSpace. Sorry for the wasted work @donmccurdy 🙏

@mrdoob mrdoob closed this Apr 26, 2023
@mrdoob mrdoob removed this from the r152 milestone Apr 26, 2023
@donmccurdy donmccurdy deleted the feat/deprecate-NoColorSpace branch April 26, 2023 13:17
@trusktr
Copy link
Contributor

trusktr commented Oct 4, 2023

If the auto-color-space idea ever went forward, instead of null there could be a new enum value:

  • AutoColorSpace - let the material choose
  • NoColorSpace - same as before: explicitly no conversions will happen

Assuming that color space could be reliably detected (I believe that was the concern that closed the other issue) the AutoColorSpace value would perhaps be more intuitive than null.

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.

6 participants