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

NodeMaterial: Rename .colorSpace -> .colorSpaced property #27238

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Nov 23, 2023

Related issue: #26440 (comment)

Description

Folowing the same logic of Material.toneMapped.

@sunag sunag added this to the r159 milestone Nov 23, 2023
@sunag sunag marked this pull request as ready for review November 23, 2023 19:36
@sunag sunag merged commit 3b2deb6 into mrdoob:dev Nov 23, 2023
11 checks passed
@sunag sunag deleted the dev-colorspaced branch November 23, 2023 21:18
@mrdoob
Copy link
Owner

mrdoob commented Nov 28, 2023

Hmmm, not sure about these...

  • fog
  • normals
  • lights
  • toneMapped
  • colorSpaced
  • forceSinglePass

Are there any other material booleans like these?

@WestLangley
Copy link
Collaborator

I hope someone can suggest a better property name than colorSpaced...

@sunag
Copy link
Collaborator Author

sunag commented Nov 28, 2023

Are there any other material booleans like these?

Some other booleans:

  • clipping
  • vertexColors
  • transparent
  • alphaHash
  • stencilWrite
  • clipShadows
  • colorWrite
  • dithering
  • alphaToCoverage
  • premultipliedAlpha

@sunag
Copy link
Collaborator Author

sunag commented Nov 28, 2023

I hope someone can suggest a better property name than colorSpaced...

I don't now if @mrdoob or you prefer applyColorSpace? ( #26440 (comment) ), the prefix apply remember me a function name, but certainly there are others possibilities.

@LeviPesin
Copy link
Contributor

Maybe correctColorSpace/colorSpaceCorrection/colorSpaceCorrected? @sunag @mrdoob @WestLangley

the prefix apply remember me a function name

I guess it is because all function names should be verbs. Not sure what should be these properties -- we already have for example clipShadows (verb) but most are just nouns (alphaToCoverage/dithering/so on).

Maybe something like useFog/useToneMapping/etc?

@WestLangley
Copy link
Collaborator

WestLangley commented Jan 14, 2024

What you are doing is referred to as a color conversion -- converting a color from one color space to another.

.colorConverted ?

But in particular, you are either honoring or ignoring the renderer's output color space. I am inclined to think the API may require a bit more thought...

AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
@LeviPesin
Copy link
Contributor

LeviPesin commented Jan 24, 2024

Hm... Maybe .colorConvert then? But colorConverted also seems OK to me.

@sunag
Copy link
Collaborator Author

sunag commented Mar 7, 2024

But in particular, you are either honoring or ignoring the renderer's output color space. I am inclined to think the API may require a bit more thought...

Some materials in particular like MeshNormalMaterial didn't do exactly this color space conversion, they would by default be colorConverted=false, although to obtain the normals for RenderTargets today with Nodes there are better ways.

I'm considering following this same logic that you mentioned for toneMapped too here #27880

@WestLangley
Copy link
Collaborator

@sunag Please keep my suggestion -- or @LeviPesin's alternative -- in mind as you think about this. 😇 .colorSpaced does not have an obvious meaning.

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