EmissiveStrength and Clearcoat extensions #4319

diegoteran · 2023-06-13T21:58:35Z

Change from MeshStandardMaterial to MeshPhysicalMaterial

Feature: #4273

Change from MeshStandardMaterial to MeshPhysicalMaterial

diegoteran · 2023-06-13T21:59:30Z

Looking for feedback on the correct way to set up the API before completing other PBR next properties + unit tests.

elalish · 2023-06-13T22:35:46Z

packages/model-viewer/src/features/scene-graph/material.ts

+    this[$ensureMaterialIsLoaded]();
+    for (const material of this[$correlatedObjects] as
+         Set<MeshPhysicalMaterial>) {
+      material.emissiveIntensity = emissiveStrength;


LGTM, but I believe these are simplified compared to the existing setters right (recall our duplicated state)? We should change the others to match this style and check to ensure that doesn't break anything before we adopt it for these. Let's add that in before we get too deep into testing these new ones.

Single source of truth is now the three.js scene graph

elalish

Looking good, just a few things.

elalish · 2023-06-27T22:26:22Z

packages/model-viewer/src/features/scene-graph/image.ts

-    };
-    super(onUpdate, gltfImage, new Set<ThreeTexture>(texture ? [texture] : []));
+  get[$threeTextures](): Set<ThreeTexture> {
+    return this[$correlatedObjects] as Set<ThreeTexture>;


Were you still planning to rename $correlatedObjects?

I think the "correlated" name comes from the default correlation through the gltfMap from ThreeJS Loader:

model-viewer/packages/model-viewer/src/features/scene-graph/model.ts

Line 96 in 8546f41

if (correlatedMaterial != null) {

I'm not sure if the name should change.

elalish · 2023-06-27T22:27:31Z

packages/model-viewer/src/features/scene-graph/image.ts

+    super(onUpdate, new Set<ThreeTexture>(texture ? [texture] : []));
+
+    if (!this[$threeTexture].image.src) {
+      this[$threeTexture].image.src = 'adhoc_image' + adhocNum++;


Should this be the name as well, as before?

elalish · 2023-06-27T22:31:49Z

packages/model-viewer/src/features/scene-graph/material.ts

-          "KHR_materials_pbrSpecularGlossiness", please use
-          "pbrMetallicRoughness" instead. Specular Glossiness materials are
-          no longer supported; to convert to metal-rough, see 
-          https://www.donmccurdy.com/2022/11/28/converting-gltf-pbr-materials-from-specgloss-to-metalrough/.`);


I'm a little sad to loose this warning, but I think it's okay because three.js throws its own anyway.

elalish · 2023-06-27T22:32:58Z

packages/model-viewer/src/features/scene-graph/material.ts

-    return (this[$sourceObject] as DefaultedMaterial).emissiveFactor;
+    return (
+        this[$backingThreeMaterial]
+            .emissive.toArray() as [number, number, number]);


Does it not work with as RGB?

elalish · 2023-06-27T22:33:35Z

packages/model-viewer/src/features/scene-graph/material.ts

    this[$onUpdate]();
  }

+  [$getAlphaMode](): string {
+    // Follows implementation of GLTFExporter from three.js


elalish · 2023-06-27T22:35:15Z

packages/model-viewer/src/features/scene-graph/material.ts

    this[$onUpdate]();
  }

  getDoubleSided(): boolean {
    this[$ensureMaterialIsLoaded]();
-    return (this[$sourceObject] as DefaultedMaterial).doubleSided;
+    // 0 for FrontSide, 2 for DoubleSide.
+    return this[$backingThreeMaterial].side == 2;


Use the DoubleSided enum from three, or whatever it's called.

elalish · 2023-06-27T22:37:23Z

packages/model-viewer/src/features/scene-graph/model.ts

-    gltfSourceMaterial.name = newMaterialName;
-    // Adds the source material clone to the gltf def.
-    const gltf = this[$correlatedSceneGraph].gltf;
-    gltf.materials!.push(gltfSourceMaterial);


💯 Always nice to remove code!

elalish · 2023-06-27T22:38:37Z

packages/model-viewer/src/features/scene-graph/pbr-metallic-roughness.ts

+  private get[$threeMaterial]() {
+    console.assert(
+        this[$correlatedObjects] != null && this[$correlatedObjects]!.size > 0,
+        'Sampler correlated object is undefined');


Is this a Sampler?

ahh good copy-paste catch

elalish · 2023-06-27T22:42:26Z

packages/model-viewer/src/features/scene-graph/sampler.ts

@@ -49,29 +49,29 @@ const isWrapMode = (() => {
             wrapModes.indexOf(value as WrapMode) > -1;
 })();

-const isValidSamplerValue = <P extends 'minFilter'|'magFilter'|'wrapS'|'wrapT'|'rotation'|'repeat'|'offset'>(
-    property: P, value: unknown): value is DefaultedSampler[P] => {


I missed this value is thing before - I've never seen that syntax. I know it's not yours, but if there's a simpler way, that would be great. Seems like it should just be P, value: DefaultedSampler<P>): boolean =>

It says DefaultedSampler is not generic. Probably no support for
?

elalish · 2023-06-28T16:55:36Z

packages/model-viewer/src/features/scene-graph/image.ts

+    return this[$correlatedObjects] as Set<ThreeTexture>;
+  }
+
+  constructor(onUpdate: () => void, texture: ThreeTexture|null) {


I know this predates you, but do we know when |null happens? There's an assert above that seems to imply null would be an error. Doesn't have to happen in this PR, but it would be great to clean this up if we can.

Yes, or at least it should work the same way as https://github.com/google/model-viewer/blob/8546f41450af57ee37fe7c00e3ef210138f6e3a0/packages/model-viewer/src/features/scene-graph/sampler.ts#L80C2-L100C56

Maybe another refactor where we remove |null from all ThreeDOMElements (Sampler, Image, Texture, etc...) is something worth looking into.

Yes, I think that would be ideal.

elalish · 2023-06-28T16:57:39Z

packages/model-viewer/src/test/features/scene-graph/material-spec.ts

-    test('Accessing the name getter does not cause throw error.', async () => {
-      expect(model.materials[2].name).to.equal('red');
-      expect(model.materials[2][$lazyLoadGLTFInfo]).to.be.ok;
-    });


Can we leave this now that name is fixed?

elalish · 2023-06-28T16:58:33Z

packages/model-viewer/src/test/features/scene-graph/model-spec.ts

@@ -150,6 +150,7 @@ suite('scene-graph/model', () => {

      test('getMaterialByName returns material when name exists', async () => {
        await loadModel(CUBES_GLTF_PATH);
+        await model.materials[2].ensureLoaded();


Likewise, is this necessary anymore?

textureInfo, material, and threeDomElement still require them for Lazyloading (assigning the correlated amterial after the constructor)

elalish

This looks great, thanks for helping to clean up our code base!

…sions (google#4319) * EmissiveStrength and Clearcoat extensions Change from MeshStandardMaterial to MeshPhysicalMaterial * Add Clearcoat Normal Texture Scale * Remove canonical GLTF element from ThreeDOMElement Single source of truth is now the three.js scene graph * Cleanup for space-opera tests * Address review comments. * Reverted tests after adding name functionality * Remove unused `|null` in some ThreeDOMElements textureInfo, material, and threeDomElement still require them for Lazyloading (assigning the correlated amterial after the constructor)

EmissiveStrength and Clearcoat extensions

7beee02

Change from MeshStandardMaterial to MeshPhysicalMaterial

diegoteran requested a review from elalish June 13, 2023 21:59

Add Clearcoat Normal Texture Scale

c4eda51

elalish reviewed Jun 13, 2023

View reviewed changes

diegoteran added 2 commits June 23, 2023 14:08

Remove canonical GLTF element from ThreeDOMElement

706b6e3

Single source of truth is now the three.js scene graph

Cleanup for space-opera tests

cc7a6b6

diegoteran requested a review from elalish June 27, 2023 22:24

elalish requested changes Jun 27, 2023

View reviewed changes

Address review comments.

e14c82a

diegoteran requested a review from elalish June 27, 2023 23:16

elalish previously approved these changes Jun 28, 2023

View reviewed changes

Reverted tests after adding name functionality

e672cf5

diegoteran dismissed elalish’s stale review via e672cf5 June 28, 2023 17:47

diegoteran requested a review from elalish June 28, 2023 17:52

Remove unused |null in some ThreeDOMElements

10eadec

textureInfo, material, and threeDomElement still require them for Lazyloading (assigning the correlated amterial after the constructor)

elalish approved these changes Jun 28, 2023

View reviewed changes

diegoteran merged commit ddef677 into master Jun 28, 2023
3 checks passed

diegoteran deleted the pbr branch June 28, 2023 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EmissiveStrength and Clearcoat extensions #4319

EmissiveStrength and Clearcoat extensions #4319

diegoteran commented Jun 13, 2023

diegoteran commented Jun 13, 2023

elalish Jun 13, 2023

elalish left a comment

elalish Jun 27, 2023

diegoteran Jun 27, 2023

elalish Jun 27, 2023

diegoteran Jun 27, 2023

elalish Jun 27, 2023

diegoteran Jun 27, 2023

elalish Jun 27, 2023

elalish Jun 27, 2023

elalish Jun 27, 2023

elalish Jun 27, 2023

elalish Jun 27, 2023

diegoteran Jun 27, 2023

elalish Jun 27, 2023

diegoteran Jun 27, 2023

elalish Jun 28, 2023

diegoteran Jun 28, 2023

elalish Jun 28, 2023

elalish Jun 28, 2023

elalish Jun 28, 2023

elalish left a comment

EmissiveStrength and Clearcoat extensions #4319

EmissiveStrength and Clearcoat extensions #4319

Conversation

diegoteran commented Jun 13, 2023

diegoteran commented Jun 13, 2023

Choose a reason for hiding this comment

elalish 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

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

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

elalish left a comment

Choose a reason for hiding this comment