Restore initial material #2764
Restore initial material #2764
Conversation
…aterial back its initial setting.
ghost
requested a review
from 
| this[$defaultMaterialIdx] = materialRef.index; | ||
| } else { | ||
| console.error( | ||
| `Primitive (${mesh.name}) missing default material reference.`); |
There was a problem hiding this comment.
Is this possible? If no material was specified, we should have created a default material and then reference that, right? If it's just an assert, that's fine, I'm just curious.
There was a problem hiding this comment.
Correct the default material works and I don't think it would be possible for this to be null, at least not with todays codebase.
| } | ||
| expect(materials.find((material: MeshStandardMaterial) => { | ||
| return material.name === 'purple'; | ||
| })).to.not.be.null; | ||
| }); | ||
|
|
||
| test('Primitive switches to default material', async () => { |
There was a problem hiding this comment.
This is good, but it's not a public method. Let's also include a more end-to-end version that sets it via the public variantName API. And let's also update the documentation on that.
There was a problem hiding this comment.
Added a test using the variantName API, a bit tricky to verify without being able to access a gltf-primitive mapping on the model, have a look, updating the docs next...
Adds a 'variant-applied' event to give the all-done signal
| @@ -47,7 +47,7 @@ interface SceneExportOptions { | |||
|
|
|||
| export interface SceneGraphInterface { | |||
| readonly model?: Model; | |||
| variantName: string|undefined; | |||
| variantName: string|null|undefined; | |||
There was a problem hiding this comment.
I don't think you need undefined here anymore. You also need to change its type in the @property declaration below; probably best to initialize it to null.
| @@ -149,8 +149,10 @@ export const SceneGraphMixin = <T extends Constructor<ModelViewerElementBase>>( | |||
| return; | |||
| } | |||
|
|
|||
| await this[$model]; | |||
| await this[$model]; | |||
There was a problem hiding this comment.
Do you need the !s after variantName? Also == 'null' seems pretty weird? Can you just pass variantName in as is?
There was a problem hiding this comment.
passing variantName as is, it's possible, this API responds to the actual value being null though, not a magical variant-name 'null' that's why this is here. Also if you assign the value null to the property variantName Lit apparently converts that into a string, don't know why, that's why I'm checking for a string at all.
Updates docs, adds a 'reset' option to the Variant switching example.
| { | ||
| "name": "variantName", | ||
| "htmlName": "variantName", | ||
| "description": "Corresponding property of the attribute <code>variant-name</code>.", |
There was a problem hiding this comment.
This is not necessary (you'll notice none of our other attributes have property definitions, since all HTML attributes always have corresponding properties). That said, I'm open to changing the overall docs template to make that more clear.
There was a problem hiding this comment.
Wasn't sure if this was omitted on purpose or not, omitting it does assume that anyone can work out the fact that variant-name corresponds to a property called variantName.
| @@ -867,8 +867,8 @@ | |||
| "Attributes": [ | |||
| { | |||
| "name": "variant-name", | |||
| "htmlName": "variantName", | |||
| "description": "Selects a model variant by name.", | |||
| "htmlName": "variant-name", | |||
There was a problem hiding this comment.
Let's stick with our conventions here. Also, might be good to add that the default is null and options include variant name string or null, in the default json.
There was a problem hiding this comment.
Is the "htmlName" supposed to be visible somewhere on the documentation page? I don't see it.
| let defaultIndex = 0; | ||
| // Patches the first primitive with variant-info and a default | ||
| // material index of 0. | ||
| // let defaultIndex = 0; |
| @@ -149,8 +149,10 @@ export const SceneGraphMixin = <T extends Constructor<ModelViewerElementBase>>( | |||
| return; | |||
| } | |||
|
|
|||
| await this[$model]; | |||
| await this[$model]; | |||
There was a problem hiding this comment.
Did you see my whole comment on this line?
| element.variantName = null; | ||
| await waitForEvent(element, 'variant-applied'); | ||
| expect((primitiveNode!.mesh.material as MeshStandardMaterial).name) | ||
| .equal('purple'); |
Allows passing 'null' to enableVariant() API in order to switch the material back its initial setting.