search

CesiumGS / gltf-pipeline

Content pipeline tools for optimizing glTF assets. :globe_with_meridians:
Apache License 2.0
1.91k stars 251 forks source link

Draco compression probably breaks `CESIUM_primitive_outline` extension #630

Closed javagl closed 1 year ago

javagl commented 1 year ago

This is strongly related to https://github.com/CesiumGS/gltf-pipeline/issues/629 , and may just be another "instance" of the same problem:

The Draco compression restructures the meshes in a non-obvious way. Every extension that is added to a glTF asset for a given mesh structure will likely break when Draco is applied to this asset. Specifically, the CESIUM_primitive_outline extension defines a reference to some indices, but when the topology of the mesh is changed by Draco, then these indices will become invalid.

I don't know the necessary technical details of Draco to say how much effort it is (or whether it is even possible) to avoid this. In any case, each extension that should "survive" the Draco compression will have to be taken into account by the gltf-pipeline (and CESIUM_primitive_outline does not seem to be taken into account right now.

EDIT: This is in fact a duplicate of https://github.com/CesiumGS/gltf-pipeline/issues/558 , and all these issues might be specific cases of https://github.com/CesiumGS/gltf-pipeline/issues/579 . One suggestion in a comment there was to use --draco.compressionLevel=0 to still have some compression, but keep the indices intact. Maybe that helps (I haven't tried it out yet).

kring commented 1 year ago

each extension that should "survive" the Draco compression will have to be taken into account by the gltf-pipeline

Well... yes and no. gltf-pipeline's Draco compression needs to know that a particular index buffer refers to vertices so that it can rewrite the index buffer when the vertex order changes. But it doesn't need to know about CESIUM_primitive_outline specifically. For example, I can imagine annotating the extension's JSON schema with enough information so that gltf-pipeline knows how to do this (as long as it can find the JSON schema for any given extension). Or a user could even supply a command-line argument that says "the indices property of CESIUM_primitive_outline identifies an index buffer that should be updated according to the renumbered vertices." I don't mean to make it sound trivial, cause it's not, but it would be nice to not need to code each extension into gltf-pipeline explicitly.

javagl commented 1 year ago

How exactly it will determine which indices have to be updated is one question. Certain extensions could be supported by default, and one could think deeply about how to offer further options beyond that (e.g. at the command line, ranging from --updateAcessors=[3,14,59] to some JSON-path-regexes like --updateAccessors=["*.CESIUM_primitive_outline.indices"] or so...).

The more challenging point is how that rewriting itself should take place. Iff the number of indices remains the same, one could try to build some map<int,int> oldToNewIndices there, but whether that mapping can trivially be reconstructed by comparing the Draco input and output, whether that can be used to update other index sets without breaking possible "semantics" that these other indices might have is not so obvious. And... when the number of vertices or indices changes, rewriting other index buffers might not even be possible....

lilleyse commented 1 year ago

Possibly related issue: https://github.com/google/draco/issues/584

javagl commented 1 year ago

There are some related issues, e.g. https://github.com/google/draco/issues/326 or https://github.com/google/draco/issues/363

But I gave this a stab.

Disclaimer:

I have no clue how Draco works, on any level, and how it is integrated into (and used in) the gltf-pipeline. I compressed the "Duck.glb" sample model with gltf-pipeline and compressionLevel=10, and had a look at the result. From what I've seen, the KHR_draco_mesh_compression extension object is only covering the attributes. The indices seem to remain unaffected. The number of vertices (i.e. e.g. the size of the POSITIONS accessor) remained the same.

Specifically: I don't know whether this is generally true, or whether gltf-pipeline "intentionally" does not modify the indices.

What I've done now:

The uncompressed one worked fine. The compressed ones caused the expected

An error occurred while rendering. Rendering has stopped. TypeError: Cannot read properties of undefined (reading 'count') at loadAccessor (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:108653:36) ...

Looking at the GLBs, I noticed that the accessor that the CESIUM_primitive_outline object referred to had simply been removed. A possible fix for this is in https://github.com/CesiumGS/gltf-pipeline/pull/631