KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.1k stars 1.13k forks source link

Loading multiple primitives from one Draco compressed mesh #1333

Open ivalylo opened 6 years ago

ivalylo commented 6 years ago

I have a mesh format which is not that odd IMO. The mesh has:

The GLTF format itself is very flexible when defining buffers and their usage. I'm creating global accessors for the vertex attributes, but per primitive accessors for the indices. The buffers are of course shared. However I'm not sure how to translate this to the draco compression extension. The most obvious way is to create one single draco mesh for all primitives. However the GLTF viewers I've tested (threejs and babylonjs) will not check that there are different primitives with different index ranges and material assignments. They just use one (a bit random) material for all primitives. Technically it seems that all the data is there. I.e. the acessors and their min/max values are present, but this case is just not handled. Is this problem of the specs or the implementations? Will it be supported?

lexaknyazev commented 6 years ago

They just use one (a bit random) material for all primitives.

Each primitive is able and should have its own material reference in the asset. If engines ignore or mishandle that, it's an implementation bug. Please consider opening issues in engines' repos with examples.

donmccurdy commented 6 years ago

However the GLTF viewers I've tested (threejs and babylonjs) will not check that there are different primitives with different index ranges and material assignments.

If you can share one of the assets you're referring to, that would be helpful.

three.js certainly supports assigning different materials to different primitives. But having multiple primitives reference different ranges of the same Draco geometry is not something I've seen before, and I'm not sure whether the syntax of the extension spec supports that... note the proposed clarification here, which is not finalized but is consistent with all existing examples.

ivalylo commented 6 years ago

Here is very simple test scene - with and without draco compression. The cube has two materials - green and red which are loaded correctly in the uncompressed version gltf_draco_test.zip

"Multiple primitives can refer to the same accessor only if it is associated with the same draco compressed geometry and draco attribute id within it." I think this makes sense and is true for this case. The cube is compressed as one draco mesh. The primitive accessors refer to different ranges of indices in that mesh.

donmccurdy commented 6 years ago

Thanks, your test scene clarifies things well. Given this snippet:

"meshes": [{
        "primitives": [{
                "attributes": {
                    "NORMAL": 1,
                    "POSITION": 2
                },
                "indices": 0,
                "mode": 4,
                "material": 0,
                "extensions": {
                    "KHR_draco_mesh_compression": {
                        "bufferView": 0,
                        "attributes": {
                            "POSITION": 1,
                            "NORMAL": 0
                        }
                    }
                }
            },{
                "attributes": {
                    "NORMAL": 1,
                    "POSITION": 2
                },
                "indices": 3,
                "mode": 4,
                "material": 1,
                "extensions": {
                    "KHR_draco_mesh_compression": {
                        "bufferView": 0,
                        "attributes": {
                            "POSITION": 1,
                            "NORMAL": 0
                        }
                    }
                }
            }
        ],
        "name": "Cube"
    }
],

I haven't included the accessors, but all four omit the bufferView index. I assume (please correct me if I'm wrong) that what you want is to assign each primitive a different index with an accessor that does reference a bufferView with uncompressed data. The problem is this line of the spec:

If the loader does support the Draco extension ... then the loader must process attributes and indices properties of the primitive. When loading each accessor, you must ignore the bufferView and go to the previously decoded Draco geometry in the primitive to get the data of indices and attributes.

The decoded Draco geometry still contains indices, and so technically the loader must ignore the indices of the original primitive. The entire geometry is rendered with both materials, and which you see is arbitrary. I don't think the current Draco spec allows what you're trying to do — there's no hint that the uncompressed indices are meant to replace the compressed indices, rather than being an uncompressed fallback.

One other minor thing: the KHR_materials_common has been set aside for glTF 2.0, you'll want to use KHR_lights instead.

donmccurdy commented 6 years ago

I would also say this should be spelled out in https://github.com/KhronosGroup/glTF/issues/1267, as the clarification it is trying to add would be useless if indices were different.

In summary:

  1. The spec should in any case be clarified.
  2. Perhaps this case can be supported in a future version of the Draco extension. For that to happen, it would be helpful if you could quantify the benefits of reusing a single Draco geometry with different indices, rather than creating a separate Draco geometry for each material.
lilleyse commented 6 years ago

I don't think the current Draco spec allows what you're trying to do — there's no hint that the uncompressed indices are meant to replace the compressed indices, rather than being an uncompressed fallback.

I've basically come to the same conclusion while looking at https://github.com/AnalyticalGraphicsInc/gltf-pipeline/issues/369. The model in that issue has the same structure as the sample model @ivalylo posted.

The one benefit of sharing the same geometry is it avoids the need to duplicate the vertices along the material seams. But at the same time... the compression probably makes up for any losses.

ivalylo commented 6 years ago

If the loader does support the Draco extension ... then the loader must process attributes and indices properties of the primitive. When loading each accessor, you must ignore the bufferView and go to the previously decoded Draco geometry in the primitive to get the data of indices and attributes.

I don't see how this format conflicts with the text. Yes, you should ignore the buffer view, but the accessor itself has a count and min/max range. So currently the whole accessor seems to be ignored, not just the buffer view. Like I said at the beginning, since the accessor is present and provides this data, since the primitives are listed, how it actually works in practice looks fishy to me.

I think this format should be supported because:

ivalylo commented 6 years ago

Actually, the min/max range of the accessor can't be used since the vertex buffers can be shared in arbitrary way. But the accessor count can be used. So basically you need to read this count and create a draw call with that index range. The index start must be tracked with the assumption that there are no gaps in the indices. So the next primitive index start will be the previous index end + 1.

donmccurdy commented 6 years ago

How do you tell that this is the same mesh after that? In GLTF you must even create different nodes for each mesh, and the original structure is totally lost...

This is why you would use the primitives list within a single mesh, each with different materials, and maintain the association. The core spec is quite flexible about how primitives are used, the KHR_draco_mesh_compression is more restrictive (each primitive must be compressed separately), but still you can group primitives together.

I also have issue with compressing morph targets. ... It seems to me that the draco extension was added to support very particular needs, and was not thought through how it will work generally with the format.

The KHR_draco_mesh_compression extension does not support compression of morph targets yet. That is planned for a future version of the extension.

Actually, the min/max range of the accessor can't be used since the vertex buffers can be shared in arbitrary way. But the accessor count can be used. So basically you need to read this count and create a draw call with that index range. The index start must be tracked with the assumption that there are no gaps in the indices. So the next primitive index start will be the previous index end + 1.

This is not how the KHR_draco_mesh_compression extension is defined; there is no assumption that primitives have sequential indices. In its current form you must apply Draco compression to each primitive separately if they have different indices. The spec allows for vertex attributes of a Draco primitive to remain uncompressed, and these may be different from attributes of other primitives referencing the same Draco compressed geometry, but there is no such allowance for indices. That is a bit inconsistent, and perhaps it should be addressed in a future version of the extension, but it does require a change to the extension as it exists today.

In general I wish there were more clarity about how primitives should be used. Your situation (different materials applied to different index ranges of a single mesh) is an important and common case, but loaders can make no assumptions here. I've raised some of these questions before in https://github.com/KhronosGroup/glTF/issues/1278.

lexaknyazev commented 6 years ago

Just so you know, decompressed indices don't have to match original indices. While encoding connectivity, Draco encoder can rearrange vertex order.

ivalylo commented 6 years ago

Ah, yes, the KHR_draco_mesh_compression is attached to the primitive... The mesh concept in GLTF is still quite odd to me.

I also have index compression in my format that rearranges the indices, but it's done per index range. It's really something that requires support in draco itself... If the specs must be changed anyway, I would propose to build this feature into the draco library itself. I don't know exactly what method of compression is used for the indices, but I doubt there isn't a way to compress the index ranges separately. If the draco mesh itself has notion about sub-primitives, it will be probably the most clear way.

donmccurdy commented 6 years ago

In summary, this change would require:

All of this requires some work, so it would be helpful to hear from interested developers: (1) example use cases, (2) general interest, and (3) quantified performance numbers — specifically, how much does the compression ratio improve when primitives use shared compressed vertex data with per-primitive indices?

monoto commented 2 years ago

Here is a scenario that demonstrate the need for additional materialIndex per triangle. The existing Index and Positions data give a list of Triangles. Each Traiangle contains 3 vertex UVs That each points to a position in a material map. With additional material index we can assign different part of mesh to a different material map albeit with the same UV. The desired result is a mesh can have multiple maps on its surface sharing one common set of UVs.

donmccurdy commented 2 years ago

@monoto I don't think this is the right issue for that question (it's specific to Draco compression) but note that glTF is intended to contain data that a GPU can render in realtime, without rebuilding the scene vertex by vertex. The best way to handle the case you describe is to have a single vertex buffer, with two different index lists, and to draw each index list with different materials.