EmissiveStrength and Clearcoat extensions by diegoteran · Pull Request #4319 · google/model-viewer

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

+    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

-    };
-    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

+    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

-          "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

-    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

  }

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


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

    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

-    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

+  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

 })();

-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

+    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

-    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


      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 suggested 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

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

Conversation

diegoteran commented Jun 13, 2023

Uh oh!

diegoteran commented Jun 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elalish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elalish left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants