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

Attribute names must have an underscore prefix #611

Closed
javagl opened this issue Jan 6, 2022 · 5 comments
Closed

Attribute names must have an underscore prefix #611

javagl opened this issue Jan 6, 2022 · 5 comments

Comments

@javagl
Copy link
Contributor

javagl commented Jan 6, 2022

The Vertex Attribute section says

Names of feature ID attribute semantics follow the naming convention FEATURE_ID_n where n must start with 0 and continue with consecutive positive integers: FEATURE_ID_0, FEATURE_ID_1, etc.

The glTF specification says

Application-specific attribute semantics MUST start with an underscore, e.g., _TEMPERATURE .

The following is a (trivial, complete, embedded, standalone) glTF asset with a quad that defines feature ID attributes

{
  "extensionsUsed" : [ "EXT_mesh_features" ],
  "accessors" : [ {
    "bufferView" : 0,
    "byteOffset" : 0,
    "componentType" : 5123,
    "count" : 6,
    "type" : "SCALAR",
    "max" : [ 3 ],
    "min" : [ 0 ]
  }, {
    "bufferView" : 1,
    "byteOffset" : 0,
    "componentType" : 5126,
    "count" : 4,
    "type" : "VEC3",
    "max" : [ 1.0, 1.0, 0.0 ],
    "min" : [ 0.0, 0.0, 0.0 ]
  }, {
    "bufferView" : 1,
    "byteOffset" : 48,
    "componentType" : 5126,
    "count" : 4,
    "type" : "VEC3",
    "max" : [ 0.0, 0.0, 1.0 ],
    "min" : [ 0.0, 0.0, 1.0 ]
  }, {
    "bufferView" : 1,
    "byteOffset" : 96,
    "componentType" : 5121,
    "count" : 4,
    "type" : "SCALAR",
    "max" : [ 45 ],
    "min" : [ 12 ]
  } ],
  "asset" : {
    "generator" : "JglTF from https://github.com/javagl/JglTF",
    "version" : "2.0"
  },
  "buffers" : [ {
    "uri" : "data:application/gltf-buffer;base64,AAABAAIAAQADAAIAAAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAAAACAPwAAgD8AAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAIA/AAAAAAAAAAAAAIA/AAAAAAAAAAAAAIA/DAAAAAAAAAAAAAAAFwAAAAAAAAAAAAAAIgAAAAAAAAAAAAAALQAAAAAAAAAAAAAA",
    "byteLength" : 156
  } ],
  "bufferViews" : [ {
    "buffer" : 0,
    "byteOffset" : 0,
    "byteLength" : 12,
    "target" : 34963
  }, {
    "buffer" : 0,
    "byteOffset" : 12,
    "byteLength" : 144,
    "byteStride" : 12,
    "target" : 34962
  } ],
  "materials" : [ {
    "pbrMetallicRoughness" : {
      "baseColorFactor" : [ 0.5, 1.0, 0.5, 1.0 ],
      "metallicFactor" : 0.0,
      "roughnessFactor" : 1.0
    },
    "alphaMode" : "OPAQUE",
    "doubleSided" : false
  } ],
  "meshes" : [ {
    "primitives" : [ {
      "extensions" : {
        "EXT_mesh_features" : {
          "featureIds" : [ {
            "attribute" : 0
          }, {
            "offset" : 5,
            "repeat" : 2
          } ]
        }
      },
      "attributes" : {
        "POSITION" : 1,
        "NORMAL" : 2,
        "FEATURE_ID_0" : 3
      },
      "indices" : 0,
      "material" : 0,
      "mode" : 4
    } ]
  } ],
  "nodes" : [ {
    "mesh" : 0
  } ],
  "scene" : 0,
  "scenes" : [ {
    "nodes" : [ 0 ]
  } ]
}

The validator reports one error:

                "code": "MESH_PRIMITIVE_INVALID_ATTRIBUTE",
                "message": "Invalid attribute name.",
                "severity": 0,
                "pointer": "/meshes/0/primitives/0/attributes/FEATURE_ID_0"

Adding the _ underscore prefix to the attribute name fixes this.

@donmccurdy
Copy link
Contributor

In my opinion a semantic formally defined by an extension should not be considered an application-specific semantic — if the validator gets support for KHR_mesh_features the warning above will disappear.

@javagl
Copy link
Contributor Author

javagl commented Jan 7, 2022

It's not a warning, but an error. I think a warning could be ignored for now, but an error might, in the worst case, cause viewers to just bail out saying "I'm not even gonna try and render an invalid asset" (for a good reason - who knows which crashes that might cause...)

Do you know, from the tip of your head, any extensions that already define attribute names without the underscore? (Otherwise, I might have a look at the existing extensions, and also on how the validator handles this - i.e. whether the validator actually allows for this case if the extension is known by the validator).

Beyond that, it may boil down to the exact meaning of "application-specific", but from a purely technical viewpoint, defining something that causes a validation error does not seem to be a good idea...

@donmccurdy
Copy link
Contributor

Historically we've used the phrase "application-specific" to separate the purpose of Extras from the purpose of Extensions – I think that also applies here. After all, why require userland attributes to have a _* prefix, if no new un-prefixed attributes can be added?

The two examples I can think of are EXT_mesh_gpu_instancing (which is somewhat different, since it has an instancing-specific attributes array) and the proposal in KhronosGroup/glTF#1235, which relies on _* vertex attributes, but allows the user to choose arbitrary names after the underscore.

Neither is the same as our case, so perhaps this is a question for Khronos.

@javagl
Copy link
Contributor Author

javagl commented Jan 8, 2022

After all, why require userland attributes to have a _* prefix, if no new un-prefixed attributes can be added?

The seemingly obvious (but admittedly, rather shallow) answer could be that (originally) there should have been the option to add attributes to the core spec, without the risk of name clashes with existing extension attributes.

More abstractly: The _ could be considered as a "namespace", meaning "this is not part of the core spec". But even more abstractly, one could raise the question of what happens when two extensions both define attributes with ambiguous names like ID or VALUE or so. In order to really prevent clashes there, a more elaborate concept of "namespaces" might be required.

I'm a bit on the fence here. On the one hand, I'd say "Let's just add that _, it does not harm, and makes our assets pass validation". On the other hand, I think that the question about namespacing and disambiguation of attribute names for extensions is important - so I just opened KhronosGroup/glTF#2111 asking about this in general, and the necessity for the undercore for extensions in particular.

@lilleyse
Copy link
Contributor

We're now using underscore-prefixed attributes in the latest version of EXT_mesh_features: CesiumGS/glTF#51

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

No branches or pull requests

3 participants