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

ColorManagement: Rename .legacyMode=false → .enabled=true #24940

Merged
merged 4 commits into from
Feb 6, 2023

Conversation

donmccurdy
Copy link
Collaborator

Related issue:

Renames ColorManagement.legacyMode = false to ColorManagement.enabled = true. While I think it's fair to say that three.js "is color managed" in some sense with or without this flag, the .legacyMode terminology is confusing. I'd frame this as a flag that enables/disables the THREE.ColorManagement API, specifically, rather than enabling/disabling all colorspace-aware decisions globally (which no one wants).

Also updates the webgl_mirror.html example to use ColorManagement.enabled = true. Because hexadecimal and CSS-like colors are assumed to be sRGB in that mode (as they are in HTML/CSS) it is necessary to update the currently-specified colors, doing either of two things:

  • (A) Keep using hexadecimal syntax; convert Linear-sRGB → sRGB
  • (B) Specify colors as RGB components; no conversion needed

I've opted for (A) here.

@LeviPesin
Copy link
Contributor

LeviPesin commented Nov 13, 2022

I oppose this (and the previous) PR for two reasons:

  1. Setting ColorManagement.enabled = false feels like disabling the color management in three.js completely, which is not the case - only hexadecimal, fog, background, and clear colors management is changed. legacyMode suits more, because it means that we had some color management mode before, and that we have another now.
  2. ColorManagement.enabled = true feels like enabling some optional mode, not that one that will become default at some time. If we want more users to migrate to the new mode, it would be better to note that the old mode is legacy even in the property name.

(Something like disableLegacy can work for both these reasons)

Also (not very related to this PR, but coming from the idea in 2), maybe we can log a deprecation warning if ColorManagement is used in the legacy mode?

@Mugen87 Mugen87 added this to the r147 milestone Nov 13, 2022
@donmccurdy
Copy link
Collaborator Author

Thanks @LeviPesin — 

maybe we can log a deprecation warning if ColorManagement is used in the legacy mode?

Do you mean logging a warning for all apps that do not specify ColorManagement.enabled = true (or equivalent?)? It's probably too soon for that, we haven't even started using it in examples... Or only warn if ColorManagement is accessed in some way? The current ColorManagement methods are mostly used internally, so I'm not sure what problem this would solve yet, or how to detect internal vs. external access.

Setting ColorManagement.enabled = false feels like disabling the color management in three.js completely, which is not the case...

I can't argue about what it feels like — that's certainly going to depend on the reader, and I understand your reaction. I guess I'd just point out that others have found the legacyMode naming to be confusing in other ways. We may have to just choose a name that is "good enough", then document what it does and does not affect.

THREE.ColorManagement.enabled = true matches the convention of THREE.Cache.enabled = true, for example, and that flag certainly does not toggle all cache-like behavior in the library. The THREE.Cache system describes its behavior, and the .enabled flag enables/disables that behavior.

@LeviPesin
Copy link
Contributor

Do you mean logging a warning for all apps that do not specify ColorManagement.enabled = true (or equivalent?)?

Yes — but if it is too soon currently, maybe in a few releases?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Nov 14, 2022

No need to convince me, I like the API already. :)

Practically I think we'll want to make the change in some number of examples, and see what issues or feedback comes up, either from users or maintainers.

If we decide to move forward, the other question is whether to make the change in isolation, or to time it alongside any other changes. Changing these two defaults at the same time would reduce friction, compared to changing either one alone:

  • THREE.ColorManagement.enabled = true
  • renderer.outputEncoding = sRGBEncoding1

1 or similarly, renderer.outputColorSpace = SRGBColorSpace, with #23936

@donmccurdy donmccurdy force-pushed the colormanagement-enabled branch from 787517f to edd78bf Compare February 6, 2023 03:51
@donmccurdy
Copy link
Collaborator Author

Rebased and updated the Editor — I'll merge the PR this week, if that's OK.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 6, 2023

Would be nice to see this change in R150!

@mrdoob mrdoob merged commit 25198b0 into mrdoob:dev Feb 6, 2023
@donmccurdy donmccurdy deleted the colormanagement-enabled branch February 6, 2023 15:03
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.

4 participants