GLTFExporter: Multi-material support #13536

takahirox · 2018-03-08T19:00:06Z

This PR adds multi-material support to GLTFExporter. I'm happy if you try with your scene because I haven't verified this change with many enough scenes yet.

The exporter exports a mesh with multi-material as a mesh including the primitives which share the same attributes but have distinct material and indices.

Note that GLTFLoader doesn't build a mesh with multi-material from it yet but builds multi meshes in a group. We should update and enable GLTFLoader to handle multi-material, too.

donmccurdy · 2018-03-08T19:07:48Z

Nice! Haven't had the chance to review in detail but will do that soon.

We should update and enable GLTFLoader to handle multi-material, too.

I have a PR in progress elsewhere for letting GLTFLoader handle multi-material meshes as a single Mesh instance. :)

takahirox · 2018-03-08T19:15:09Z

Cool!

a mesh including the primitives which share the same attributes but have distinct material and indices.

Can the loader with your PR build a single mesh with multi-material from this data structure? Let me know if you have any requests to this PR for handling multi-material in the loader.

donmccurdy · 2018-03-08T19:31:21Z

Exactly that, yes, the structure looks right. Although it depends on BufferGeometryUtils.mergeBufferGeometries(). So it would sort of be a conditional upgrade when that utility is present.

donmccurdy · 2018-03-10T21:20:55Z

examples/js/exporters/GLTFExporter.js

+				start = Math.max( start, geometry.drawRange.start );
+				count = Math.min( end, end2 ) - start;
+
+				if ( count < 0 ) count = 0;

 			}


Slightly related question for @fernandojsg — in what situations would the truncateDrawRange option be used?

@donmccurdy in apps where you are creating a very big buffer but just fill a small part of it (eg: A-Painter) you would like to export just the buffer that actually contains real data, not the whole buffer filled with zeros.

donmccurdy · 2018-03-10T21:23:49Z

examples/js/exporters/GLTFExporter.js

 			var bufferViewTarget;

 			// If geometry isn't provided, don't infer the target usage of the bufferView. For
 			// animation samplers, target must not be set.
 			if ( geometry !== undefined ) {

-				var isVertexAttributes = componentType === WEBGL_CONSTANTS.FLOAT;
-				bufferViewTarget = isVertexAttributes ? WEBGL_CONSTANTS.ARRAY_BUFFER : WEBGL_CONSTANTS.ELEMENT_ARRAY_BUFFER;
+				bufferViewTarget = attribute === geometry.index ? WEBGL_CONSTANTS.ELEMENT_ARRAY_BUFFER : WEBGL_CONSTANTS.ARRAY_BUFFER;


~~This doesn't look right — ELEMENT_ARRAY_BUFFER should be used as the target for the index attribute, but not for vertex attributes of indexed geometry.~~ Oops, I misread. 😑

donmccurdy · 2018-03-10T21:28:54Z

examples/js/exporters/GLTFExporter.js

+			if ( geometry.index === null && options.forceIndices ) {
+
+				var numFaces = geometry.attributes.position.count;
+				var indices = new Uint32Array( numFaces );


nit: this should probably be called numIndices rather than numFaces

I prefer

var indices = new Uint32Array( geometry.attributes.position.count ); for ( var i = 0, il = indices.length; i < il; i ++ ) { indices[ i ] = i; }

because we don't need to mind var name hehe.

Updated, thanks!

donmccurdy · 2018-03-10T21:45:21Z

examples/js/exporters/GLTFExporter.js

+				var primitive = {
+					mode: mode,
+					attributes: attributes,
+				};


Interesting interesting.... this is not how I expected primitives to be used 😅, but it looks totally valid too. I was thinking that each primitive would have different accessors, based on the start/count subslice of the mesh's attributes. And then in GLTFLoader, we would have to concatenate them all back together.

What you're doing seems more efficient with indexed geometry, or if you want to re-draw the entire mesh with different materials. But it also assumes the loader does not do what I was planning, concatenating all of the accessors. Or if forceIndices had been false, and each group had different draw range, that's not handled here.

If you look at the BrainStem.gltf model, it has a bunch of primitives, all with different accessors. I think we should start an issue on the glTF repo to mention these different use cases and make sure they're all OK.

Opened: KhronosGroup/glTF#1278

Agreed with discussing best practive in glTF thread.

I'm sure this style is very fitting to Three.js structure and requires less changes. But yea, I've missed no-index case... Hm...

Added an temporal workaround so far for no-indexed geometry, force index for no-indexed geometry with multi-material. I don't think this's a good solution so just temporal.

But it also assumes the loader does not do what I was planning, concatenating all of the accessors.

Hm... probably I guess GLTFLoader ends up supporting both shared primitive accessors and different primitive accessors because the both are valid (depending on the discussion in glTF thread tho).

GLTFLoader should be fine for now, since it still creates a mesh for each primitive. At least it will reuse the same Geometry in this case.

Yup. I think this change is ok once I fix materials loop I commented below. The exporter exports valid glTF and I confirmed GLTFLoader and FB can load.

So my plan is we merge this change for now, and make another PR switching shared accessors to different accessors if necessary.

Looks good! 👍

donmccurdy · 2018-03-11T22:45:49Z

examples/js/exporters/GLTFExporter.js

+			if ( ! forceIndices && isMultiMaterial ) {
+
+				// temporal workaround.
+				console.warn( 'THREE.GLTFExporter: Force index for a mesh with multi-material.', mesh );


Let's change this message to:

THREE.GLTFExporter: Creating index for non-indexed multi-material mesh.

("Force" sounds like a we are telling the user to do something, rather than telling them what we are doing)

And, right now this message will appear if the mesh is already indexed.. let's hide the message in that case.

Oops, I've missed adding geometry.index === null.

Updated, thanks.

donmccurdy · 2018-03-11T22:57:32Z

examples/js/exporters/GLTFExporter.js

+			var materials = isMultiMaterial ? mesh.material : [ mesh.material ] ;
+			var groups = isMultiMaterial ? mesh.geometry.groups : [ { materialIndex: 0, start: undefined, count: undefined } ];
+
+			// assuming materials.length === groups.length


Let's add some more detail in this comment, since it might not be obvious based on my spec questions below. Suggested:

Export one primitive for each material on the mesh. Only a subset of cases are currently handled:

[×] Single material, no geometry groups

[×] N materials, N geometry groups each using full draw range

[ ] N materials, N geometry groups with different draw ranges

[ ] N materials, M > 1 geometry groups

^ If that looks right? I'm trying to list cases that are valid in three.js, but not everything that glTF might allow.

Oops, this logic isn't correct. I should loop with groups and we don't need to assume materials.length === groups.length.

for ( var i = 0, il = groups.length; i < il; i ++ ) {

I'll update. With the update, this'll be fine

[ ] N materials, M > 1 geometry groups

BTW, I think this's ok even without the update.

[ ] N materials, N geometry groups with different draw ranges

I'm not really sure if I understand correctly that item, would you please explain with example?

Updated, thanks.

donmccurdy · 2018-03-11T22:59:48Z

examples/js/exporters/GLTFExporter.js

+				var primitive = {
+					mode: mode,
+					attributes: attributes,
+				};


GLTFLoader should be fine for now, since it still creates a mesh for each primitive. At least it will reuse the same Geometry in this case.

donmccurdy · 2018-03-12T15:42:19Z

examples/js/exporters/GLTFExporter.js

+				start = Math.max( start, geometry.drawRange.start );
+				count = Math.min( end, end2 ) - start;
+
+				if ( count < 0 ) count = 0;


quick thought, no changes needed for this PR: the code here would be a bit simpler if we didn't have to think about .groups and .drawRange separately... might be interesting to look at implementing setDrawRange differently:

setDrawRange: function ( start, count ) { this.clearGroups(); this.addGroup( start, count, 0 ); },

Hm, for me .groups would be more like model-author-defined for multi-material while .drawRange is runtime user controllable (maybe?) so better not to combine? For example, imagine user wanna narrow drawRange of multi-material mesh runtime for little time and then reset the range. Can't reset original .groups info because it has been lost if we combine.

Yeah, I think if we combined them it would be with the eventual goal of deprecating setDrawRange() entirely... it's not clear to me what happens when .drawRange and .groups are both set, so merging might just make things clearer. But just an idea, I don't have much preference on this.

it's not clear to me what happens when .drawRange and .groups are both set

You can try mesh.geometry.setDrawRange(start, count) on console on mmd example

https://threejs.org/examples/webgl_loader_mmd.html

This model has multi-material and groups.

donmccurdy · 2018-03-12T15:46:51Z

examples/js/exporters/GLTFExporter.js

+
+				}
+
+				geometry.index = new THREE.Uint32BufferAttribute( indices, 1 );


minor: is it preferred to use geometry.setIndex( foo )?

Oh, I didn't know .setIndex() method exists. Updated, thanks.

I haven't replaced index reference with .getIndex() because it seems .getIndex() isn't used even in core.

donmccurdy · 2018-03-12T15:50:42Z

examples/js/exporters/GLTFExporter.js

+
+			}
+
+			var forcedIndex = false;


minor: just to avoid confusing forceIndices and forcedIndex, could you rename this one to didForceIndices or something like that?

Updated, thanks.

donmccurdy · 2018-03-12T18:38:18Z

Nice work! 👏

mrdoob · 2018-03-14T01:09:46Z

Thanks!

GLTFExporter: Multi-material support

7030a46

takahirox mentioned this pull request Mar 9, 2018

GLTFExporter #11951

Closed

87 tasks

donmccurdy reviewed Mar 10, 2018

View reviewed changes

donmccurdy mentioned this pull request Mar 10, 2018

Intended use(s) of mesh primitives KhronosGroup/glTF#1278

Open

takahirox added 3 commits March 11, 2018 22:09

GLTFExporter: Clean up forceIndices

b14f706

GLTFExporter: Add workaround for no-index geometry with multi-material

fe70963

GLTFExporter: Clean up

7983714

donmccurdy reviewed Mar 11, 2018

View reviewed changes

donmccurdy mentioned this pull request Mar 12, 2018

GLTFExporter GLB compatible with Facebook viewer #13397

Closed

5 tasks

takahirox added 2 commits March 12, 2018 23:11

GLTFExporter: Add geometry.index === null check for createing index

3975936

GLTFExporter: Fix loop for groups in geometry

7ac5b2e

donmccurdy reviewed Mar 12, 2018

View reviewed changes

takahirox added 2 commits March 13, 2018 03:09

GLTFExporter: Rename foceIndex with didForceIndices.

b35b2d4

GLTFExporter: Use BufferGeometry.setIndex()

2d1a897

donmccurdy approved these changes Mar 12, 2018

View reviewed changes

mrdoob added this to the r91 milestone Mar 14, 2018

mrdoob merged commit 37ae761 into mrdoob:dev Mar 14, 2018

This was referenced Mar 21, 2018

enhancement: merge groups and setDrawRange() #7277

Closed

GLTFLoader: MultiMaterial support #13707

Merged

donmccurdy mentioned this pull request Mar 2, 2019

GLTFLoader: Remove multipass and primitive merging logic #15889

Merged

Mugen87 mentioned this pull request Nov 28, 2023

GLTFExporter: Restore multi material support for non-indexed geometries. #27268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GLTFExporter: Multi-material support #13536

GLTFExporter: Multi-material support #13536

takahirox commented Mar 8, 2018 •

edited

Loading

donmccurdy commented Mar 8, 2018

takahirox commented Mar 8, 2018 •

edited

Loading

donmccurdy commented Mar 8, 2018

donmccurdy Mar 10, 2018

fernandojsg Mar 11, 2018

donmccurdy Mar 10, 2018 •

edited

Loading

donmccurdy Mar 10, 2018 •

edited

Loading

takahirox Mar 10, 2018

takahirox Mar 11, 2018

donmccurdy Mar 10, 2018

donmccurdy Mar 10, 2018

takahirox Mar 10, 2018 •

edited

Loading

takahirox Mar 11, 2018

takahirox Mar 11, 2018

donmccurdy Mar 11, 2018

takahirox Mar 11, 2018 •

edited

Loading

donmccurdy Mar 12, 2018

donmccurdy Mar 11, 2018

takahirox Mar 11, 2018

takahirox Mar 12, 2018

donmccurdy Mar 11, 2018

takahirox Mar 11, 2018 •

edited

Loading

takahirox Mar 12, 2018

donmccurdy Mar 11, 2018

donmccurdy Mar 12, 2018

takahirox Mar 12, 2018 •

edited

Loading

donmccurdy Mar 12, 2018 •

edited

Loading

takahirox Mar 12, 2018

donmccurdy Mar 12, 2018

takahirox Mar 12, 2018

donmccurdy Mar 12, 2018

takahirox Mar 12, 2018

donmccurdy commented Mar 12, 2018

mrdoob commented Mar 14, 2018


		}

		geometry.index = new THREE.Uint32BufferAttribute( indices, 1 );

GLTFExporter: Multi-material support #13536

GLTFExporter: Multi-material support #13536

Conversation

takahirox commented Mar 8, 2018 • edited Loading

donmccurdy commented Mar 8, 2018

takahirox commented Mar 8, 2018 • edited Loading

donmccurdy commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

donmccurdy Mar 10, 2018 • edited Loading

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

takahirox Mar 10, 2018 • edited Loading

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

takahirox Mar 11, 2018 • edited Loading

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

takahirox Mar 11, 2018 • edited Loading

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

takahirox Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

donmccurdy Mar 12, 2018 • edited Loading

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

donmccurdy commented Mar 12, 2018

mrdoob commented Mar 14, 2018

takahirox commented Mar 8, 2018 •

edited

Loading

takahirox commented Mar 8, 2018 •

edited

Loading

donmccurdy Mar 10, 2018 •

edited

Loading

donmccurdy Mar 10, 2018 •

edited

Loading

takahirox Mar 10, 2018 •

edited

Loading

takahirox Mar 11, 2018 •

edited

Loading

takahirox Mar 11, 2018 •

edited

Loading

takahirox Mar 12, 2018 •

edited

Loading

donmccurdy Mar 12, 2018 •

edited

Loading