KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.14k stars 1.14k forks source link

Is BufferView.Target optional? #1440

Closed prollin closed 2 years ago

prollin commented 6 years ago

I am working on a glTF importer and I am a bit confused how to efficiently load a model. At first I thought i could simply load BufferView that are ARRAY_BUFFER and ELEMENT_ARRAY_BUFFER directly into the OpenGL equivalent (vbo/ibo) but I stumbled on a couple models (from the sample directory) that do not have the "Target" field on any BufferView. It seems that the only reliable way to load geometry is to follow the Mesh->Primitives->Accessors chain and painfully assemble interleaved vertex buffers by iterating through each accessor data; is this what is expected or am I missing something? It seems to me that BufferView.Target should probably be removed so it is clear those buffers are not really a direct mapping to OpenGL equivalent (usually you want only 1 big buffer for the whole model anyway).

I am new to glTF so it is entirely possible that I don't understand how things are supposed to be used or the constrains of the intended use cases (web).

lexaknyazev commented 6 years ago

Hi Philippe,

This part of the spec probably deserves a better explanation (/cc @javagl for tutorials). Since a buffer view can be used for non-GPU data (such as images or animations), bufferView.target cannot be a required field. Unfortunately, we cannot enforce all exporters to write out target for GPU buffers but some existing spec restrictions make loading quite simple.

  1. If target is defined, an importer can trust it.
  2. Regardless of target presence, a buffer view cannot be used for mixed data (i.e., it cannot contain both indices and vertex attributes).
prollin commented 6 years ago

Thanks for the info Alexey!

From experience, anything that is left under-specified will be misused (COLLADA). I am perfectly happy with the Accessor system, I just think it is misleading to have this BufferView.Target field that may or may not describe a GPU friendly buffer.

On a similar note, I do think that it would be far more efficient if BufferView wasn't referenced by accessors, but by the Primitive itself, or even the Mesh (the exporter would be responsible to properly interleave the data) so loading geometry could be reduced to 1 memcpy per mesh (the Accessors would still be there to describe how the data inside the BufferView should be interpreted).

Tying BufferViews to Accessors makes for a very flexible system but it makes importing more complex and doesn't encourage good performance practice (ideally an entire model should fit in a single VBO); I just don't see the use case where the same Accessor could be used by multiple primitive (maybe this is useful for animations?)

Again, I am not yet familiar with many aspect of the spec (animation, skinning, morph targets) so there are use cases I am probably missing.

lexaknyazev commented 6 years ago

BufferView.Target field that may or may not describe a GPU friendly buffer.

Let me elaborate a bit more here:

  1. target is present. This means that a buffer view contains exactly target data.
  2. target isn't present. Buffer view still contains only one kind of data (i.e, one of: index buffer, vertex buffer, animation data, image data, or skinning data).

In both cases, a buffer view can be referenced only by appropriate accessors, so it is guaranteed that a buffer view used by, e.g., position attribute, contains only vertex data. Some importers may choose to always infer target based on usage and ignore provided value.

it would be far more efficient... Tying BufferViews to Accessors makes for a very flexible system

This is exporters' choice. They may choose to pack all vertex attributes and use only two buffer views (index and vertex) per primitive, or (on the opposite end) they can put each attribute into a separate buffer stored in a separate file.

prollin commented 6 years ago

Some importers may choose to always infer target based on usage and ignore provided value.

Makes sense, thank you for taking the time to explain.

They may choose to pack all vertex attributes and use only two buffer views (index and vertex) per primitive

This is the last part that is not clear; you seem to suggest that you can you have multiple vertex attribute in a single BufferView? However according to the spec: When a buffer view contain vertex indices or attributes, they must be its only content, i.e., it's invalid to have more than one kind of data in the same buffer view. Does "kind of data" means index/vertex (or anim data) where multiple attribute of a vertex could be in the same bufferview (which is my initial understanding) or can you only have 1 attribute type per bufferview, meaning that each accessor has its own bufferview (which seem to be the case in a a few samples and an assumption a couple open source importer seem to be making).

Thank you again for the clarifications!

prollin commented 6 years ago

Looking at a few more sample files I think i just answered my own question:

If I understand properly, you can have multiple vertex attributes in the same buffer view but they need to be the same type (where type = accessor.ComponentType+accessor.Type). I guess a better way to phrase the remaining of the above question is: Is it valid to have interleaved vertex attribute data in the same bufferview like such: [POSITION][NORMAL][TEXCOORD][POSITION][NORMAL][TEXCOORD][POSITION][NORMAL][TEXCOORD]... or is the data always non-interleaved: [POSITION][POSITION][POSITION]...[NORMAL][NORMAL][NORMAL]...[TEXCOORD][TEXCOORD][TEXCOORD]?

lexaknyazev commented 6 years ago

For several attributes to be in the same buffer view, the byte stride (distance in bytes between vertices) must be the same for all attributes. Also, attributes must be aligned to 4-byte boundaries.

Example with one triangle with three attributes per vertex: position (float VEC3), normal (float VEC3), color (ubyte normalized VEC4).

For each vertex:
|00|01|02|03|04|05|06|07|08|09|0A|0B|0C|0D|0E|0F|10|11|12|13|14|15|16|17|18|19|1A|1B|
| float32 px| float32 py| float32 pz| float32 nx| float32 ny| float32 nz|cr|cg|cb|ca|

Full JSON description:

{
    "asset": {
        "version": "2.0"
    },
    "buffers": [
        {
            "uri": "data.bin",
            "byteLength": 84
        }
    ],
    "bufferViews": [
        {
            "buffer": 0,
            "byteOffset": 0,
            "byteLength": 84,
            "byteStride": 28
        }
    ],
    "accessors": [
        {
            "bufferView": 0,
            "byteOffset": 0,
            "componentType": 5126,
            "type": "VEC3",
            "count": 3,
            "min": [
                0,
                0,
                0
            ],
            "max": [
                1,
                1,
                1
            ]
        },
        {
            "bufferView": 0,
            "byteOffset": 12,
            "componentType": 5126,
            "type": "VEC3",
            "count": 3
        },
        {
            "bufferView": 0,
            "byteOffset": 24,
            "componentType": 5121,
            "type": "VEC4",
            "count": 3,
            "normalized": true
        }
    ],
    "meshes": [
        {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": 0,
                        "NORMAL": 1,
                        "COLOR_0": 2
                    }
                }
            ]
        }
    ],
    "nodes": [
        {
            "mesh": 0
        }
    ],
    "scenes": [
        {
            "nodes": [
                0
            ]
        }
    ],
    "scene": 0
}
prollin commented 6 years ago

This was my original understanding but this clarifies everything. Thank you very much!

zellski commented 6 years ago

I wonder if we could not profitably add @lexaknyazev's helpful ASCII graphic to the spec.

It always confused me that, when reading spec top-to-bottom, packed interleaved attributes are introduced hastily and implicitly with "Buffer view could have byteStride property. It means byte-distance between consequential elements. This field is defined only for buffer views that contain vertex attributes."

There's a slightly more descriptive passage in the data alignment section, but even there it's kind of an aside to the main point.

I think it could be worthwhile introducing this concept as if it was not already well-understood to the reader. I hesitate to propose language of my own, but I could if nobody more qualified felt called.

lexaknyazev commented 6 years ago

I'm not sure about spec rearrangement regarding interleaved attributes, but glTF-Tutorials has a full example with better images.

lexaknyazev commented 5 years ago

@pjcozzi @javagl This is clearly a documentation issue. How could we properly point readers to the relevant glTF-Tutorials section so that we do not extend the spec too much?

pjcozzi commented 5 years ago

In this case and inngeneral, it is OK with me for the spec to point to @javagl's official tutorials for more info on a topic.

javagl commented 5 years ago

Although I'm not strictly opposed to this, I'd hesitate to link from the spec to the tutorial. The spec should be the ultimate (and in that sense: complete) source of truth.

(BTW: The image in the tutorial was created with a small tool ("accessorVis") that I used internally, also for the images in the sample models (e.g. https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/Triangle#data-layout ). I already considered to make it available (as a part of JglTF), but .... time).

I was a bit surprised when I just noticed that the spec does not seem to refer to interleaved data at all (it's implicitly allowed by not being explicitly forbidden).

One suggestion could be to update the https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#buffers-and-buffer-views section . The part that says

When a buffer view contain vertex indices or attributes, they must be its only content, i.e., it's invalid to have more than one kind of data in the same buffer view.

Implementation Note: This allows a runtime to upload buffer view data to the GPU without any additional processing. [...]

Could be extended (probably inside the implementation note, to avoid ratification hassle), by emphasizing that


Above, @prollin said

If I understand properly, you can have multiple vertex attributes in the same buffer view but they need to be the same type (where type = accessor.ComponentType+accessor.Type).

But I think even this is not necessary. Correct me if I'm wrong, but I think that - even it would hardly make sense - it is allowed to have

+-----------------------------------------------------------------------+
| posX      | something | posY      |           | posZ      |           |
| (float)   | (float)   | (float)   |           | (float)   |           |    
+-----------------------------------------------------------------------+

Meaning that a POSITION VEC3(float) attribute and a something SCALAR(float) attribute are interleaved, and the "unneeded" elements are ignored, right? (I'm not even sure whether the component type would have to be float for the SCALAR attribute. I think it could also be int)

greggman commented 5 years ago

just to add I found this

i.e., it's invalid to have more than one kind of data in the same buffer view.

confusing as well. I sounds like I can't have FLOAT positions and UNSIGNED_BYTE colors in the same buffer view. My impression is that's not true, you can have both, but that part of the spec then seems hard to interpret and it might be helpful to reword it?

bjornbytes commented 5 years ago

I ran into this as well while writing an importer. It would be helpful to be able to easily deduce whether a bufferView needs to be uploaded to the GPU or not. For now I was able to work around it, though it's at the cost of some extra complexity in the importer.

It would be great if the spec either made target more reliable (i.e. if a bufferView is used for vertices or indices then the target must be present), or removed it completely, since it seems like it's always a mistake to use it when it's unreliable like this.

prollin commented 5 years ago

or removed it completely, since it seems like it's always a mistake to use it when it's unreliable like this.

I think this is the only option that makes sense. If it is unreliable, it is useless. Following the Mesh->Primitive->Accessor chain to reassemble the data has been the only reliable way to load every model I could get my hands on. It is a bit more work (and slow) than a memcpy could be but it is probably the only way to provide something flexible enough to export from most DCC. A clear example of how to reassemble vertex buffers this way in the doc would go a long way.

donmccurdy commented 3 years ago

In a future glTF version, I think we should consider requiring bufferView.target on vertex attribute and indices buffer views.

lexaknyazev commented 2 years ago

The relevant sections were clarified in the AsciiDoc spec release. Although the bufferView.target property is still optional (we cannot change that at this point), the exporters are explicitly encouraged to write it and the validator will be updated to flag when it's missing.

hybridherbst commented 2 years ago

@lexaknyazev one follow-up question here: I was just trying to implement this and confused about bufferView.target for sparse accessors; practically those do contain vertex/index data in my mind but I don't think it would make sense uploading them directly, so it should be left empty? That's also unclear. (https://doc.babylonjs.com/typedoc/interfaces/babylon.gltf2.iaccessorsparsevalues mentions that it must not be set)

lexaknyazev commented 2 years ago

Sparse accessors have more than one buffer view reference.

hybridherbst commented 2 years ago

Yes, I was referring to the bufferViews in accessor.sparse. Thanks for verifying!

I find the description in the spec confusing in that regard:

When a buffer view is used by vertex indices or attribute accessors it SHOULD specify bufferView.target with a value of element array buffer or array buffer respectively

as one could easily assume that indices and values in sparse accessors are bufferViews used by vertex indices or attribute accessors. I think the correct reading would be that they're not directly used by the accessor but indirectly used by the accessor.