Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent the code to fail when loading models without the "materials" property or having a mesh primitive without the material association #3287

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/model-viewer/src/features/scene-graph/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class Model implements ModelInterface {
const {gltf, threeGLTF, gltfElementMap} = correlatedSceneGraph;
this[$threeScene] = threeGLTF.scene;

for (const [i, material] of gltf.materials!.entries()) {
for (const [i, material] of gltf.materials?.entries() ?? []) {
const correlatedMaterial =
gltfElementMap.get(material) as Set<MeshStandardMaterial>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,11 @@ export class PrimitiveNode extends Node {
// Captures the primitive's initial material.
const materialMappings =
threeObjectMap.get(mesh.material as ThreeMaterial)!;
if (materialMappings.materials != null) {
if (materialMappings?.materials != null) {
this[$initialMaterialIdx] = this[$activeMaterialIdx] =
materialMappings.materials;
} else {
console.error(
console.warn(
`Primitive (${mesh.name}) missing initial material reference.`);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const ASTRONAUT_GLB_PATH = assetPath('models/Astronaut.glb');
const KHRONOS_TRIANGLE_GLB_PATH =
assetPath('models/glTF-Sample-Models/2.0/Triangle/glTF/Triangle.gltf');
const CUBES_GLTF_PATH = assetPath('models/cubes.gltf');
const VASE_NO_MATERIALS_GLB_PATH = assetPath('models/VaseNoMaterials.glb');

suite('scene-graph/model', () => {
suite('Model', () => {
Expand Down Expand Up @@ -82,6 +83,13 @@ suite('scene-graph/model', () => {
expect(collectedMaterials.size).to.be.equal(materials.size);
});

test('should have no materials given a model without any materials defined', async () => {
const threeGLTF = await loadThreeGLTF(VASE_NO_MATERIALS_GLB_PATH);
const model = new Model(CorrelatedSceneGraph.from(threeGLTF));

expect(model.materials.length).to.be.eq(0);
});

suite('Model Variants', () => {
test('Switch variant and lazy load', async () => {
const threeGLTF = await loadThreeGLTF(CUBES_GLTF_PATH);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const CUBE_GLTF_PATH = assetPath('models/cube.gltf');
const MESH_PRIMITIVES_GLB_PATH = assetPath('models/MeshPrimitivesVariants.glb');
const KHRONOS_TRIANGLE_GLB_PATH =
assetPath('models/glTF-Sample-Models/2.0/Triangle/glTF/Triangle.gltf');
const VASE_NO_MATERIALS_GLB_PATH = assetPath('models/VaseNoMaterials.glb');

const findPrimitivesWithVariant = (model: Model, variantName: string) => {
const result = new Array<any>();
Expand Down Expand Up @@ -69,6 +70,29 @@ suite('scene-graph/model/mesh-primitives', () => {
});
});

suite('Primitive with no material', () => {
let element: ModelViewerElement;
setup(async () => {
element = new ModelViewerElement();
element.src = VASE_NO_MATERIALS_GLB_PATH;
document.body.insertBefore(element, document.body.firstChild);
await waitForEvent(element, 'load');
});

teardown(() => {
document.body.removeChild(element);
});

test('has a no material', async () => {
const model = element.model!;

expect(model[$primitivesList].length).to.equal(1);
expect(model.materials.length).to.equal(0);
expect(model[$primitivesList][0][$initialMaterialIdx]).to.be.undefined;
expect(model.materials[0]).to.be.undefined;
});
});

suite('Static Primitive Without Variant', () => {
let model: Model;
setup(async () => {
Expand Down
Binary file added packages/shared-assets/models/VaseNoMaterials.glb
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export class MaterialPanel extends ConnectedLitElement {
@internalProperty() isInterpolating: boolean = false;

@query('#material-container') panel!: HTMLDivElement;
@query('#material-placeholder') placeholder!: HTMLDivElement;
@query('me-color-picker#base-color-picker') baseColorPicker!: ColorPicker;
@query('me-slider-with-input#roughness-factor')
roughnessFactorSlider!: SliderWithInputElement;
Expand Down Expand Up @@ -525,10 +526,18 @@ export class MaterialPanel extends ConnectedLitElement {
}

get selectedMaterialIndex(): number {
return this.selectableMaterials[this.materialSelector.selectedIndex].index;
if (!this.materialSelector) {
return 0;
}

return this.selectableMaterials[this.materialSelector.selectedIndex]?.index ?? 0;
}

set selectedMaterialIndex(index: number) {
if (!this.materialSelector) {
return;
}

this.materialSelector.selectedIndex = index;
this.onSelectMaterial();
}
Expand Down Expand Up @@ -1099,6 +1108,10 @@ export class MaterialPanel extends ConnectedLitElement {
`;
}

get hasSelectableMaterials(): boolean {
return this.selectableMaterials?.length > 0;
}

updateSelectableMaterials() {
this.selectableMaterials = [];
for (const material of getModelViewer()!.model!.materials) {
Expand All @@ -1125,7 +1138,11 @@ export class MaterialPanel extends ConnectedLitElement {

render() {
return html`
<div id="material-container" style="display: none">
<div>
<div id="material-placeholder" style="display: ${!this.hasSelectableMaterials ? 'block' : 'none'}">
<pre>No materials</pre>
</div>
<div id="material-container" style="display: ${this.hasSelectableMaterials ? 'block' : 'none'}">
${this.renderVariantsTab()}
${this.renderEditVariantDialog()}
${this.renderCreateVariantDialog()}
Expand All @@ -1138,6 +1155,7 @@ export class MaterialPanel extends ConnectedLitElement {
${this.renderOcclusionTextureTab()}
${this.renderOtherTab()}
</div>
</div>
`;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {waitForEvent} from '../utils/test_utils.js';
const TEXTURE_CUBE_GLTF_PATH = '../base/shared-assets/models/textureCubes.gltf';
const CUBES_GLTF_PATH = '../base/shared-assets/models/cubes.gltf';
const TRIANGLE_GLTF_PATH = '../base/shared-assets/models/Triangle.gltf';
const VASE_NO_MATERIALS_GLB_PATH = '../base/shared-assets/models/VaseNoMaterials.glb';
const TEXTURE_PATH = 'base/shared-assets/models/ORM.png';

async function checkUpload(
Expand Down Expand Up @@ -75,20 +76,38 @@ describe('material panel test', () => {
document.body.removeChild(preview);
});

it('selector reflects materials in GLTF, including defaults where undefined',
async () => {
panel.selectedMaterialIndex = 0;
await panel.updateComplete;
expect(panel.selectedBaseColor).toEqual([1, 0, 1, 1]);
expect(panel.selectedRoughnessFactor).toEqual(0.2);
expect(panel.selectedMetallicFactor).toEqual(1);
describe('Given a model without any materials', () => {
beforeEach(async () => {
reduxStore.dispatch(dispatchGltfUrl(VASE_NO_MATERIALS_GLB_PATH));
await preview.loadComplete;
await panel.updateComplete;
});

panel.selectedMaterialIndex = 1;
await panel.updateComplete;
expect(panel.selectedBaseColor).toEqual([1, 1, 0, 1]);
expect(panel.selectedRoughnessFactor).toEqual(1);
expect(panel.selectedMetallicFactor).toEqual(1);
});
it('should display a placeholder', () => {
const placeholder = panel.shadowRoot?.getElementById('material-placeholder');
expect(placeholder!.style.display).not.toEqual('none');
});

it('should not show the material panel', () => {
const form = panel.shadowRoot?.getElementById('material-container');
expect(form!.style.display).toEqual('none');
});
});

it('selector reflects materials in GLTF, including defaults where undefined',
async () => {
panel.selectedMaterialIndex = 0;
await panel.updateComplete;
expect(panel.selectedBaseColor).toEqual([1, 0, 1, 1]);
expect(panel.selectedRoughnessFactor).toEqual(0.2);
expect(panel.selectedMetallicFactor).toEqual(1);

panel.selectedMaterialIndex = 1;
await panel.updateComplete;
expect(panel.selectedBaseColor).toEqual([1, 1, 0, 1]);
expect(panel.selectedRoughnessFactor).toEqual(1);
expect(panel.selectedMetallicFactor).toEqual(1);
});

it('Model with variants has visible variant selector', async () => {
reduxStore.dispatch(dispatchGltfUrl(CUBES_GLTF_PATH));
Expand Down