Skip to content
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

Move byteStride from accessor to bufferView? #827

Closed
kainino0x opened this issue Feb 3, 2017 · 15 comments
Closed

Move byteStride from accessor to bufferView? #827

kainino0x opened this issue Feb 3, 2017 · 15 comments

Comments

@kainino0x
Copy link
Contributor

According to #817 glTF 2.0 will target other graphics APIs. Apologies if this issue has been discussed, I couldn't find it.

In the newer graphics APIs (Metal, Vulkan, D3D12, D3D11, GL 4.3+, GL ES 3.1+), I think we need stride to be constant across multiple accessors pointing at the same bufferView. This is because stride is a per-input (per-buffer-binding) property while offsets are a per-attribute property.

The current structure of glTF is difficult (and slow) to shoehorn into these new designs. Is there anything preventing byteStride from being moved to bufferView?

Here's how I think the glTF concepts map to API concepts:

Vulkan: stride; accessor.offset

Metal: stride; accessor.offset

D3D12: stride; accessor.offset

D3D11: offset+stride; accessor.offset

OpenGL ES 3.1+, OpenGL 4.3+: offset+stride; accessor.offset

@lexaknyazev
Copy link
Member

Related: #819

@pjcozzi
Copy link
Member

pjcozzi commented Feb 4, 2017

@sbtron do you have any input here?

@kainino0x we can think through moving byteStride to bufferView, but it would need to happen quickly so we can get it into glTF 2.0 so we minimize future breaking changes.

The other options that may be lower impact is that other profiles (right now glTF just has the WebGL project) provide a guarantee that, for example, all accessors pointed to by a primitive have the same byteStride.

@lexaknyazev
Copy link
Member

At the first glance, such change is justifiable by API design (if I understood them correctly, see images below: one buffer for positions and other for normals and UVs). One of possible reasons to store different attributes in different buffers is to disable some of them on some passes (e.g., disable everything but positions on shadow pass).

OpenGL (ES):

opengl

Vulkan:

vulkan

Metal:

metal


However, glTF has other means for bufferViews and accessors.

  • Embedded images (via image.bufferView). Stride must be 0, easy.
  • Inverse bind matrices. Stride must be 0, also easy.
  • Animations (animation.sampler.input, animation.sampler.output). With guaranteed same stride across all accessors, we could think of interleaving animation data. I'm not sure about it though.
  • Sparse accessors. @emilian0, any thoughts?

Anyway, I can't see any cons to this change, so let's do it.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 5, 2017

@lexaknyazev this is kind of just pushing around the restrictions, right? Without changing anything, a profile could guarantee that, for example, all accessors pointed to by a primitive have the same byteStride.

If we change this, we would have the restrictions you bulleted above, plus the extra implementation requirement to load animation and morphs (stride isn't just passed to an API for these like it is for vertices), unless we require those strides to also be zero (which again is all about just where we want the restriction to be).

@lexaknyazev
Copy link
Member

For attribute-related accessors, this change reduces redundancy (and possible errors). Having such restriction in the core reduces 2.0 implementations fragmentation (btw, we don't have any profile yet - maybe we don't need them).

extra implementation requirement to load animation and morphs

Currently, I'm sure that their stride must be 0 anyway (because reading interleaved data on client isn't easy). So there's zero implementation cost here.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 5, 2017

extra implementation requirement to load animation and morphs

Currently, I'm sure that their stride must be 0 anyway (because reading interleaved data on client isn't easy). So there's zero implementation cost here.

If all bufferView strides except vertex attributes would have to be zero, I agree this becomes attractive. It would be OK with me, if everyone agrees.

It could lead to more bufferViews to compensate, but I don't think that is an issue nor do I think it will happen often in practice.

@lexaknyazev
Copy link
Member

It could lead to more bufferViews to compensate, but I don't think that is an issue nor do I think it will happen often in practice.

How?

Case when several accessors use the same bufferView with different strides (e.g., sequence of separate meshes with different attribute layouts) must be discouraged from implementation's point of view.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 5, 2017

How?

Current scenario of two accessors with different strides referencing the same bufferView.

It's not a big deal, just pointing it out.

@pjcozzi
Copy link
Member

pjcozzi commented Jun 15, 2017

Updated in #826

@pjcozzi pjcozzi closed this as completed Jun 15, 2017
@javagl
Copy link
Contributor

javagl commented Aug 27, 2017

Just to get the implications of this right (mainly @lexaknyazev ) :

Assuming one has

  • vertex positions (as a float[n*3] array) and
  • texture coordinates (as a float[n*2] array)

Is it right that it is not possible to create accessor objects for these data blocks unless one already knows whether they will end up in the same bufferView?

And if they end up in the same bufferView, then the float[n*2] array of the texture coordinates has to be padded with zeros...

old: | x0 | y0 | x1 | y1 | ...  | xn | yn |

new: | x0 | y0 | 0  | x1 | y1 | 0  | ... | xn | yn | 0  |
                 ^              ^                    ^

in order to have the same byteStride that is used for the float[n*3] array that is used for the "positions"?

@lexaknyazev
Copy link
Member

@javagl

Is it right that it is not possible to create accessor objects for these data blocks unless one already knows whether they will end up in the same bufferView?

Right. But that's also because of byteOffset.

And if they end up in the same bufferView, then the float[n*2] array of the texture coordinates has to be padded with zeros...

Minimum stride of float VEC3 is 4 * 3 = 12 bytes. Minimum stride of float VEC2 is 4 * 2 = 8 bytes.
To store them in the same bufferView, one must pad VEC2 with zeros (as in your example) or interleave data like this:

x0 y0 z0 t0 s0 x1 y1 z1 t1 s1 x2 y2 z2 t2 s2 

With stride of 4 * (3 + 2) = 20 bytes.

@javagl
Copy link
Contributor

javagl commented Aug 28, 2017

Thanks for the clarification. The main issue here (for me, currently) is that the data layout of the texture coordinate data will change. Adjusting the byteOffset later is easy. But padding a float[2*n] to become a float[2*n+n] is a comparatively complex "post-processing" step. However, it's solvable, of course.

@lexaknyazev
Copy link
Member

Adjusting the byteOffset later is easy. But padding a float[2*n] to become a float[2*n+n] is a comparatively complex "post-processing" step.

Someone has to do it anyway: some APIs (e.g., Vulkan) don't allow loading GPU buffer with different strides. It's better to put the burden on export side. To avoid "post-processing", one could use a new bufferView for each accessor type (e.g., the first bufferView for all VEC3 attributes, the second for VEC2).

@javagl
Copy link
Contributor

javagl commented Sep 1, 2017

Although I'm not an expert here, the structure of having multiple accessors that refer to the same buffer view probably has a reason. Most likely, it's related to some performance issues that could be caused by having to bind one VBO (buffer view) for each attribute (regardless of whether these performance concerns are actually justified).


However, related to that, and still related to this issue:

Right now, the bufferView.byteStride is optional. If it is present, it is constrained to be >=4. The spec also says "When two or more accessors use the same bufferView, this field must be defined.".

So it is not possible to have two accessor instances that refer to the same bufferView when both accessors should be used for indices. (E.g. it is not possible to mix an unsigned short indices accessor and an unsigned int indices accessor in the same bufferView). Right?

(I didn't find this case being mentioned in the spec, but maybe I overlooked it...)

@lexaknyazev
Copy link
Member

@javagl

Right now, the bufferView.byteStride is optional. If it is present, it is constrained to be >=4. The spec also says "When two or more accessors use the same bufferView, this field must be defined.".

So it is not possible to have two accessor instances that refer to the same bufferView when both accessors should be used for indices. (E.g. it is not possible to mix an unsigned short indices accessor and an unsigned int indices accessor in the same bufferView). Right?

That field is defined only for vertex attributes (in GL sense). Indices accessors of different types are free to use the same bufferView.

Could you open a PR with more consistent wording for all relevant places in the spec?

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

No branches or pull requests

4 participants