Add support for material variants #4528

GSterbrant · 2022-08-10T14:39:26Z

Description

Adds support for KHR_materials_variants as well as provides an API for changing the material. Also fixes the default material index ownership, having them live in the container instead of on the mesh.

Fixes #3647

API changes

Adds getMaterialVariants to GlbContainerResource. This API fetches the list of variants present in the GLB.
Adds applyMaterialVariant(name, entity) to GlbContainerResource. This API applies a material variant to all mesh instances in the render component of an entity.
Adds applyMaterialVariantInstances(name, instances) to GlbContainerResource. This API applies a material variant to a set of mesh instances, using the GlbContainerResource as the source of the variants, their mappings, and the materials.

mvaligursky · 2022-08-10T14:41:33Z

Could you please add an overview of the new public API to the PR description.

src/resources/parser/glb-container-resource.js

src/resources/parser/glb-parser.js

src/resources/parser/glb-container-resource.js

src/resources/parser/glb-parser.js

src/resources/parser/glb-container-resource.js

src/resources/container.js

mvaligursky · 2022-08-16T16:17:22Z

src/resources/parser/glb-container-resource.js

+    }
+
+    // reset material variants on entity
+    _resetMaterialVariant(entity) {


I think I'd get rid of these _reset functions, as its the same code as the function above apart from the material selection - just make a small change to that function to handle it and remove the code duplication?

The API lives entirely in the GLB container, allowing the user to get the list of variants and apply it to a mesh. The variants are parsed, and the data for the variants are stored on the resource level of the container, thus avoiding any intrusion into any engine types.

…he material itself.

…ire entity.

…rnally, as well as debug warn if the variant can't be found.

…t slices.

…riant is applied as well. Also fixed lint.

Also provide documentation clarifying that the name is optional, and if not provided resets the material variants.

mvaligursky · 2022-08-17T08:27:38Z

src/resources/parser/glb-container-resource.js

+                const meshVariants = this.data.meshVariants[instance.mesh.id];
+                if (meshVariants) {
+                    instance.material = this.data.materials[meshVariants[variant]];
+                    Debug.assert(instance.material);


if you could move the assert outside of if/else, so that it validates both cases.

mvaligursky

approved with a single small comment.

GSterbrant requested review from willeastcott, slimbuck and mvaligursky August 10, 2022 14:39

GSterbrant self-assigned this Aug 10, 2022

GSterbrant added the enhancement label Aug 10, 2022

GSterbrant added the area: graphics Graphics related issue label Aug 10, 2022

mvaligursky reviewed Aug 10, 2022

View reviewed changes

src/resources/parser/glb-container-resource.js Show resolved Hide resolved

mvaligursky reviewed Aug 10, 2022

View reviewed changes

src/resources/parser/glb-container-resource.js Outdated Show resolved Hide resolved

mvaligursky reviewed Aug 10, 2022

View reviewed changes

src/resources/parser/glb-container-resource.js Outdated Show resolved Hide resolved

slimbuck reviewed Aug 11, 2022

View reviewed changes

src/resources/parser/glb-parser.js Outdated Show resolved Hide resolved

src/resources/parser/glb-container-resource.js Show resolved Hide resolved

GSterbrant force-pushed the gsterbrant_materials_variants branch from e5c7d06 to 632755d Compare August 11, 2022 10:58

GSterbrant requested review from mvaligursky and slimbuck August 11, 2022 11:08