GLTFExporter: Prevent exporting empty geometry #13681

fernandojsg · 2018-03-25T01:39:33Z

I've just found a bug when exporting empty buffer geometries. eg in a-painter I create a buffer geometry for each kind of material but maybe I don't use one of them so drawRange will be 0. If you try to export it it will generate the following glTF:

{
  "asset": {
    "version": "2.0",
    "generator": "THREE.GLTFExporter"
  },
  "scenes": [
    {
      "nodes": [
        0
      ],
      "name": "AuxScene"
    }
  ],
  "scene": 0,
  "nodes": [
    {
      "name": "Custom buffered empty (drawrange)",
      "mesh": 0
    }
  ],
  "bufferViews": [
    {
      "buffer": 0,
      "byteOffset": 0,
      "byteLength": 0,
      "target": 34962,
      "byteStride": 12
    },
    {
      "buffer": 0,
      "byteOffset": 0,
      "byteLength": 0,
      "target": 34962,
      "byteStride": 12
    }
  ],
  "buffers": [
    {
      "byteLength": 0,
      "uri": "data:"
    }
  ],
  "accessors": [
    {
      "bufferView": 0,
      "componentType": 5126,
      "count": 0,
      "max": [
        null,
        null,
        null
      ],
      "min": [
        null,
        null,
        null
      ],
      "type": "VEC3"
    },
    {
      "bufferView": 1,
      "componentType": 5126,
      "count": 0,
      "max": [
        null,
        null,
        null
      ],
      "min": [
        null,
        null,
        null
      ],
      "type": "VEC3"
    }
  ],
  "materials": [
    {
      "pbrMetallicRoughness": {
        "metallicFactor": 0,
        "roughnessFactor": 0.9
      },
      "extensions": {
        "KHR_materials_unlit": {}
      },
      "doubleSided": true
    }
  ],
  "meshes": [
    {
      "primitives": [
        {
          "mode": 4,
          "attributes": {
            "POSITION": 0,
            "COLOR_0": 1
          },
          "material": 0
        }
      ]
    }
  ],
  "extensionsUsed": [
    "KHR_materials_unlit"
  ]
}

The accessors has an invalid range and the bufferviews are empty.

This PR prevents it to happens by skipping attributes that has no values, and primitives that has no attributes, and meshes that has no primitives.

I added a empty node and a button to test it. The expected output for the previous example will be:

{
  "asset": {
    "version": "2.0",
    "generator": "THREE.GLTFExporter"
  },
  "scenes": [
    {
      "nodes": [
        0
      ],
      "name": "AuxScene"
    }
  ],
  "scene": 0,
  "nodes": [
    {
      "name": "Custom buffered empty (drawrange)"
    }
  ]
}

donmccurdy · 2018-03-25T02:06:19Z

examples/js/exporters/GLTFExporter.js

@@ -484,6 +478,13 @@ THREE.GLTFExporter.prototype = {

 			}

+			// Skip creating an accessor if the attribute doesn't have data to export
+			if ( count === 0) {


nit: missing space after 0

donmccurdy · 2018-03-25T02:08:20Z

examples/js/exporters/GLTFExporter.js

@@ -1119,8 +1130,20 @@ THREE.GLTFExporter.prototype = {

 			}

+			if ( primitives.length === 0 ) {
+
+				return -1;


In processNode we're returning null when no node is created. Would probably vote for null over -1 personally but either way let's make it consistent.

donmccurdy · 2018-03-25T02:10:38Z

examples/misc_exporter_gltf.html

+				empty.name = 'Custom buffered empty (drawrange)';
+				scene1.add( empty );
+
+


What do you think about putting this case in GLTFExporter.tests.js instead? I think that might be a better fit than in the example.

Yeah! I was thinking about it actually, but I didn't know we already had a GLTFExporter.tests.js 👼 I'll take a look and include it there thanks!

takahirox · 2018-03-25T09:34:58Z

examples/js/exporters/GLTFExporter.js


 				}

-				if ( geometry.index !== null ) {
+				// Skip meshes without exportable attributes
+				if ( Object.keys(primitive.attributes).length > 0 ) {


You need spaces after and before primitive.attributes

Thanks! done

takahirox · 2018-03-25T09:43:39Z

examples/js/exporters/GLTFExporter.js

+				return null;
+
+			}
+


Adding

if ( Object.keys( attributes ).length === 0 ) { return null; }

after

for ( var attributeName in geometry.attributes ) { .... }

above instead isn't good?

Is there any use case where you could have just morph attributes but no other attribute?

IMO, no. At least, glTF validator seems to raise MESH_PRIMITIVE_MORPH_TARGET_NO_BASE_ACCESSOR "No base accessor for this attribute semantic." error if corresponding attribute isn't in attributes.

Yeah makes sense, added! thanks

Nice. So I think you can remove this check here now.

@mrdoob I believe he's referring to this #13681 (review) right? /cc @takahirox

No. I think we can remove here

if ( primitives.length === 0 ) { return null; }

because we added above

if ( Object.keys( attributes ).length === 0 ) { return null; }

Or you care the case where groups is an empty array? (In other words, how can primitives.length be zero here?)

You could have buffer geometries without groups eg: the sphere in the example https://github.com/mrdoob/three.js/blob/dev/examples/misc_exporter_gltf.html#L248 or a custom buffergeometry: https://github.com/mrdoob/three.js/blob/dev/examples/misc_exporter_gltf.html#L312-L337 but I haven't worked with multimaterial before so I'm not sure how that case would be handled here.
Because if you have one of the previous objects with a multimaterial.

var groups = isMultiMaterial ? mesh.geometry.groups : [ { materialIndex: 0, start: undefined, count: undefined } ];

groups would be mesh.geometry.groups that would be empty.

I confirmed rendering nothing for multi-material with empty groups.

So how about

if ( isMultiMaterial && mesh.geometry.groups.length === 0 ) return null;

it'd be more straightforward.

@takahirox thanks! done https://github.com/mrdoob/three.js/pull/13681/files#diff-6ae4adaab3880188935e6a1e30808c56R1073

fernandojsg · 2018-03-25T09:51:33Z

@donmccurdy added unit test, although I can't get it running, are the examples test broken? I believe we should include three build on them or am I missing something?

takahirox · 2018-03-25T10:29:27Z

examples/js/exporters/GLTFExporter.js


 				}

-				if ( geometry.index !== null ) {
+				// Skip meshes without exportable attributes
+				if ( Object.keys( primitive.attributes ).length > 0 ) {


You can remove this check here now

Nice catch! removed!

donmccurdy · 2018-03-26T14:01:59Z

@fernandojsg They should be able to run in the browser if you use npm run dev-test and then open http://localhost:8080/test/unit/UnitTests.html, I don't know about running them via CLI.

fernandojsg · 2018-03-26T16:39:20Z

@donmccurdy thanks! I've just fixed it and test passed ^_^

mrdoob · 2018-03-27T20:24:05Z

Thanks!

fernandojsg added 2 commits March 25, 2018 03:33

GLTFExporter: Prevent exporting empty geometry

a6bb8b2

GLTFExporter: Fix material if empty primitive

925ebcf

fernandojsg requested review from donmccurdy and takahirox March 25, 2018 01:39

GLTFExporter: change -1 for null when returning empty elements

071db54

donmccurdy reviewed Mar 25, 2018

View reviewed changes

takahirox reviewed Mar 25, 2018

View reviewed changes

fernandojsg added 3 commits March 25, 2018 11:39

GLTFExporter: fix linter

b037537

GLTFExporter: Remove empty geometry from example

94f26da

GLTFExporter: Add unit test for empty geometry

214435b

takahirox reviewed Mar 25, 2018

View reviewed changes

GLTFExporter: Add early exit if attributes not found in processMesh

bbf74b9

fernandojsg force-pushed the fixempty branch from 27c1c1b to bbf74b9 Compare March 25, 2018 15:08

GLTFExporter: Fix test

aab9953

donmccurdy approved these changes Mar 26, 2018

View reviewed changes

GLTFExporter: Prevent exporting multimaterial without groups

7eff795

takahirox approved these changes Mar 27, 2018

View reviewed changes

mrdoob added this to the r92 milestone Mar 27, 2018

mrdoob merged commit 66ee44e into mrdoob:dev Mar 27, 2018

fernandojsg deleted the fixempty branch March 27, 2018 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GLTFExporter: Prevent exporting empty geometry #13681

GLTFExporter: Prevent exporting empty geometry #13681

fernandojsg commented Mar 25, 2018 •

edited

Loading

donmccurdy Mar 25, 2018

fernandojsg Mar 25, 2018

donmccurdy Mar 25, 2018

donmccurdy Mar 25, 2018

fernandojsg Mar 25, 2018

takahirox Mar 25, 2018

fernandojsg Mar 25, 2018

takahirox Mar 25, 2018

fernandojsg Mar 25, 2018

takahirox Mar 25, 2018

fernandojsg Mar 25, 2018

takahirox Mar 25, 2018

fernandojsg Mar 27, 2018

takahirox Mar 27, 2018 •

edited

Loading

fernandojsg Mar 27, 2018

takahirox Mar 27, 2018 •

edited

Loading

fernandojsg Mar 27, 2018 •

edited

Loading

fernandojsg commented Mar 25, 2018

takahirox Mar 25, 2018 •

edited

Loading

fernandojsg Mar 25, 2018

donmccurdy commented Mar 26, 2018

fernandojsg commented Mar 26, 2018

mrdoob commented Mar 27, 2018

		empty.name = 'Custom buffered empty (drawrange)';
		scene1.add( empty );

GLTFExporter: Prevent exporting empty geometry #13681

GLTFExporter: Prevent exporting empty geometry #13681

Conversation

fernandojsg commented Mar 25, 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

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

Choose a reason for hiding this comment

takahirox Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

fernandojsg Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

fernandojsg commented Mar 25, 2018

takahirox Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Mar 26, 2018

fernandojsg commented Mar 26, 2018

mrdoob commented Mar 27, 2018

fernandojsg commented Mar 25, 2018 •

edited

Loading

takahirox Mar 27, 2018 •

edited

Loading

takahirox Mar 27, 2018 •

edited

Loading

fernandojsg Mar 27, 2018 •

edited

Loading

takahirox Mar 25, 2018 •

edited

Loading