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.

Sign up for GitHub

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

Jump to bottom

GLTFLoader: Specify color space of inputs #26534

Merged
merged 2 commits into from
Aug 5, 2023
Merged

GLTFLoader: Specify color space of inputs #26534

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb34e98
GLTFLoader: Specify color space of 'srgb-linear' sources
donmccurdy Aug 4, 2023
1c3e8d1
Fix missing semicolon
donmccurdy Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions examples/jsm/loaders/GLTFLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
BufferGeometry,
ClampToEdgeWrapping,
Color,
ColorManagement,
DirectionalLight,
DoubleSide,
FileLoader,
Expand All @@ -25,6 +26,7 @@ import {
LinearFilter,
LinearMipmapLinearFilter,
LinearMipmapNearestFilter,
LinearSRGBColorSpace,
Loader,
LoaderUtils,
Material,
Expand Down Expand Up @@ -545,7 +547,7 @@ class GLTFLightsExtension {

const color = new Color( 0xffffff );

if ( lightDef.color !== undefined ) color.fromArray( lightDef.color );
if ( lightDef.color !== undefined ) color.setRGB( ...lightDef.color, LinearSRGBColorSpace );

const range = lightDef.range !== undefined ? lightDef.range : 0;

Expand Down Expand Up @@ -663,7 +665,7 @@ class GLTFMaterialsUnlitExtension {

const array = metallicRoughness.baseColorFactor;

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

}
Expand Down Expand Up @@ -939,7 +941,7 @@ class GLTFMaterialsSheenExtension {

if ( extension.sheenColorFactor !== undefined ) {

materialParams.sheenColor.fromArray( extension.sheenColorFactor );
materialParams.sheenColor.setRGB( ...extension.sheenColorFactor, LinearSRGBColorSpace );

}

Expand Down Expand Up @@ -1077,7 +1079,7 @@ class GLTFMaterialsVolumeExtension {
materialParams.attenuationDistance = extension.attenuationDistance || Infinity;

const colorArray = extension.attenuationColor || [ 1, 1, 1 ];
materialParams.attenuationColor = new Color( colorArray[ 0 ], colorArray[ 1 ], colorArray[ 2 ] );
materialParams.attenuationColor = new Color().setRGB( ...colorArray, LinearSRGBColorSpace );

return Promise.all( pending );

Expand Down Expand Up @@ -1180,7 +1182,7 @@ class GLTFMaterialsSpecularExtension {
}

const colorArray = extension.specularColorFactor || [ 1, 1, 1 ];
materialParams.specularColor = new Color( colorArray[ 0 ], colorArray[ 1 ], colorArray[ 2 ] );
materialParams.specularColor = new Color().setRGB( ...colorArray, LinearSRGBColorSpace );

if ( extension.specularColorTexture !== undefined ) {

Expand Down Expand Up @@ -3386,7 +3388,7 @@ class GLTFParser {

const array = metallicRoughness.baseColorFactor;

materialParams.color.fromArray( array );
materialParams.color.setRGB( array[ 0 ], array[ 1 ], array[ 2 ], LinearSRGBColorSpace );
materialParams.opacity = array[ 3 ];

}
Expand Down Expand Up @@ -3478,7 +3480,7 @@ class GLTFParser {

if ( materialDef.emissiveFactor !== undefined && materialType !== MeshBasicMaterial ) {

materialParams.emissive = new Color().fromArray( materialDef.emissiveFactor );
materialParams.emissive = new Color().setRGB( ...materialDef.emissiveFactor, LinearSRGBColorSpace );

}

Expand Down Expand Up @@ -4559,6 +4561,12 @@ function addPrimitiveAttributes( geometry, primitiveDef, parser ) {

}

if ( ColorManagement.workingColorSpace !== LinearSRGBColorSpace && 'COLOR_0' in attributes ) {

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

}
Comment on lines +4564 to +4568
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.


assignExtrasToUserData( geometry, primitiveDef );

computeBounds( geometry, primitiveDef, parser );
Expand Down