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

KHR_draco_mesh_compression #874

Merged
merged 50 commits into from Feb 14, 2018

Conversation

@fanzhanggoogle
Contributor

fanzhanggoogle commented Mar 13, 2017

Hi,

We made an extension for using Google Draco geometry compression library with glTF. It could also be easily extended to more compression libraries if it suits well to glTF 2.0.

Let us know if there is any flaws in the design. Thank you very much.

We also provide an alternative approach in the appendix in case anyone is interested in discussing it.

@ondys
@FrankGalligan

@lexaknyazev

This comment has been minimized.

Show comment
Hide comment
@lexaknyazev

lexaknyazev Mar 14, 2017

Member

Also, geometryType property duplicates mesh.primitive.mode, so could we remove it and add needed restrictions on valid core enum values (i.e. it's either TRIANGLE_LIST, or POINTS)?

Member

lexaknyazev commented Mar 14, 2017

Also, geometryType property duplicates mesh.primitive.mode, so could we remove it and add needed restrictions on valid core enum values (i.e. it's either TRIANGLE_LIST, or POINTS)?

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Mar 14, 2017

Member

For the extension name prefix, let's just start with KHR_. This will avoid churn in the extensions namespace and this will have multiple implementations quickly.

Beyond that the complete extension name is up to the Draco team.

Member

pjcozzi commented Mar 14, 2017

For the extension name prefix, let's just start with KHR_. This will avoid churn in the extensions namespace and this will have multiple implementations quickly.

Beyond that the complete extension name is up to the Draco team.

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Mar 14, 2017

Member

@sbtron @bghgary perhaps you also want to review this?

Member

pjcozzi commented Mar 14, 2017

@sbtron @bghgary perhaps you also want to review this?

Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
Show outdated Hide outdated extensions/Khronos/KHR_draco_mesh_compression/README.md Outdated
@@ -0,0 +1,138 @@
# KHR_draco_mesh_compression

This comment has been minimized.

@FrankGalligan

FrankGalligan Nov 1, 2017

Contributor

This comment has been minimized.

@FrankGalligan

FrankGalligan Nov 1, 2017

Contributor

As the Draco extension is referring to significant external documents – namely the Bitstream specification we should also include, and be consistent with the additional header wording for normative clarity:
https://www.khronos.org/members/login/groups/Agreements%20and%20Licenses/Open%20Source%20Repository%20Resources/Khronos%20Copyright%20License%20Header%20Addition%20for%20NORMATIVE%20CLARITY%20Oct17.txt Note - this has some boiler plate that needs to be tweaked to fit the spec.

The key text in the header is:

Some parts of this Specification are purely informative and do not define requirements
necessary for compliance and so are outside the Scope of this Specification. These
parts of the Specification are marked by the "Note" icon or designated "Informative"
<replace/insert specific conventions for the specification here>.

And

Where this Specification includes normative references to external documents, only the
specifically identified sections and functionality of those external documents are in
Scope. Requirements defined by external documents not created by Khronos may contain
contributions from non-members of Khronos not covered by the Khronos Intellectual
Property Rights Policy.

@FrankGalligan

FrankGalligan Nov 1, 2017

Contributor

As the Draco extension is referring to significant external documents – namely the Bitstream specification we should also include, and be consistent with the additional header wording for normative clarity:
https://www.khronos.org/members/login/groups/Agreements%20and%20Licenses/Open%20Source%20Repository%20Resources/Khronos%20Copyright%20License%20Header%20Addition%20for%20NORMATIVE%20CLARITY%20Oct17.txt Note - this has some boiler plate that needs to be tweaked to fit the spec.

The key text in the header is:

Some parts of this Specification are purely informative and do not define requirements
necessary for compliance and so are outside the Scope of this Specification. These
parts of the Specification are marked by the "Note" icon or designated "Informative"
<replace/insert specific conventions for the specification here>.

And

Where this Specification includes normative references to external documents, only the
specifically identified sections and functionality of those external documents are in
Scope. Requirements defined by external documents not created by Khronos may contain
contributions from non-members of Khronos not covered by the Khronos Intellectual
Property Rights Policy.

This comment has been minimized.

@fanzhanggoogle

fanzhanggoogle Nov 1, 2017

Contributor

Done

@fanzhanggoogle

fanzhanggoogle Nov 1, 2017

Contributor

Done

@zellski

This comment has been minimized.

Show comment
Hide comment
@zellski

zellski Nov 8, 2017

Contributor

Having just recently added morph targets to my glTF generation, I'm suddenly noticing that they're absent from Draco considerations. It's likely out of scope and in any case too late to add this to the initial release of the extension, but may I join @donmccurdy in advocating for its usefulness?

Sparse accessors salvage morph targets from extreme bloat, but they're still likely to take up a disproportionate amount of space when all the other static geometry has been compressed.

As a corollary, I know the Draco team is at least thinking about the temporaral dimensions viz. animations, and so it's worth noticing here that the 'weights' target for animations is likely very compressible.

Contributor

zellski commented Nov 8, 2017

Having just recently added morph targets to my glTF generation, I'm suddenly noticing that they're absent from Draco considerations. It's likely out of scope and in any case too late to add this to the initial release of the extension, but may I join @donmccurdy in advocating for its usefulness?

Sparse accessors salvage morph targets from extreme bloat, but they're still likely to take up a disproportionate amount of space when all the other static geometry has been compressed.

As a corollary, I know the Draco team is at least thinking about the temporaral dimensions viz. animations, and so it's worth noticing here that the 'weights' target for animations is likely very compressible.

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Nov 20, 2017

Member

This is probably too close to final, as you said, but even if it weren't — if we added support for morph targets with the Draco format as it is now, I assume each target would need to be treated as a separate mesh by the decoder. This would limit the performance benefit, compared to using a temporal dimension per the long-term roadmap.

So let's give feedback on the use cases in the Draco repository (say, google/draco#126) and consider bringing Draco-compressed morph targets to glTF as an official KHR_ extension when those dependencies are in place.

In the meantime it would certainly be possible to use vendor extensions for short-term needs.

Member

donmccurdy commented Nov 20, 2017

This is probably too close to final, as you said, but even if it weren't — if we added support for morph targets with the Draco format as it is now, I assume each target would need to be treated as a separate mesh by the decoder. This would limit the performance benefit, compared to using a temporal dimension per the long-term roadmap.

So let's give feedback on the use cases in the Draco repository (say, google/draco#126) and consider bringing Draco-compressed morph targets to glTF as an official KHR_ extension when those dependencies are in place.

In the meantime it would certainly be possible to use vendor extensions for short-term needs.

@JeremieA

This comment has been minimized.

Show comment
Hide comment
@JeremieA

JeremieA Dec 7, 2017

Hello,
Great work on this very useful and well-designed extension !
I have two suggestions below, but as it is indeed very late in the process it would be fine to only consider them for the next version. However I think it would be great to include them if possible, as they are simple but quite useful.

For our use-case (physics-based simulations) we are very interested in animations, hence I created a proof-of-concept implementation of adding morph targets to this extension. It is actually pretty simple, the attributes within targets can be compressed in the same way as the regular attributes, and the extension format for them can follow exactly the same principle. Because in glTF the morph targets are stored as displacements from the initial mesh, they are actually very efficient to compress, without changing anything in the Draco encoder itself :)

Please see the simple additions I propose to add support for morph targets in the extension text and json schema in 13c1d79.

To demonstrate the usefulness of this feature, you can have a look at :

  • Implementing morph targets decompression in three.js GLTFLoader can be done in 19 lines (after the refactoring discussed below)
  • Implementing morph targets compression in a simple experimental online tool (credits to @donmccurdy for the great viewer!) can be added in 43 lines
  • A water animated mesh (credits to 3dartel for the model) can be compressed from 32.8MB to 5.6MB (total size as GLB). For reference, compressing only the attributes leaves a 31MB file (which is not even valid as there is no easy way at least in the javascript version to apply the indices permutation from the Draco encoder)
  • Combining morph targets and skinning works great (credits to Rob Allen for the model). The overall savings (2.48MB -> 1.83MB) are not as good because of the included texture, but still better than not compressing the morph targets (2.34MB)

(you can use Pack ZIP then Save to have a look at the glTF json and data, or inspect gltfContent.gltf from the console)

Note that one of the reason why supporting this feature can be simple is by refactoring DRACOLoader as used within three.js GLTFLoader to be a transparent preprocess step when loading the data of the glTF accessors, rather than as a replacement for all the mesh primitives parsing code. The corresponding commit is 41b75f0 (with a later update in 14e91e8 after merging with the dependencies refactoring that was recently pushed to three.js). This design requires that each different compressed mesh uses unique glTF accessor ids, which is currently not an explicit requirement in the extension, but does make sense (given that information such as min/max must still be given for each attribute), and it is the case for all draco-compressed glTF created by current encoders as far as I could test. The added benefit is that this adds support for mixing compressed and uncompressed attributes without any code required. It does also make implementing a decompress tool quite simple (as the list of accessors would be unchanged, only bufferViews would be added back)
To make this explicit in the extension text, I propose to add a paragraph such as 2ff0ffa

JeremieA commented Dec 7, 2017

Hello,
Great work on this very useful and well-designed extension !
I have two suggestions below, but as it is indeed very late in the process it would be fine to only consider them for the next version. However I think it would be great to include them if possible, as they are simple but quite useful.

For our use-case (physics-based simulations) we are very interested in animations, hence I created a proof-of-concept implementation of adding morph targets to this extension. It is actually pretty simple, the attributes within targets can be compressed in the same way as the regular attributes, and the extension format for them can follow exactly the same principle. Because in glTF the morph targets are stored as displacements from the initial mesh, they are actually very efficient to compress, without changing anything in the Draco encoder itself :)

Please see the simple additions I propose to add support for morph targets in the extension text and json schema in 13c1d79.

To demonstrate the usefulness of this feature, you can have a look at :

  • Implementing morph targets decompression in three.js GLTFLoader can be done in 19 lines (after the refactoring discussed below)
  • Implementing morph targets compression in a simple experimental online tool (credits to @donmccurdy for the great viewer!) can be added in 43 lines
  • A water animated mesh (credits to 3dartel for the model) can be compressed from 32.8MB to 5.6MB (total size as GLB). For reference, compressing only the attributes leaves a 31MB file (which is not even valid as there is no easy way at least in the javascript version to apply the indices permutation from the Draco encoder)
  • Combining morph targets and skinning works great (credits to Rob Allen for the model). The overall savings (2.48MB -> 1.83MB) are not as good because of the included texture, but still better than not compressing the morph targets (2.34MB)

(you can use Pack ZIP then Save to have a look at the glTF json and data, or inspect gltfContent.gltf from the console)

Note that one of the reason why supporting this feature can be simple is by refactoring DRACOLoader as used within three.js GLTFLoader to be a transparent preprocess step when loading the data of the glTF accessors, rather than as a replacement for all the mesh primitives parsing code. The corresponding commit is 41b75f0 (with a later update in 14e91e8 after merging with the dependencies refactoring that was recently pushed to three.js). This design requires that each different compressed mesh uses unique glTF accessor ids, which is currently not an explicit requirement in the extension, but does make sense (given that information such as min/max must still be given for each attribute), and it is the case for all draco-compressed glTF created by current encoders as far as I could test. The added benefit is that this adds support for mixing compressed and uncompressed attributes without any code required. It does also make implementing a decompress tool quite simple (as the list of accessors would be unchanged, only bufferViews would be added back)
To make this explicit in the extension text, I propose to add a paragraph such as 2ff0ffa

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Dec 8, 2017

Member

Great comments, thanks @JeremieA!

I'll leave aside the question of whether it is too late in the process — I suspect that is so, but would defer to others. Noticing that you've used a DracoEncoderModule.GENERIC attribute to encode the morph targets, there may be better savings possible with changes to the Draco encoder to factor in morph target and animation data as recognized types. That's probably on the longer-term roadmap for Draco, as mentioned in google/draco#126. If so, and given that it's easier to iterate on the encoding/decoder/loader code than on a published specification, it might be best to do this in a follow-up version.

These changes for implementation within THREE.GLTFLoader and THREE.DRACOLoader look promising in their own rights, as well. By the way, how are you doing .ZIP export? THREE.GLTFExporter hasn't added that option yet although we're interested. 🙂

The restriction on accessor IDs looks reasonable to me... is it considered OK for an extension to narrow the core spec when in use? I'm inclined to say this should be an implementation note, which we could still add after the extension is finished... This also buys me some time to work on implementation: I've been meaning to try to get DRACOLoader running in a Web Worker, with multiple workers in parallel for multiple meshes, which might get around the question of preprocessing.

Member

donmccurdy commented Dec 8, 2017

Great comments, thanks @JeremieA!

I'll leave aside the question of whether it is too late in the process — I suspect that is so, but would defer to others. Noticing that you've used a DracoEncoderModule.GENERIC attribute to encode the morph targets, there may be better savings possible with changes to the Draco encoder to factor in morph target and animation data as recognized types. That's probably on the longer-term roadmap for Draco, as mentioned in google/draco#126. If so, and given that it's easier to iterate on the encoding/decoder/loader code than on a published specification, it might be best to do this in a follow-up version.

These changes for implementation within THREE.GLTFLoader and THREE.DRACOLoader look promising in their own rights, as well. By the way, how are you doing .ZIP export? THREE.GLTFExporter hasn't added that option yet although we're interested. 🙂

The restriction on accessor IDs looks reasonable to me... is it considered OK for an extension to narrow the core spec when in use? I'm inclined to say this should be an implementation note, which we could still add after the extension is finished... This also buys me some time to work on implementation: I've been meaning to try to get DRACOLoader running in a Web Worker, with multiple workers in parallel for multiple meshes, which might get around the question of preprocessing.

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Dec 8, 2017

Member

/cc @lexaknyazev for the question of accessor ID requirements.

Member

donmccurdy commented Dec 8, 2017

/cc @lexaknyazev for the question of accessor ID requirements.

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Feb 8, 2018

Member

I think this is ready to merge. Any other feedback?

Member

pjcozzi commented Feb 8, 2018

I think this is ready to merge. Any other feedback?

@donmccurdy donmccurdy changed the title from KHR_draco_geometry_compression to KHR_draco_mesh_compression Feb 8, 2018

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Feb 8, 2018

Member

Looks good to me. 👍

Member

donmccurdy commented Feb 8, 2018

Looks good to me. 👍

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Feb 8, 2018

Member

About unique IDs on accessors — the pending three.js implementation does not require unique IDs, but I am fine with including the restriction in some form. Morph targets will be left for a future version of the extension (or new, separate extension, TBD) covering animation, and should be omitted from the proposed text.

A question about implications — suppose my model has three (3) compressed meshes, and each mesh is identical except for an application-specific attribute (say, _ROUGHNESS) that will differ between instances of the mesh. To use Draco compression, what can I do?

a. Duplicate all compressed data for each mesh, in a new Draco-compressed bufferView.
b. Encode multiple roughness attributes (_ROUGHNESS_0, _ROUGHNESS_1, ...) within a single Draco-compressed bufferView. All three meshes will reference the same Draco data, but will use different IDs into the compressed data for their _ROUGHNESS attributes.
c. Leave the _ROUGHNESS attribute uncompressed, pointing to a different accessor for each mesh.

If I understand correctly, (a) and (b) are fine, but (c) would be disallowed with this requirement.

Member

donmccurdy commented Feb 8, 2018

About unique IDs on accessors — the pending three.js implementation does not require unique IDs, but I am fine with including the restriction in some form. Morph targets will be left for a future version of the extension (or new, separate extension, TBD) covering animation, and should be omitted from the proposed text.

A question about implications — suppose my model has three (3) compressed meshes, and each mesh is identical except for an application-specific attribute (say, _ROUGHNESS) that will differ between instances of the mesh. To use Draco compression, what can I do?

a. Duplicate all compressed data for each mesh, in a new Draco-compressed bufferView.
b. Encode multiple roughness attributes (_ROUGHNESS_0, _ROUGHNESS_1, ...) within a single Draco-compressed bufferView. All three meshes will reference the same Draco data, but will use different IDs into the compressed data for their _ROUGHNESS attributes.
c. Leave the _ROUGHNESS attribute uncompressed, pointing to a different accessor for each mesh.

If I understand correctly, (a) and (b) are fine, but (c) would be disallowed with this requirement.

@FrankGalligan

This comment has been minimized.

Show comment
Hide comment
@FrankGalligan

FrankGalligan Feb 13, 2018

Contributor

@donmccurdy Yes a. and b. are fine, c. is disallowed. You can file a feature request for a future version if you think this use case is useful.

Contributor

FrankGalligan commented Feb 13, 2018

@donmccurdy Yes a. and b. are fine, c. is disallowed. You can file a feature request for a future version if you think this use case is useful.

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Feb 13, 2018

Member

I don't have any need for (c), just wanted to verify that restricting accessor IDs to be unique-per-mesh would have the implications expected. 👍

Member

donmccurdy commented Feb 13, 2018

I don't have any need for (c), just wanted to verify that restricting accessor IDs to be unique-per-mesh would have the implications expected. 👍

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Feb 14, 2018

Member

Is this ready to merge?

Member

pjcozzi commented Feb 14, 2018

Is this ready to merge?

@pjcozzi pjcozzi merged commit b842686 into KhronosGroup:master Feb 14, 2018

@JeremieA

This comment has been minimized.

Show comment
Hide comment
@JeremieA

JeremieA Feb 17, 2018

Congrats on the official launch of this extension !

I will update my tools to reflect the final standard (disabling the morph targets support by default).

Regarding the attributes from multiple mesh instances, option (b) is what I implemented in my compressor (see here). All primitives sharing the same indices and positions data are compressed as a single Draco-compressed mesh, as I assumed that it would be most efficient. While testing this compression on various glTF assets, I did not notice this case of two primitives using different attributes while referring to the same positions and indices. I suspect most exporters would duplicate all attributes in this case. But I know of at least GearboxAssy from glTF-Samples-Model with two primitives referring to the same attributes (but different materials).

Sorry @donmccurdy for not answering your earlier questions. The ZIP export that I implemented was done as a completely separate code from THREE.GLTFExporter, but it shouldn't be too difficult to use something similar. The relevant code is here.

JeremieA commented Feb 17, 2018

Congrats on the official launch of this extension !

I will update my tools to reflect the final standard (disabling the morph targets support by default).

Regarding the attributes from multiple mesh instances, option (b) is what I implemented in my compressor (see here). All primitives sharing the same indices and positions data are compressed as a single Draco-compressed mesh, as I assumed that it would be most efficient. While testing this compression on various glTF assets, I did not notice this case of two primitives using different attributes while referring to the same positions and indices. I suspect most exporters would duplicate all attributes in this case. But I know of at least GearboxAssy from glTF-Samples-Model with two primitives referring to the same attributes (but different materials).

Sorry @donmccurdy for not answering your earlier questions. The ZIP export that I implemented was done as a completely separate code from THREE.GLTFExporter, but it shouldn't be too difficult to use something similar. The relevant code is here.

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