Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Tightening indexed attribute semantics #1023

Closed
lexaknyazev opened this issue Jun 18, 2017 · 9 comments
Closed

Tightening indexed attribute semantics #1023

lexaknyazev opened this issue Jun 18, 2017 · 9 comments
Assignees

Comments

@lexaknyazev
Copy link
Member

lexaknyazev commented Jun 18, 2017

From meshes:

Client implementations must support at least two UV texture coordinate sets, one vertex color, and one joints/weights set.

Atm, spec doesn't require indexed semantic to start with 0. This leads to couple tricky cases:

{
    "meshes": [
        {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": 0,
                        "TEXCOORD_1": 1,
                        "JOINTS_1": 2
                    }
                }
            ]
        }
    ]
}
  1. Could UV set TEXCOORD_1 be declared without TEXCOORD_0?
  2. Same question about JOINTS_1.
  3. Could there be two UV sets TEXCOORD_1 and TEXCOORD_2 (let's say that material's texture info uses matching indices)?

Spec has a table with valid semantic names that start with 0, but it's not as strict as it should be, imho.
Also, we should add that JOINTS and WEIGHTS semantics must be both present or both absent.

CC @pjcozzi @bghgary @UX3D-nopper @javagl

@McNopper
Copy link
Contributor

Suggest, that index has to start with 0 (no 1, 2, 3) and no "gaps" (no 0, 1, 3, 4) are allowed.

@javagl
Copy link
Contributor

javagl commented Jun 18, 2017

The description of TEXCOORD_0 as "UV texture coordinates for the first set", and the pattern "...must be of the form [semantic]_[set_index]" does not leave very much freedom here, but indeed, it could be worded more clearly.

However, I'll have to think about where this actually matters in the implementation. In the end, these are just strings. I assume that this aims at preventing implementations from doing short-sighted things like

if (!attributeKeys.contains("TEXCOORD_0")) { /* no texture coordinates here */ }

which may overlook a TEXCOORD_1, right?

@McNopper
Copy link
Contributor

So, TEXCOORD_100 would be allowed?
The loaders just get simpler, when the index starts with 0 and so on.

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Jun 18, 2017

There could be a corner case with undefined TEXCOORD_0, but defined TEXCOORD_1, could you evaluate its validity?

Both primitives use the same material, but the second one uses vertex colors instead of baseColorTexture.

{
    "materials": [
        {
            "pbrMetallicRoughness": {
                "baseColorTexture": {
                    "index": 0,
                    "texCoord": 0
                }
            },
            "occlusionTexture": {
                "index": 1,
                "texCoord": 1
            }
        }
    ],
    "meshes": [
        {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": 0,
                        "TEXCOORD_0": 1,
                        "TEXCOORD_1": 2
                    },
                    "material": 0
                },
                {
                    "attributes": {
                        "POSITION": 3,
                        "COLOR_0": 4,
                        "TEXCOORD_1": 5
                    },
                    "material": 0
                }
            ]
        }
    ]
}

@UX3D-nopper
Copy link
Contributor

In my opinion, the above glTF is invalid:

  • The second primitive uses a material, which requires two texture coordinates but does only have one set.

If a material is assigned to a primitive, it is expected, that all parameters are used and finally the visual output is as expected. In my opinion it should not be allowed, that material parameters are discarded, just because the primitive does not have the required parameters.

Any exporter has to take care of this.

I have done this by hand but I think this is the way an exporter should solve this:

{
    "materials": [
        {
            "pbrMetallicRoughness": {
                "baseColorTexture": {
                    "index": 0,
                    "texCoord": 0
                }
            },
            "occlusionTexture": {
                "index": 1,
                "texCoord": 1
            }
        },
        {
            "occlusionTexture": {
                "index": 0,
                "texCoord": 0
            }
        }
    ],
    "meshes": [
        {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": 0,
                        "TEXCCORD_0": 1,
                        "TEXCOORD_1": 2
                    },
                    "material": 0
                },
                {
                    "attributes": {
                        "POSITION": 3,
                        "COLOR_0": 4,
                        "TEXCOORD_0": 5
                    },
                    "material": 1
                }
            ]
        }
    ]
}

@lexaknyazev
Copy link
Member Author

it should not be allowed, that material parameters are discarded, just because the primitive does not have the required parameters

I agree with this and with

index starts with 0

@pjcozzi @bghgary any objections?

@pjcozzi
Copy link
Member

pjcozzi commented Jun 19, 2017

No objections.

@bghgary
Copy link
Contributor

bghgary commented Jun 19, 2017

Seems reasonable to me.

@pjcozzi
Copy link
Member

pjcozzi commented Oct 19, 2017

Discussed on a 3D Formats call.

  • All indices for attribute semantics, must start with 0 and be continuous: TEXCOORD_0, TEXCOORD_1, etc.
  • Materials can't reference to non existing attributes.

@lexaknyazev volunteered to make this explicit in this spec and to update the validator.

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

No branches or pull requests

6 participants