GLTFLoader: Remove onBeforeRender dependency #14099

pailhead · 2018-05-20T03:29:23Z

Per conversation with @mrdoob.

Refactor GLTFLoader to remove the onBeforeRender dependency with onBeforeCompile dependency. It seems to make more sense to have the material manage the uniforms, rather than a scene graph node.

Ideally i would not use onBeforeCompile but an alternate approach where different systems of the WebGLRenderer could be leveraged. But this is a step.

The linter should maybe be run in this file, as it's got a lot of complaints in the dev branch.

ping @takahirox @donmccurdy

WestLangley · 2018-05-20T04:51:22Z

extend StandardMaterial with onBeforeCompile

Yes, I am inclined to agree that doing so is better than creating a custom ShaderMaterial, but it is still a bit tedious.

pailhead · 2018-05-20T06:15:36Z

I agree, however, I mustered up an API example that is slightly less tedious and better suited for automation. #14031.

Unfortunately i didn't realize that there were lint issues with the GLTF loader so when i linted the entire file, it made the diff look scary. But this example should be cleaner.

Slightly cleaner PR to look at might be #13198

I think for the time being, it's a way to leverage more of the existing code, remove some redundancy and and decouple some concerns within the existing system. onBeforeCompile is a way to do so currently available.

examples/js/loaders/GLTFLoader.js

donmccurdy · 2018-05-20T06:23:03Z

examples/js/loaders/GLTFLoader.js

+					shader.fragmentShader = shader.fragmentShader.replace( '#include <metalnessmap_fragment>', params.chunks.glossinessMapFragmentChunk );
+					shader.fragmentShader = shader.fragmentShader.replace( '#include <lights_physical_fragment>', params.chunks.lightPhysicalFragmentChunk );
+
+				};


Would this still work if we extend MeshStandardMaterial with a new type, GLTFMeshStandardSGMaterial, and set GLTFMeshStandardSGMaterial.prototype.onBeforeCompile?

function GLTFMeshStandardSGMaterial (params) { MeshStandardMaterial.call(this, params); material.userData.extraUniforms = { KHR_MATERIALS_PBR_SPECULAR_GLOSSINESS: params.uniforms }; }

It would be nice to be able to return a normal constructor from getMaterialType, or at minimum to have an .isGLTFMeshStandardSGMaterial===true flag on the material for GLTFExporter to find.

I'm not sure about the prototype, i think it might have to be bound in the constructor. This is one of those gotchas that i always have to console.log. I don't think onBeforeCompile exists on the prototype object at all.

Ideally i would like to not have to keep onBeforeCompile for too long in lieu of material includes. But for now, if this works, yes i think a new class is the best way to handle this.

donmccurdy · 2018-05-21T04:48:20Z

examples/js/loaders/GLTFLoader.js

+					glossiness: { value: 0.5 },
+					specularMap: { value: null },
+					glossinessMap: { value: null }
+				};


I realize this isn't new with your changes, but let's use the defaults from the extension spec:

specular: 0xffffff
glossiness: 1.0

https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_materials_pbrSpecularGlossiness

Will do, if this looks good i can proceed to clean this diff up a bit. This is being set to that default somewhere else though, but that's probably cruft, it should be here. I also realized i haven't actually removed the onBeforeRender, there should be no mention of it in this file.

examples/js/loaders/GLTFLoader.js

takahirox · 2018-05-22T18:50:45Z

Nice work.

I don't really like the current specular-glossiness material implementation, especially refreshUniforms. I'm sure we should remove duplicated refreshUniforms from GLTFLoader.

I tried onBeforeCompile, too, to simplify the design but I gave up because it can't handle additional uniforms. Piping the value from material to uniforms with setter/getter would be a good solution to solve this issue. (Let me know if there's any concerns about it)

The current implementation has another issue. refreshUniforms is bound to mesh.onBeforeRender, not material. When user wants to share the material with other mesh, one needs to be ware of the necessity of copying .onBeforeRender.

mesh2.material = mesh.material;
mesh2.onBeforeRender = mesh.onBeforeRender;

This solution can also solve this issue.

pailhead · 2018-05-22T19:48:12Z

It's as tricky to clone something with onBeforeCompile as it is with onBeforeRender but at least the responsibility is in a more appropriate realm (material manages uniform, not an Object3D). I'll clean this up today or tomorrow.

I do have concerns though about onBeforeCompile i don't like how it behaves nor how tedious it is to use it. GLTFLoader happened to be a really good example where shader injection in general can better solve the problem, BUT it doesn't have to be with onBeforeCompile. This IMO is a superior way:
#13198

Would you be open to giving this other API some support after this lands?

pailhead · 2018-05-26T00:23:41Z

examples/js/loaders/GLTFLoader.js

@@ -728,15 +759,20 @@ THREE.GLTFLoader = ( function () {
 				material.glossinessMap = params.glossinessMap === undefined ? null : params.glossinessMap;
 				material.glossiness = params.glossiness;

+				if ( material.glossinessMap ) {


im not sure what to do about this

pailhead · 2018-05-26T00:48:33Z

Mkay, i took a stab at this hack for the roughness thing. I think with the alternative to onBeforeCompile this could be simplified. This feels hacky but works, should possibly set needsUpdate to true, but it would make it into a pretty side-effecty setter.

With this code:

var clone = object.clone();
clone.position.set(.025,0,0);
scene.add(clone);
clone.children[0].material = clone.children[0].material.clone()
console.log(clone.children[0].material)
clone.children[0].material.color.setStyle(`#ff0000`)

I was able to achieve this with spec gloss.

It would be cool if someone reviewed this, since this may be the most I could pick up, i'm not that fmiliar with the rest of the loader. There might also be some leftover cruft.

@takahirox @donmccurdy

pailhead · 2018-05-26T01:08:06Z

And maybe link this here:
#14009

I'm not a CS person, but i'm trying to learn, i've been curious about the callback, why it makes sense, why it doesn't. I'm not sure where this implementation falls.

pailhead · 2018-07-18T05:14:27Z

The further this is pushed out the more work it causes. Conflicts were solved already.

donmccurdy

Agreed that this improves the loader.

I want to test this on a few models still, but would like to work toward merging this.

donmccurdy · 2018-07-18T16:02:43Z

examples/js/loaders/GLTFLoader.js

+	 * @pailhead
+	 */
+
+	function SpecularGlossinessPbrMaterial( params ) {


I think we discussed possible names if a spec/gloss PBR material were added to three.js once before, and MeshStandardSGMaterial was the candidate? Perhaps let's use that.

donmccurdy · 2018-07-18T16:03:51Z

examples/js/loaders/GLTFLoader.js

+		].join( '\n' );
+
+		var uniforms = {
+			specular: { value: new THREE.Color().setHex( 0x111111 ) },


Not a new issue with your code I realize, but could you make this 0xFFFFFF to match the spec?

donmccurdy · 2018-07-18T16:04:22Z

examples/js/loaders/GLTFLoader.js

+
+		var uniforms = {
+			specular: { value: new THREE.Color().setHex( 0x111111 ) },
+			glossiness: { value: 0.5 },


Similarly, default in glTF should be 1.0 here.

donmccurdy · 2018-07-18T16:12:16Z

examples/js/loaders/GLTFLoader.js

+		this._extraUniforms = uniforms;
+
+		// please see #14031 or #13198 for an alternate approach
+		this.onBeforeCompile = function ( shader ) {


If we put onBeforeCompile on the SpecularGlossinessPbrMaterial prototype, does it still work? Having it support cloning would probably make sense in this case.

I’m not sure I think it won’t like the prototype. But cloning this should still work since it calls the constructor. I’ll rename these look at the conflict and see where it leads us. Thank you.

Ok, this looks good as-is.

pailhead · 2018-07-21T09:57:03Z

rebase yeey. With the messed up branch i had, it was even more fun. I think it's fixed now, and i've addressed the naming. @donmccurdy

donmccurdy

Looks good to me, thanks! 👍

You can also remove the now-unused cloneMaterial helper function.

donmccurdy · 2018-07-21T23:18:20Z

examples/js/loaders/GLTFLoader.js

+		this._extraUniforms = uniforms;
+
+		// please see #14031 or #13198 for an alternate approach
+		this.onBeforeCompile = function ( shader ) {


Ok, this looks good as-is.

pailhead · 2018-07-21T23:33:53Z

@donmccurdy

nuked cloneMaterial nice, shaved off more code

pailhead · 2018-07-21T23:35:16Z

examples/js/loaders/GLTFLoader.js

+
+		};
+
+		Object.defineProperties(


@donmccurdy

do you think it would be ok to add /*eslint-disable*/ for these lines, they're reaaaaly spaced out for getters and setters?

Perhaps use one-liners like get: function () { return uniforms.specularMap.value; }? I'd be fine with that yeah. 👍

pailhead · 2018-07-22T10:34:27Z

examples/js/loaders/GLTFLoader.js

+
+						uniforms.glossinessMap.value = v;
+						//how about something like this - @pailhead
+						if ( v ) {


@donmccurdy did you catch this? It's a little bit quirky i think, since needsUpdate is also probably needed. I see two issues here, the general pattern with three.js when refreshing shaders, and then these defines in particular.

Oh thanks for calling that out, and the comment about USE_ROUGHNESSMAP. Requiring needsUpdate seems fine / I don't have any better ideas. 👍

Excellent! Call it done for now :)

pailhead · 2019-07-23T01:19:00Z

Is this still relevant? I can fix the conflicts but sinceI've fixed them several times in the past 15 months, it'd be cool if one of these milestones were actually upheld :)

mrdoob · 2019-07-31T03:23:35Z

I'll let @takahirox and @donmccurdy decide if it's still relevant.

pailhead · 2019-07-31T18:28:36Z

Damn, sorry I didn’t know this would be such a tough one.

Mugen87 · 2019-12-18T11:15:45Z

I'll let @takahirox and @donmccurdy decide if it's still relevant.

Um, seems it's not. If this refactoring is still wanted, I suggest to create a new PR from scratch.

pailhead · 2019-12-18T19:34:07Z

Could it please be elaborated a bit what made this irrelevant and what should the new PR created from scratch contain? Is it just the conflicts? I think it was fine conflict wise during these R103 period and was approved but didn’t make it. Some pointers for the new PR if it’s needed at all would be useful :) Thank you!

@donmccurdy @takahiro

donmccurdy · 2019-12-18T19:42:30Z

I've approved the PR, as it meaningfully simplifies the current code — I have no concerns with the changes.

mrdoob · 2019-12-18T22:09:05Z

Is it just the conflicts?

Yes.

mrdoob · 2019-12-18T22:10:06Z

Also, I feel @takahirox no longer has time to review GLTFLoader related PRs.

pailhead · 2019-12-18T22:46:19Z

Sweet thank you for the info! I can look into this by the end of this week.

pailhead · 2019-12-19T04:51:15Z

It looks like i'm unable to reopen this though.

elalish · 2020-01-24T22:17:06Z

@pailhead Thanks for fixing my issue before it even appeared! I fixed your conflicts and your work was just merged at long last, so you're welcome to close this one. Sorry for the wait.

mrdoob · 2020-01-24T22:21:49Z

@elalish Merging your PR merged this PR 😀

WestLangley · 2020-01-25T04:27:56Z

@pailhead Thank you ! :-)

pailhead changed the title ~~Remove on before render dep gltf loader~~ Remove onBeforeRender dep gltf loader May 20, 2018

pailhead mentioned this pull request May 20, 2018

GLTFLoader: Some glTF models cannot be cloned #11573

Closed

12 tasks

donmccurdy reviewed May 21, 2018

View reviewed changes

takahirox reviewed May 21, 2018

View reviewed changes

examples/js/loaders/GLTFLoader.js Outdated Show resolved Hide resolved

mrdoob added this to the r94 milestone May 22, 2018

pailhead commented May 26, 2018

View reviewed changes

donmccurdy mentioned this pull request May 29, 2018

[WIP] GLTFLoader: Use node materials. #14149

Closed

This was referenced May 30, 2018

NodeMaterial #7522

Closed

[WIP] tiny api and example how to combine material extensions #14206

Closed

This was referenced Jun 7, 2018

Material shader extensions #14231

Closed

Alternative to .onBeforeCompile for material extension #14232

Closed

What are the plans regarding NodeMaterial? #14245

Closed

mrdoob modified the milestones: r94, r95 Jun 27, 2018

donmccurdy reviewed Jul 18, 2018

View reviewed changes

pailhead force-pushed the remove-onBeforeRender-dep-GLTFLoader branch from 6204e49 to c9b7607 Compare July 21, 2018 09:53

donmccurdy approved these changes Jul 21, 2018

View reviewed changes

pailhead commented Jul 21, 2018

View reviewed changes

pailhead commented Jul 22, 2018

View reviewed changes

donmccurdy changed the title ~~Remove onBeforeRender dep gltf loader~~ GLTFLoader: Remove onBeforeRender dep Jul 23, 2018

donmccurdy approved these changes Jul 23, 2018

View reviewed changes

mrdoob modified the milestones: r107, r108 Jul 31, 2019

mrdoob modified the milestones: r108, r109 Aug 27, 2019

mrdoob modified the milestones: r109, r110 Sep 23, 2019

mrdoob modified the milestones: r110, r111 Oct 30, 2019

mrdoob modified the milestones: r111, r112 Nov 27, 2019

Mugen87 closed this Dec 18, 2019

Mugen87 removed this from the r112 milestone Dec 18, 2019

mrdoob reopened this Dec 19, 2019

Mugen87 mentioned this pull request Jan 4, 2020

New MeshStandardMaterial in r112 has banding artifacts on some GPUs #18265

Closed

takahirox mentioned this pull request Jan 18, 2020

GLTFLoader: Introduce plugin system #18421

Closed

elalish mentioned this pull request Jan 24, 2020

Make specularGlossiness inherit from MeshStandardMaterial #18474

Merged

mrdoob merged commit 2443963 into mrdoob:dev Jan 24, 2020

GLTFLoader: Remove onBeforeRender dependency #14099

GLTFLoader: Remove onBeforeRender dependency #14099

Conversation

pailhead commented May 20, 2018 • edited

WestLangley commented May 20, 2018

pailhead commented May 20, 2018 • edited

Choose a reason for hiding this comment

pailhead May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pailhead May 21, 2018 • edited

Choose a reason for hiding this comment

takahirox commented May 22, 2018 • edited

pailhead commented May 22, 2018

Choose a reason for hiding this comment

pailhead commented May 26, 2018 • edited

pailhead commented May 26, 2018 • edited

pailhead commented Jul 18, 2018

donmccurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pailhead commented Jul 21, 2018 • edited

donmccurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pailhead commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Jul 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pailhead commented Jul 23, 2019

mrdoob commented Jul 31, 2019

pailhead commented Jul 31, 2019

Mugen87 commented Dec 18, 2019

pailhead commented Dec 18, 2019 • edited

donmccurdy commented Dec 18, 2019

mrdoob commented Dec 18, 2019

mrdoob commented Dec 18, 2019

pailhead commented Dec 18, 2019

pailhead commented Dec 19, 2019

elalish commented Jan 24, 2020

mrdoob commented Jan 24, 2020

WestLangley commented Jan 25, 2020

pailhead commented May 20, 2018 •

edited

pailhead commented May 20, 2018 •

edited

pailhead May 21, 2018 •

edited

pailhead May 21, 2018 •

edited

takahirox commented May 22, 2018 •

edited

pailhead commented May 26, 2018 •

edited

pailhead commented May 26, 2018 •

edited

pailhead commented Jul 21, 2018 •

edited

donmccurdy Jul 23, 2018 •

edited

pailhead commented Dec 18, 2019 •

edited