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

Add support for stored tangents and add mesh attributes table #901

Merged
merged 6 commits into from
Apr 11, 2017

Conversation

bghgary
Copy link
Contributor

@bghgary bghgary commented Apr 10, 2017

No description provided.

|`TANGENT`|`"VEC4"`|`5126` (FLOAT)|XYZW vertex tangents where the w component is a signed float (-1, +1) indicating handedness of the tangent basis|
|`TEXCOORD_0`|`"VEC2"`|`5126` (FLOAT)|UV texture coordinates for the first set|
|`TEXCOORD_1`|`"VEC2"`|`5126` (FLOAT)|UV texture coordinates for the second set|
|`COLOR_0`|`"VEC3"`<br>`"VEC4"`|`5126`&nbsp;(FLOAT)|RGB or RGBA vertex color|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normalized UNSIGNED_BYTE should be also allowed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can add that.


|Name|Accessor Type(s)|Component Type(s)|Description|
|----|----------------|-----------------|-----------|
|`POSITION`|`"VEC3"`|`5126`&nbsp;(FLOAT)|XYZ vertex positions|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to define component type here?
WEB3D_quantized_accessors requires normalized UNSIGNED_SHORT.

In general, it's not important spec-wise what the data type is as long as it matches actually stored data. Shaders shouldn't be affected anyway.

Copy link
Contributor Author

@bghgary bghgary Apr 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that runtime implementations that have an abstraction for vertex data don't directly deal with buffers. BabylonJS, for example, can only accept number[] or Float32Array for uvs and vertex colors. I don't think we should leave it open ended.

Edited: After more thinking, revised wording so that it makes more sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, runtime should be able to read any vertex format using info from accessor's fields. I agree that for the core spec other types for vertex positions make little sense.
Can we say that required extensions (e.g. WEB3D_quantized_accessors) may extend this table?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say for any extension not just required extension may extend this table. This is analogous to the versioning concepts. I will add some text that states this explicitly.

|----|----------------|-----------------|-----------|
|`POSITION`|`"VEC3"`|`5126`&nbsp;(FLOAT)|XYZ vertex positions|
|`NORMAL`|`"VEC3"`|`5126`&nbsp;(FLOAT)|XYZ vertex normals|
|`TANGENT`|`"VEC4"`|`5126`&nbsp;(FLOAT)|XYZW vertex tangents where the w component is a signed float (-1, +1) indicating handedness of the tangent basis|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good candidate for 10-10-10-2 vertex format (ES 3.0+). Could be considered for the next glTF update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

|`POSITION`|`"VEC3"`|`5126`&nbsp;(FLOAT)|XYZ vertex positions|
|`NORMAL`|`"VEC3"`|`5126`&nbsp;(FLOAT)|XYZ vertex normals|
|`TANGENT`|`"VEC4"`|`5126`&nbsp;(FLOAT)|XYZW vertex tangents where the w component is a signed float (-1, +1) indicating handedness of the tangent basis|
|`TEXCOORD_0`|`"VEC2"`|`5126`&nbsp;(FLOAT)|UV texture coordinates for the first set|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normalized UNSIGNED_SHORT should be allowed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add normalized UNSIGNED_SHORT to all relevant attributes.

|`NORMAL`|`"VEC3"`|`5126`&nbsp;(FLOAT)|XYZ vertex normals|
|`TANGENT`|`"VEC4"`|`5126`&nbsp;(FLOAT)|XYZW vertex tangents where the w component is a signed float (-1, +1) indicating handedness of the tangent basis|
|`TEXCOORD_0`|`"VEC2"`|`5126`&nbsp;(FLOAT)|UV texture coordinates for the first set|
|`TEXCOORD_1`|`"VEC2"`|`5126`&nbsp;(FLOAT)|UV texture coordinates for the second set|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we limit the number of UVs to 2?
If so, we should state this more explicitly somewhere.

Copy link
Contributor Author

@bghgary bghgary Apr 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We need to limit them to something to prevent inconsistencies between implementations. 2 seemed reasonable to me considering we are running out of vertex attributes already for morph targets. I'll add more text on this.

@pjcozzi
Copy link
Member

pjcozzi commented Apr 11, 2017

+1

@sbtron sbtron mentioned this pull request Apr 11, 2017
@bghgary
Copy link
Contributor Author

bghgary commented Apr 11, 2017

This addresses #812.

@bghgary bghgary merged commit fb8b472 into 2.0 Apr 11, 2017
@bghgary bghgary deleted the storedTangents branch April 12, 2017 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants