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

Accessor alignment requirements #802

Closed
javagl opened this issue Dec 31, 2016 · 8 comments
Closed

Accessor alignment requirements #802

javagl opened this issue Dec 31, 2016 · 8 comments

Comments

@javagl
Copy link
Contributor

javagl commented Dec 31, 2016

The alignment constraints for accessors currently say:

The offset of an accessor into a bufferView (i.e., accessor.byteOffset) and the offset of an accessor into a buffer (i.e., accessor.byteOffset + bufferView.byteOffset) must be a multiple of the size of the accessor's attribute type.

where "the size of the accessor's attribute type" is defined to be the size of the full type, (e.g. for a 3D float vector, it's 3*4 bytes.

According to the discussion at KhronosGroup/glTF-Validator#10 , it may not be necessary to require the alignment to be the size of the full type. It might be sufficient to have an alignment that matches the size of the component type (e.g. 4 for a 3D float vector).

If and only if nobody can imagine a reason to keep the alignment requirement in its current form, it could be relaxed (always keeping in mind that it can afterwards never be changed back).

@pjcozzi
Copy link
Member

pjcozzi commented Jan 4, 2017

Relaxing is OK with me as long as JavaScript typed array alignment requirements are met such that views can be created without having to copy subsets of the buffer. That was the original intent; I may have made it to restrictive.

CC @lexaknyazev

@lexaknyazev
Copy link
Member

From offline discussion:

  • Spec must enforce 4- or 2-byte (depending on componentType) deep alignment for all accessors, to enable fastpath for JS reading (this is already implemented everywhere, including validator);
  • Spec should enforce 4-byte alignment for attribute accessors (for each vertex) to follow GPU efficiency practices (new constraint, only Metal demands it, other APIs just recommend so it could be a warning).

@javagl
Copy link
Contributor Author

javagl commented Jan 7, 2017

So I assume that the alignment requirements will be relaxed in this point for 1.1.


This may make things simpler. And this may come in handy, because I think that obeying the alignment constraints of glTF 1.0 is surprisingly tricky. I'm currently working on this, and my gut feeling is that I overlooked something obvious or simply got lost somehow. But here's an example, to illustrate what I'm currently struggling with.

Imagine the configuration shown in this image:

square

(This configuration is not valid, as shown below)

To summarize:

The accessors:

  • An accessor for indices with 12 bytes, and an alignment of 2
  • An accessor for positions with 48 bytes and an alignment of 12
  • An accessor for normals with 48 bytes and an alignment of 12
  • An accessor for texCoords0 with 32 bytes and an alignment of 8

The buffer views:

  • An indicesBufferView with the indices accessor
  • An attributesBufferView with the positions, normals and texCoords0 accessors

All this should be combined into a single buffer.

So the buffer will start with the indicesBufferView, containing 12 bytes.
Then comes the attributesBufferView. Its byteOffset is 12 bytes.
The data of the positions accessor can be added without any padding, filling the bytes 12-60 of the buffer (which are bytes 0-48 of the attributesBufferView)
The data of the normals accessor can be added without any padding, filling the bytes 60-108 of the buffer (which are bytes 48-96 of the attributesBufferView)
Now comes the texCoords0 accessor data:

  • The current number of bytes in the attributesBufferView is 96. This is divisible by 12.
  • The current number of bytes in the buffer is 108. This is not divisible by 12. A padding will be necessary.

But how?

Adding 4 to the byteOffset of the textCoords0 accessor (as done in the image) will yield a proper alignment referring to the buffer. But then, the alignment for the texCoords inside the attributesBufferView will be wrong. So the aligment requirement cannot be obeyed without going back and inserting a padding before the attributesBufferView. But then, additional paddings before the positions accessor and before the normals accessor will be necessary.

Of course, it can be solved. One could generously spread padding bytes here and there and everywhere, based on the smallest common multiple of all alignment requirements, to trivially make sure that all constraints are obeyed. But when I thought about how to compute an optimal structure, with as few padding bytes as possible, this started feeling like a somewhat non-trivial constraint satisfaction problem. (I don't want to suggest that it may be NP complete, but it wouldn't surprise me if it was).

Am I overlooking something obvious here?

@javagl
Copy link
Contributor Author

javagl commented Jan 8, 2017

Probably not. It is difficult.

One reasonable strategy is to make sure that the byteOffset of each bufferView is divisible by the smallest common multiple of the alignment requirements of all accessor objects that are contained in the bufferView. This makes sure that all accessors can be aligned properly, and that the byteOffset of each accessor can be computed locally, ignoring the byteOffset of its surrounding bufferView (because this is known to be a multiple of the accessor's alignment requirement).

This may cause some bytes to be wasted. But computing the optimal alignment (maybe even considering reorderings of the accessor objects of a bufferView!) seems to be challenging, and I'm not yet bored enough to tackle this.

@lexaknyazev
Copy link
Member

As @pjcozzi said, that wasn't intended restriction. Let's just integrate requirements from post above.

@javagl
Copy link
Contributor Author

javagl commented Jan 9, 2017

Sure, that was my assumption.


The part below the ---line--- was rather an approach to dissect the implications of the original spec. For example, the models

BoxAnimated
BrainStem
CesiumMan
CesiumMilkTruck
Monster
RiggedFigure
RiggedSimple
VC
WalkingLady

are not compliant to the original specification in this regard. But this may not be sooo important, because the validator did not check it, and the existing loaders will not unnecessarily insist on this part of the spec. They should not have a reason to do this, and thus, it is unlikely that they reject one of the above models as being invalid.

@lexaknyazev
Copy link
Member

Alignment reqs updated via lexaknyazev@2113534.

@javagl
Copy link
Contributor Author

javagl commented Feb 11, 2017

The spec has been updated, so I guess this can be closed.

@javagl javagl closed this as completed Feb 11, 2017
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

3 participants