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

GLTFLoader : Failure of combination Draco compression and multiple primitives #15039

Closed
3 of 13 tasks
ft-lab opened this issue Oct 12, 2018 · 8 comments
Closed
3 of 13 tasks
Assignees

Comments

@ft-lab
Copy link

ft-lab commented Oct 12, 2018

I'm sorry for sentences that are difficult to understand by automatic translation.

Hi,
I am developing glTF Importer/Exporter in C++.

With Draco compression and some combinations,
I found two problems in GLTFLoader.js.

I created a sample glb file for confirmation (Using my exporter).
I confirmed it with three.js(r97) and glTF Viewer ( https://gltf-viewer.donmccurdy.com/ ).

Case 1 : Draco compression + Share vertices data

Sample glb file :
https://ft-lab.github.io/gltf/apple/apple_cut_vert_share_draco.glb

The following conditions are specified.

  • Draco compression
  • Multiple primitives in one mesh
  • Share vertices data (position/normal/uv) of each primitive with a BufferView

However, in this case, Indices + all the vertices data are stored in Draco compressed BuferView, so waste increases ,,,

Result :
accessors is null.
I confirmed it with glTF Viewer.
threejs_r97_draco_share_vertices
Reference : https://ft-lab.github.io/gltf/apple/apple_cut_vert_share.glb
(Draco compression unused glb file)

Cause place :
js/loaders/GLTFLoader.js

When isMultiPass = true in GLTFParser.prototype.loadGeometries,
The number of array elements of primitives is 1.
In the case of Draco compression, I think that an error occurs because isMultiPass = true processing is not included.

Case 2 : Draco compression + Morph Targets

Sample glb file :
https://ft-lab.github.io/gltf/MorphTargets/multiplePrimitives_draco.glb

The following conditions are specified.

  • Draco compression
  • Multiple primitives in one mesh
  • Morph Targets

Result :
If the weight value of Morph Targets is not 0.0, some Primitives disappear.
threejs_r97_draco_morphtargets
Reference : https://ft-lab.github.io/gltf/MorphTargets/multiplePrimitives.glb
(Draco compression unused glb file)

Cause place :
js/loaders/GLTFLoader.js

Failure with drawElements.
In GLTFParser.prototype.loadGeometries,
In "if ( primitive.extensions && primitive.extensions[ EXTENSIONS.KHR_DRACO_MESH_COMPRESSION ] )",
I think that asynchronous processing in "var geometryPromise ... then", the primitive is not passed correctly.

It worked fine as follows.

In decodePrimitive function,

resolve( geometry );

-->

resolve( {'geometry' : geometry, 'primitive' : primitive} );

In GLTFParser.prototype.loadGeometries function,

var geometryPromise = extensions[ EXTENSIONS.KHR_DRACO_MESH_COMPRESSION ]
    .decodePrimitive( primitive, parser )
    .then( function ( geometry ) {
      addPrimitiveAttributes( geometry, primitive, accessors );
      return geometry;
  } );

-->

var geometryPromise = extensions[ EXTENSIONS.KHR_DRACO_MESH_COMPRESSION ]
  .decodePrimitive( primitive, parser )
  .then( function ( data ) {
      addPrimitiveAttributes( data['geometry'], data['primitive'], accessors );
      return data['geometry'];
  } );
Three.js version
  • Dev
  • r97
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@donmccurdy
Copy link
Collaborator

I've checked that the models all look valid. This is a bug in our loader. @takahirox do you have time to investigate this? It will be a week until I'm able at least.

Thank you for providing so much detail on this bug report. 🙂

@donmccurdy
Copy link
Collaborator

Also, @ft-lab — would you mind if I add that second model (with the morph targets) to the list of sample models on https://github.com/KhronosGroup/glTF-Sample-Models? It seems to be a useful test case.

@ft-lab
Copy link
Author

ft-lab commented Oct 13, 2018

Thank you for your reply!
Oh, it's a good idea to upload it to https://github.com/KhronosGroup/glTF-Sample-Models.
If so, I'm very happy.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 19, 2018

I've found the bug in the loader that prevents the first model from loading, and will open a PR soon.

On the second model (with morph targets) I think there may be a problem in the model. The second primitive seems to have 21 vertices in the compressed Draco buffer — but the attribute and morph target accessors only have 9 vertices. This isn't correct and will cause errors. One possible cause that is important to know about is that Draco compression could change the number of vertices, and their order. So if the mesh data is compressed and there are morph targets, you may need to be careful: KhronosGroup/glTF#1353 (comment)

@donmccurdy
Copy link
Collaborator

I do think it's very weird that the other primitive should have 21 vertices though... maybe we're reusing the same Draco decoder in the wrong way, and getting data for the wrong primitive? 🤔

@ft-lab
Copy link
Author

ft-lab commented Oct 19, 2018

In the second case, I think ...
When storing Mesh data and using with Morph Targets, "mesh sequential encoding" is specified so that the order of vertices does not change in Draco.
When I get this with C++'s Draco library ( https://github.com/google/draco ), I can get it as follows.

[primitive 0]
num_faces : 24
num_attributes : 3
num_points : 21

[primitive 1]
num_faces : 8
num_attributes : 3
num_points : 9

This was consistent with the accessors' count of json part of multiplePrimitives_draco.gltf.

In GLTFLoader.js (r97),
I think primitive 0 and primitive 1 are pointing to the same thing.

With the following code,
By decodePrimitive being called delay,
I thought the cause is that primitive 1 is primitive 0.

var geometryPromise = extensions[ EXTENSIONS.KHR_DRACO_MESH_COMPRESSION ]
    .decodePrimitive( primitive, parser )
    .then( function ( geometry ) {
      addPrimitiveAttributes( geometry, primitive, accessors );
      return geometry;
  } );

@ft-lab
Copy link
Author

ft-lab commented Oct 19, 2018

I did not upload the sample here, but I'll write it for reference.
When using Draco
Since I store the triangle indices + vertices data with one Draco's bitstream,
I think that waste may increase when compressing Draco.

When I verified with the specification of Draco, when sharing vertices data with multiple primitives,
All the vertices data had to be copied to Draco's bitstream.

I'm sorry if I misunderstood the specification.

@donmccurdy
Copy link
Collaborator

You're right, it was a bug. I'll have this fixed soon. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants