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

GLTFLoader: Specify color space of inputs #26534

Merged
merged 2 commits into from
Aug 5, 2023

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Aug 4, 2023

In order to support wide-gamut working color spaces, loaders should declare the colorspace of source data when constructing THREE.Color instances, rather than (a) explicitly converting it to LinearSRGBColorSpace, or (b) implicitly assuming the working color space matches the input.

In GLTFLoader, that means...

// before
color.fromArray( lightDef.color );


// after
color.setRGB( ...lightDef.color, LinearSRGBColorSpace );

Conversion of vertex colors is slightly more work (consider that morph targets contain deltas against the base vertex color) so I've simply logged a warning to alert us to that situation, if it comes up.

No user-facing differences are expected as a result of this PR; it takes effect only if/when we allow changing the working color space.

Comment on lines +4564 to +4568
if ( ColorManagement.workingColorSpace !== LinearSRGBColorSpace && 'COLOR_0' in attributes ) {

console.warn( `THREE.GLTFLoader: Converting vertex colors from "srgb-linear" to "${ColorManagement.workingColorSpace}" not supported.` );

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like converting color attributes was lost in the new roadmap in #26479. We should plan to cover this case in the long term, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I do intend that to be part of step 2.4, just not ready to make those changes here yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or did you mean just handling sRGB vs Linear sRGB vertex colors in existing workflows, without this wide gamut work? Are we missing something there, if so?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - I think I expect there to be more to this color vertex attribute issue than just updating the loaders. I think item 3.3 we added in #23614 captured what I imagine being needed more completely. For example if users are dynamically generating vertex colors for geometry they need a way to ensure it's converted to the working color space. Though I guess that's convenience more than anything at this point since users can use the Color instance convert before constructing the vertex data array 🤔

Either way I think this is a good PR!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah. This is kind of verbose and unintuitive today:

const _color = new THREE.Color();
const color = geometry.attributes.color;
for (let i = 0; i < color.count; i++) {
  _color.setRGB(color.getX(i), color.getY(i), color.getZ(i), inputColorSpace);
  color.setXYZ(i, ..._color);
}

I don't know if I like the idea of a color-specific BufferAttribute subclass, but a cleaner way to do this would be nice.

@Mugen87 Mugen87 added this to the r156 milestone Aug 4, 2023
@donmccurdy donmccurdy merged commit 7f7f778 into mrdoob:dev Aug 5, 2023
17 checks passed
@donmccurdy donmccurdy deleted the feat/gltfloader-explicit-colors branch August 5, 2023 19:52
@wlinna
Copy link
Contributor

wlinna commented Sep 4, 2023

Unfortunately, it seems like this PR broke GLTFLoader. Let's take this for an example


			if ( Array.isArray( metallicRoughness.baseColorFactor ) ) {

				const array = metallicRoughness.baseColorFactor;

				materialParams.color.setRGB( ...array, LinearSRGBColorSpace);
				
				materialParams.opacity = array[ 3 ];

			}

Now if array has four elements, the fourth one (opacity) will be passed as an argument for colorSpace parameter, and LinearSRGBColorSpace will be ignored.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 4, 2023

Good catch! Would you like to fix this with a PR?

@wlinna
Copy link
Contributor

wlinna commented Sep 4, 2023

I suppose I could do it later today

@wlinna
Copy link
Contributor

wlinna commented Sep 4, 2023

Here's the PR

#26691

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