GLTFLoader: Regressions in PBR rendering with r138

[elalish]

Describe the bug

I've got two fidelity test failures on upgrading model-viewer to r138.2: See the .pngs I uploaded in the PR for how the renders changed. The SpecGloss extension has lightened considerably, and we're now failing most of the khronos-TextureSettingsTest. @donmccurdy @Mugen87 Any thoughts on what PRs might have affected this? I'm happy to help, but hoping you might have a pointer first.

[mrdoob]

You've upgraded from r136 to r138.
Can you try upgrading to r137 instead so we can narrow down the issue?

[elalish]

I should probably just bisect three...

[elalish]

Okay, git bisect pointed to the #23420 as what broke khronos-TextureSettingsTest, but I haven't looked into why yet. @takahirox Any thoughts? I'm going to check the other one now.

[elalish]

Okay, and the other git bisect points to #23129, which makes sense since it has the look of a wrong sRGB conversion for the spec gloss texture, @Mugen87.

[elalish]

Here are the changed renderers in case you don't want to dig in my PR:

[elalish]

I'm done for today; if no one has looked into these tomorrow I'll start debugging.

[Mugen87]

Okay, and the other git bisect points to #23129, which makes sense since it has the look of a wrong sRGB conversion for the spec gloss texture, @Mugen87.

The removal of the inline sRGB decode was done under the assumption the color space for the texture is properly defined. It looks like this is not the case.

[donmccurdy]

About the failing texture clamp/wrap/repeat test, I've noticed that if the internal cache here is commented out...

three.js/examples/jsm/loaders/GLTFLoader.js

Lines 2905 to 2917 in c5e0fdf

	if ( this.sourceCache[ sourceIndex ] !== undefined ) {
	return this.sourceCache[ sourceIndex ].then( function ( texture ) {
	return texture.clone();
	} ).catch( function ( error ) {
	throw error;
	} );
	}

... the test cases all pass. So I think the changes by @takahirox in the PR are correct, but there's something about sharing a Source that prevents reuse with different clamp/wrap/repeat settings?

TextureSettingsTest.glb.zip

[Mugen87]

Actually, GLTFLoader just misses to set Texture.needsUpdate to true after the clone() operation.

[elalish]

Thanks for figuring these out @Mugen87! @mrdoob any chance we could see #23630 and #23637 in an r138.3? Sorry I didn't find these sooner; I thought I had run this test last week, but apparently I forgot to push...

[Mugen87]

@mrdoob Please also include #23644 and #23650 in r138.3 😇 .

[mrdoob]

Done!

[mrdoob]

@elalish 0.138.3 is out.

[elalish]

I've merged r138.3 and it's looking good 👍