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

Use clamped material.emissiveIntensity at GLTF export #22007

Merged
merged 8 commits into from
Jun 18, 2021

Conversation

SBRK
Copy link
Contributor

@SBRK SBRK commented Jun 17, 2021

Related issue: Fixed #22000

Description

With the recent change that exported emissive without multiplying by emissiveIntensity, materials that had an emissiveIntensity between 0 and 1 would come out too bright on exported gltf.
Clamping the emissiveIntensity between 0 and 1 before multiplying by it ensures the output GLTF complies with specs and emissiveFactor r g b components are between 0 and 1

build/three.js Outdated Show resolved Hide resolved
@mrdoob mrdoob added this to the r130 milestone Jun 17, 2021
Comment on lines 1188 to 1190
// note: `emissiveIntensity` is clamped between 0 and 1 to accommodate glTF spec. see #21849 and #22000.
const emissiveIntensity = Math.min( Math.max( material.emissiveIntensity, 0 ), 1 );
const emissive = material.emissive.clone().multiplyScalar( emissiveIntensity ).toArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is not what I tried to suggest :-) . See #22000 (comment).

First scale the color by intensity.

If any RGB component exceeds 1, scale the color by ( 1 / max-component ) and throw a warning.

I think that is the best temporary workaround until the glTF specification is updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WestLangley but if we do that, then we might end up with a color that is much different than the original, right ? (in most cases, just white)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok scrap my previous comment, I see what you mean. If one component exceeds 1, then scale ALL components by (1 / max component). It will indeed keep the right hue and scale closer to the intended emissiveIntensity than if we just clamp it before scaling

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Something like this:

const emissive = material.emissive.clone().multiplyScalar( material.emissiveIntensity );

const maxEmissiveComponent = Math.max( emissive.r, emissive.g, emissive.b );

if ( maxEmissiveComponent > 1 ) {

	emissive.multiplyScalar( 1 / maxEmissiveComponent );

	console.warn( 'THREE.GLTFExporter: Some emissive components exceed 1; emissive has been limited' );

}

if ( maxEmissiveComponent > 0 ) {

	materialDef.emissiveFactor = emissive.toArray();

}

Copy link
Owner

@mrdoob mrdoob Jun 18, 2021

Choose a reason for hiding this comment

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

@SBRK I think @WestLangley's suggestion is more readable 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I implemented his suggestion

@mrdoob mrdoob merged commit a5ce030 into mrdoob:dev Jun 18, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jun 18, 2021

Thanks!

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.

Materials with emissiveIntensity under 1 are exported with an emissive way too bright with GLTFExporter
4 participants