KhronosGroup / glTF

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

Interleaved animation data #1864

Open lexaknyazev opened 4 years ago

lexaknyazev commented 4 years ago

The bufferView.byteStride was intended only for vertex attributes, its description says:

The stride, in bytes, between vertex attributes.

It turns out that some tools may interleave animation data and some engines even support that, see https://forum.babylonjs.com/t/gltf-animations-loading-sampler-data-from-interleaved-buffer/14091

We should clarify the spec whether it should be allowed.

/cc @donmccurdy @bghgary

donmccurdy commented 4 years ago

Hm, that'll certainly break in three.js now. We can fix this, but (like Babylon.js) it'll hit a slow path for us because we'd have to unpack the animation data. I'm mildly inclined to prohibit this, but would be glad to learn if there's some advantage to it, and at least one tool where that advantage has a quantifiable impact.

@bghgary although the generator string says Khronos glTF Blender I/O v0.9.2, Blender cannot export multiple .bin files like that. Do you mind asking the user what tool generated this file? Perhaps a custom fork of the Blender addon, or an optimizing tool like gltfpack?

bghgary commented 4 years ago

Yeah, I thought it was Blender due to that string. The user already says

I am not sure if this asset was created by Blender, we use multiple tools to process glTF assets and some are our own implementations.

Do you want me to ask specifically which tools?

donmccurdy commented 4 years ago

Hm, if it's something custom then I guess the specific tools don't matter. But if there is some particular reason their tool does that, it might be good to know.

donmccurdy commented 4 years ago

I think I've stumbled across one reason an exporter might choose to do this. From https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#bufferviewbytestride — 

bufferView.byteStride

The stride, in bytes, between vertex attributes. When this is not defined, data is tightly packed. When two or more accessors use the same bufferView, this field must be defined.

The "...this field must be defined" restriction is not explicitly limited to vertex attributes, so putting animation accessors with different byteStrides end-to-end in a buffer view would be prohibited. Exporters (reasonably) don't want to create more bufferViews than necessary, so interleaving animation accessors with the same number of keyframes would be one way to avoid that.

On the other hand, this property's meaning, "stride between vertex attributes", isn't well defined if the content isn't vertex attributes, so something should be clarified. Would it be OK to limit the restriction to vertex attributes? I guess that was the original intention.

When two or more accessors containing vertex attributes use the same bufferView, this field must be defined.

I made this assumption myself in https://github.com/donmccurdy/glTF-Transform/issues/99, tightly packing IBMs and animation input/output data in the same buffer view.

lexaknyazev commented 4 years ago

byteStride was never intended to be used with anything except vertex attributes (hence very specific min/max limits) because that requires engines to implement interleaved reads. At some point, the property was required for all vertex attributes but that was relaxed later (i.e. it could be omitted when each accessor uses its own buffer view).

Interleaving IBMs would prevent uploading them as a single UBO, so it should be avoided.

donmccurdy commented 4 years ago

@lexaknyazev Agreed on your comments above. In that case, though, why do the errors in https://github.com/donmccurdy/glTF-Transform/issues/99 appear? I'm not seeing why IBMs and animation in/out accessors couldn't be in the same buffer view.

lexaknyazev commented 4 years ago

I'm not seeing why IBMs and animation in/out accessors couldn't be in the same buffer view.

IIRC, this came from a somewhat implicit concept that glTF buffer view corresponds to a GL buffer object.

donmccurdy commented 4 years ago

The spec provides an explicit definition of bufferView.target — since IBMs aren't in it, I think we need to clarify the implicit parts or reduce the severity from 'error'. Are there any other implicit buffer view targets/usages?

lexaknyazev commented 4 years ago

Admittedly, the initial exclusion of interleaved layouts from non-VB data blocks didn't help with simplifying engines because interleaved accessors can still be sparse thus requiring an engine to implement interleaved reads and writes.

Are there any other implicit buffer view targets/usages?

For buffer views:

Apparently, there's no check implemented for buffer views referenced from morph data accessors.

For accessors (most of these have different formats anyway):

donmccurdy commented 4 years ago

It should be OK to put animation inputs and outputs in the same buffer view, correct?

lexaknyazev commented 4 years ago

Correct.

nmalisa-sb commented 4 years ago

Hello all, I am the user who asked for the bug fix in BabylonJS. We are actually putting animation sampler inputs and outputs into interleaved buffer to speed up reading it from memory. This enables us to read all animation data for one step in one or two memory reads instead of reading data from 4 separate buffers (data being time, position, rotation, scale for two consecutive frames).

zeux commented 3 years ago

The "two or more" wording is surprising / confusing; it feels like we should tighten the language to say "When two or more vertex data accessors use the same bufferView". In absence of this using multiple accessors with non-interleaved data in the same bufferView through byteOffset is prohibited unless byteStride is used, but the use of byteStride on non-vertex data seems counter to the intent of byteStride which should be used for vertex data as implied by the language "When a buffer view is used for vertex attribute data, it may have a byteStride property".

It seems like https://github.com/KhronosGroup/glTF/issues/1537 reached the same conclusion but that was never acted upon. Would anyone be opposed to me opening a PR with a clarification to that effect? (this would not change whether interleaving is valid or not, and would merely remove the byteStride usage requirement for non-interleaved non-vertex data cases).

lexaknyazev commented 3 years ago

@zeux Please proceed with a spec clarification update.

donmccurdy commented 3 years ago

Interesting point about benefits to interleaved animation data, as @nmalisa-sb suggests — it would be great if we could quantify this?

But ultimately, I think we have to make this decision based on backward-compatibility and current state of the ecosystem. If that means only vertex data can be interleaved, that is still something we could revisit in a future glTF version.

@lexaknyazev, you've asserted that The bufferView.byteStride was intended only for vertex attributes ... do you feel that this intention was clear enough that we should consider it a breaking change to relax that? Interleaved animation data is not supported by three.js now ... I'm willing to fix that, but would have to assume there are many other tools that also didn't expect this case ...

lexaknyazev commented 3 years ago

The updated spec is more explicit about byteStride usage. Interleaving animation data would require a new extension.