Skip to content

Commit

Permalink
Add support for stored tangents and add mesh attributes table (#901)
Browse files Browse the repository at this point in the history
* Add support for tangents and specify mesh attribute limits.

* Some corrections and more descriptions for attributes

* Updates based on feedback

* More updates based on feedback
  • Loading branch information
bghgary authored Apr 11, 2017
1 parent 594c3c9 commit fb8b472
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 8 deletions.
34 changes: 27 additions & 7 deletions specification/2.0/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ In glTF, meshes are defined as arrays of *primitives*. Primitives correspond to

> **Implementation note:** Splitting one mesh into *primitives* could be useful to limit number of indices per draw call.
If `material` is not supplied, and no extension is present that defines material properties, then the object should be rendered using a 50% gray emissive color.
If `material` is not supplied, then the object should be rendered using a 50% gray PBR metallic-roughness material with `[ 0.5, 0.5, 0.5 ]` for `baseColorFactor`, `0` for `metallicFactor`, and `1` for `roughnessFactor`.

The following example defines a mesh containing one triangle set primitive:

Expand All @@ -636,9 +636,10 @@ The following example defines a mesh containing one triangle set primitive:
"primitives": [
{
"attributes": {
"NORMAL": 25,
"POSITION": 23,
"TEXCOORD_0": 27
"NORMAL": 23,
"POSITION": 22,
"TANGENT": 24,
"TEXCOORD_0": 25
},
"indices": 21,
"material": 3,
Expand All @@ -652,13 +653,32 @@ The following example defines a mesh containing one triangle set primitive:

Each attribute is defined as a property of the `attributes` object. The name of the property corresponds to an enumerated value identifying the vertex attribute, such as `POSITION`. The value of the property is the index of an accessor that contains the data.

Valid attribute semantic property names include `POSITION`, `NORMAL`, `TEXCOORD`, `COLOR`, `JOINT`, and `WEIGHT`. `TEXCOORD` and `COLOR` attribute semantic property names must be of the form `[semantic]_[set_index]`, e.g., `TEXCOORD_0`, `TEXCOORD_1`, `COLOR_1`, etc. For forward-compatibility, application-specific semantics must start with an underscore, e.g., `_TEMPERATURE`.
Valid attribute semantic property names include `POSITION`, `NORMAL`, `TANGENT`, `TEXCOORD_0`, `TEXCOORD_1`, `COLOR_0`, `JOINT`, and `WEIGHT`. Application-specific semantics must start with an underscore, e.g., `_TEMPERATURE`.

Each mesh has a limit of two UV texture coordinate sets and one vertex color.

This comment has been minimized.

Copy link
@lexaknyazev

lexaknyazev Apr 12, 2017

Member

Can we make this line more flexible? Like that:

TEXCOORD, COLOR, JOINTS, and WEIGHTS attribute semantic property names must be of the form [semantic]_[set_index], e.g., TEXCOORD_0, TEXCOORD_1, COLOR_0. Clients must support at least two UV texture coordinate sets, one vertex color, and one joints/weights set.

In that case, the "meaning" of TEXCOORD_2 or JOINTS_1 will be defined, but clients don't have to support it.

This comment has been minimized.

Copy link
@McNopper

McNopper Apr 12, 2017

Contributor

+1
Furthermore, after 2.0 spec is final, we can either define an extension and/or profile, where e.g.
JOINTS_1/WEIGHTS_1 has to be supported.

This comment has been minimized.

Copy link
@bghgary

bghgary Apr 12, 2017

Author Contributor

Sure. I'll make this change.


Valid accessor type and component type for each attribute semantic property are defined below.

|Name|Accessor Type(s)|Component Type(s)|Description|
|----|----------------|-----------------|-----------|
|`POSITION`|`"VEC3"`|`5126` (FLOAT)|XYZ vertex positions|
|`NORMAL`|`"VEC3"`|`5126` (FLOAT)|XYZ vertex normals|
|`TANGENT`|`"VEC4"`|`5126` (FLOAT)|XYZW vertex tangents where the w component is a sign value (-1 or +1) indicating handedness of the tangent basis|
|`TEXCOORD_0`|`"VEC2"`|`5126`&nbsp;(FLOAT)<br>`5120`&nbsp;(UNSIGNED_BYTE)&nbsp;normalized<br>`5123`&nbsp;(UNSIGNED_SHORT)&nbsp;normalized|UV texture coordinates for the first set|
|`TEXCOORD_1`|`"VEC2"`|`5126`&nbsp;(FLOAT)<br>`5120`&nbsp;(UNSIGNED_BYTE)&nbsp;normalized<br>`5123`&nbsp;(UNSIGNED_SHORT)&nbsp;normalized|UV texture coordinates for the second set|
|`COLOR_0`|`"VEC3"`<br>`"VEC4"`|`5126`&nbsp;(FLOAT)<br>`5120`&nbsp;(UNSIGNED_BYTE)&nbsp;normalized<br>`5123`&nbsp;(UNSIGNED_SHORT)&nbsp;normalized|RGB or RGBA vertex color|
|`JOINT`|`"VEC4"`|`5120`&nbsp;(UNSIGNED_BYTE)<br>`5123`&nbsp;(UNSIGNED_SHORT)|See [Morph Targets](#morph-targets) or [Skins](#skins)|
|`WEIGHT`|`"VEC4`|`5126`&nbsp;(FLOAT)<br>`5120`&nbsp;(UNSIGNED_BYTE)&nbsp;normalized<br>`5123`&nbsp;(UNSIGNED_SHORT)&nbsp;normalized|See [Morph Targets](#morph-targets)|

Extensions can add additional property names, accessor types, and/or accessor component types.

> **Implementation note:** Each primitive corresponds to one WebGL draw call (engines are, of course, free to batch draw calls). When a primitive's `indices` property is defined, it references the accessor to use for index data, and GL's `drawElements` function should be used. When the `indices` property is not defined, GL's `drawArrays` function should be used with a count equal to the count property of any of the accessors referenced by the `attributes` property (they are all equal for a given primitive).
#### Tangent-space definition
> **Implementation note:** When normals are not specified, implementations should calculate smoothed normals.
**TODO**
> **Implementation note:** When tangents are not specified, implementations should calculate tangents using default MikkTSpace algorithms. For best results, the mesh triangles should also be processed using default MikkTSpace algorithms.
> **Implementation note:** When normals and tangents are specified, implementations should compute the bitangent by taking the cross product of the normal and tangent xyz vectors and multiplying against the w component of the tangent: `bitangent = cross(normal, tangent.xyz) * tangent.w`
#### Morph Targets

Expand Down
2 changes: 1 addition & 1 deletion specification/2.0/schema/mesh.primitive.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
},
"targets": {
"type": "array",
"description": "An array of Morph Targets, each Morph Target is a dictionary mapping attributes (only \"POSITION\" and \"NORMAL\" supported) to their deviations in the Morph Target.",
"description": "An array of Morph Targets, each Morph Target is a dictionary mapping attributes (only `POSITION`, `NORMAL`, and `TANGENT` supported) to their deviations in the Morph Target.",
"items": {
"$ref": "mesh.primitive.target.schema.json"
},
Expand Down

29 comments on commit fb8b472

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason, that exactly two texture coordinate sets can be present.
Also, will WEIGHT and JOINT changed to the _X format as well?

@bghgary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reason, that exactly two texture coordinate sets can be present.

Limiting UV sets to a fixed number ensures that all implementations have consistent support for the number of UV sets. Additionally, because of the addition of morph targets, we need to limit the number of total vertex attributes that can be present at the same time. Most web browsers can only support up to 16 vertex attributes.

Also, will WEIGHT and JOINT changed to the _X format as well?

My understanding is no. See https://github.com/KhronosGroup/glTF/tree/2.0/specification/2.0#skinned-mesh-attributes.

@McNopper
Copy link
Contributor

@McNopper McNopper commented on fb8b472 Apr 11, 2017

Choose a reason for hiding this comment

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

Well, it would be great, that WEIGHT and JOINT are also with named _X. Because - already made a ticket for this - that later on more influences could be used e.g. in an extension.

But why exactly two uv sets and not just one? For which use case?

@lexaknyazev
Copy link
Member

Choose a reason for hiding this comment

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

@bghgary
I agree with JOINTS / WEIGHTS change. We had concerns about hard-limit on number of active joints before. This issue is similar to the number of active morph targets, so let it be up to client.

@bghgary
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, sounds good. I just copied from the existing spec and wasn't trying to deal with issue #878. @lexaknyazev Do you want to make this change?

@bghgary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why exactly two uv sets and not just one? For which use case?

We added support for referencing multi UVs for #742.

@McNopper
Copy link
Contributor

@McNopper McNopper commented on fb8b472 Apr 11, 2017

Choose a reason for hiding this comment

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

Thx - also, can someone comment on the uvs?
I know, that three.js is normally using two uvs sets, where specific(!) textures uses dedicated uvs. If this is the purpose of the the two uvs, then the current spec allows too much.
E.g. I can assign the baseColor to uv1, the metallic roughness to uv0, emissive to uv1 and so on.
All combinations are currently allowed by the spec.

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay we just crossed us.
Let me check,

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I expected. The original request comes form the AO map, where a second UV set makes sense, E.g. three.js assigns AO to the second uv set by default.

Back to glTF and the current spec:
What is the suggested solution for this?
Pass to the shader, which texture is using which texture coordinate and making an if else before texcoord fetch? Or is it expected, that the shader is automatically generated?

@bghgary
Copy link
Contributor Author

@bghgary bghgary commented on fb8b472 Apr 11, 2017

Choose a reason for hiding this comment

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

Pass to the shader, which texture is using which texture coordinate and making an if else before texcoord fetch? Or is it expected, that the shader is automatically generated?

I think this is up to the implementation. BabylonJS, for example, does this: https://github.com/BabylonJS/Babylon.js/blob/master/src/Shaders/pbr.vertex.fx#L138

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks a lot.

@lexaknyazev
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to make this change?

Sure. I just need to carefully go through all mentions in the spec and check how existing use cases are affected by this.

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Following glTF snippet:
"materials" : [
{
"name" : "Cube",
"pbrMetallicRoughness" : {}
}
],
is a valid, minimal PBR material.
Can we agree, that this is also the default PBR material, when no material is provided?

It looks like this:
grafik

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also change to:
"... Implementation note: When normals are not specified, implementations should calculate flat normals. ..."

One simple reason:

  • Easier, faster to implement, as it is just the normalized cross product of two edges from a processed triangle.

The background reason is, that if someone wants to write a simple, basic, glTF compliant rendered, one has to implement quite a lot.

@lexaknyazev
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion here, but implementing smooth normals doesn't seem to be the most difficult part of glTF 2.0 feature set. On the other hand, simple models (like cube or tetrahedron) will benefit from not storing normals.

@McNopper
Copy link
Contributor

@McNopper McNopper commented on fb8b472 Apr 12, 2017

Choose a reason for hiding this comment

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

I totally agree, that simple models needs to be supported with as much minimal data as possible. E.g. triangles and cubes can perfectly be used for mathematical/scientifc applications - even in documents,

Regarding the smoothing: Several smothing algorithms do exist. Averaging just the normals, averaging the normals dependign on adjacent face area and so on. The output can differ much, which finally results in different rendering result - which glTF tries to avoid.

Also, please take into account, that glTF will also be used in embedded and or IOT devices, where CPU power is limited. So, every code, which has to be executed and present, is "expensive" in regards of execution time and code footprint.
The problem primarly does not exist on Smartphones or the Browser, as using WebGL and or OpenGL ES with having a relative strong GPU and SoC/CPU, the code is executed quite fast.

Finally, a cube, thetadron and so on, the normals should not be smoothed. Also, using 3D content creation tools like Blender, the intial created mesh is normally not smoothed - even the sphere.
Also, if a mesh needs normals - for any reason - then these should be exported and not depend on any assumptions in the specification.

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, but the other problem with flat is, that e.g. the cube is defined with 8 vertices, the cube needs to be extracted, that per face normals can be used.

@lexaknyazev
Copy link
Member

Choose a reason for hiding this comment

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

That's an implementation detail. Duplicating seems to be easier than computing averages. Moreover, there's no need in such duplicating with GL ES 3.0+.

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean using provoking vertices?

@lexaknyazev
Copy link
Member

Choose a reason for hiding this comment

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

You mean using provoking vertices?

Yes. Maybe it's not very efficient for cubes, but triangle-based flat-shaded meshes could benefit from that.

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree for triangles, but for cubes, especially complex meshes, finding the proviking vertex takes some compute effort. So as you suggested, dublicating is the better approach.

@bghgary Can we agree on flat as default?

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

(if @lexaknyazev agrees as well)

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

@bghgary Also, changing the default material (see above)

@lexaknyazev
Copy link
Member

Choose a reason for hiding this comment

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

I agree on flat as default mainly because of:

Regarding the smoothing: Several smothing algorithms do exist. Averaging just the normals, averaging the normals dependign on adjacent face area and so on. The output can differ much, which finally results in different rendering result - which glTF tries to avoid.

@McNopper How does your proposed default material differ from the current one?

If material is not supplied, then the object should be rendered using a 50% gray PBR metallic-roughness material with [ 0.5, 0.5, 0.5 ] for baseColorFactor, 0 for metallicFactor, and 1 for roughnessFactor.

@bghgary Could you make default material definition more formal (i.e. add a header to it and a reference in ToC) and maybe move it to materials section (with a cross-link from current location)?

@McNopper
Copy link
Contributor

@McNopper McNopper commented on fb8b472 Apr 12, 2017

Choose a reason for hiding this comment

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

I am just using the default values descibed here:
https://github.com/KhronosGroup/glTF/blob/2.0/specification/2.0/schema/material.pbrMetallicRoughness.schema.json
So
[ 1.0, 1.0, 1.0, 1.0 ] for baseColorFactor
1.0 for metallicFactor
1.0 for roughnessFactor
[ 0.0, 0.0, 0.0 ] for emissiveFactor

As no occlusionTexture is present, the occlusion strength is not used.

So
"materials" : [
{
"name" : "Cube",
"pbrMetallicRoughness" : {}
}
],

results in the above image, which is also somehow a grey.

So no material is equal to no material parameters present.

@bghgary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you make default material definition more formal (i.e. add a header to it and a reference in ToC) and maybe move it to materials section (with a cross-link from current location)?

Yes, I can make that change.

@bghgary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we agree on flat as default?

I don't have a strong preference either. People who want smooth normals can always specify the normals. I put smooth because we needed to put something to ensure consistency and smooth was fairly typical in the engines I've looked at. I'll change it to flat.

@bghgary
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've addressed all of these with #910.

@lexaknyazev @McNopper Please review.

@McNopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree on this. Thanks for the changes,

Please sign in to comment.