-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add note about attribute name disambiguation #2514
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
base: main
Are you sure you want to change the base?
Add note about attribute name disambiguation #2514
Conversation
extensions/README.md
Outdated
|
|
||
| Examples include: | ||
| * **New properties**: `KHR_texture_transform` introduces a set of texture transformation properties, e.g., | ||
| Extensions may add new properties and values, such as attribute semantics or texture mime types. In all Khronos (KHR) extensions, and as best practice for vendor extensions, these feature additions are designed to allow safe fallback consumption in tools that do not recognize an extension in the `extensionsUsed` array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
texture mime types
Should be "media types"
In all Khronos (KHR) extensions, and as best practice for vendor extensions, these feature additions are designed
This sounds not quite right. We cannot say "are designed" (a fact of now that may not hold true in the future) in relation to "best practice for vendor extensions" (merely a suggestion). Please rephrase.
Please also add a short "Extension naming" section before "Extension mechanics" stating that an extension name consists of the uppercase prefix followed by an underscore and the feature name spelled with snake case convention (with a note that some old extensions may not follow this rule for historical reasons); also that section should state that extension names should not have special characters beyond underscore for splitting words, and in particular, they must not contain a colon symbol as it's used for disambiguating extension-defined vertex attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That part of the text was taken from the current state, which also already contains most of the information about the extension naming at https://github.com/KhronosGroup/glTF/blob/65f4ed6b1b78d61991a9f14e25e9e817867d04bc/extensions/README.md#naming
Moving that further to the top probably makes (as well as adding the hint about the : there). And I'll try to come up with a rewording for the original text part...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. The current naming section should certainly be moved higher and btw all links in it are broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather as a note to myself: There are other broken in-document links at https://github.com/KhronosGroup/glTF/blob/65f4ed6b1b78d61991a9f14e25e9e817867d04bc/extensions/README.md#adding-additional-features-to-gltf
I'll probably do a broader cleanup here, and move that information into some dedicated 'ExtensionDevelopment.md' or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "media types" part was updated as part of the larger refactoring, including moving the "naming" section closer to the top of th technical part and mentioning the additional constraints.
The weasel-words that related to fallback behavior have been omitted here. More specific statements are in the last paragraph of https://github.com/javagl/glTF/blob/disambiguate-extension-attribute-names/extensions/ExtensionDevelopment.md#extension-declarations (taken from the current state). Claiming that it is a "best practice" to offer some form of fallback is too broad. (It wouldn't make sense to put compressed and uncompressed data into one file "just to have a fallback"...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't make sense to put compressed and uncompressed data into one file "just to have a fallback"
Even that case may be reasonable in certain deployments. For example, a fast and large on-prem storage used with different apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just anecdotally: The idea here was good, but ... in hindsight I can't recall seeing extension fallback mechanisms used in production applications. There are enough situations that benefit from different versions of the same glTF asset (e.g. scene.mobile.glb vs scene.desktop.glb) and it is simpler to handle fallbacks for texture formats similarly.
Not suggesting we discourage fallbacks now — consistency with existing extensions is important too! — but if a hypothetical major revision of glTF happened someday, I think there would be a fair case for omitting the fallbacks. Particularly with Draco and Meshopt, the fallbacks create some wrinkles in implementation IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is so much variety in the nature of the extension, the resulting possibilities for fallbacks, and the specific ways of handling fallbacks in the application that I'd say that it's always an engineering decision on the side of the extension author and the users. For example: Storing the same geometry as Draco+Uncompressed will (sometimes, but) rarely make sense. Things like the fallback from 'gaussian splats' to 'point clouds' seem to be reasonable (but eventually, clients may choose to not use that fallback).
|
I tried to address some of the points that have been brought up in the review. These are, in some sense, parts of the considerations that we've been pondering with for years now, namely, to describe the extension development process more clearly. Although I recently closed #2225 because ... ... ... eventually, nobody really cares, right? ... ... I now moved some of these points into a dedicated |
|
I took the libertry to mark the pure wording/formatting tweaks as "resolved". Broader points: I don't know some formal details about the ratification process. And for contributors, the role of the About "must -> MUST" and other wording tweaks: Part of the text had been taken from the current state of the extensions README. Others have been taken from the now-closed #2225. Other parts have been reworded or written from scratch. So it may be a bit of a Frankentext. But I wondered whether this text should try to consistently use RFC2119 style (maybe also to set an example for the 'normative language' that is mentioned as one requirement for the actual specification text). If this is intended, I can do another pass. |
That's the issue. Since this document is not truly normative (yet), there's no strict need to adhere to BCP 14. That said, having two different spellings of normative terms in the same document (if not the same sentence) is quite confusing. |
|
The EXT_primitive_voxels extension proposal relies on application-specific attribute semantics (e.g. In that case, a @javagl, @lexaknyazev, what do you think? Is this idea overkill or is it something we should consider? |
|
The original issue brought up the seemingly philosophical question of what constitutes "application-specific" (which was the term used for the ones to be prefixed with What is currently suggested in this PR completely ignores that question. It only applies to extension-specific attributes, so to speak. This is crucial for being able to use two extensions in a single asset without risking name clashes, and I think that it is resolved sensibly with the prefixing here. Whether there should anything be done for truly application-specific attributes has not yet been discussed. The term sounds like something that is rather "short-lived" or ~"only used internally", in some specific content processing workflow. But eventually, all this should end up in actual assets, and these assets should be as portable as possible. The example of the High-level thoughts (I haven't thought through all cases of the given extensions, and even less possible corner cases) :
|
|
I think the case you mention is plausible and we should provide some guidance here for authors. I'm actually a fan of namespacing both with their own semantic and point them towards the same accessor index as demonstrated in your first bullet point. It seems like an elegant solution for preventing namespace clashes from user defined semantics in extensions that allow such behavior. |
|
@javagl Is this PR ready for another review? Are there any open concerns? |
|
@lexaknyazev The points from the first pass should have been addressed. The point from #2514 (comment) could either be integrated here, or tracked in its own issue/PR. My understanding/summary of that open question is: When an extension does not specify certain attribute names, but just allows (application-specific) attributes, should these application-specific attributes also be prefixed with the extension name? I think that this could make sense. Applications can trivially "ignore" these prefixes, but it would still be a way to make clear which extension it is that is expected to be able to do something with these attributes. |
|
Application-specific attributes are already allowed by the base spec; extensions do not affect them at all. I suppose the question is
Given that underscore-prefixed attributes are basically ignored by the base spec, I think the same logic could apply here, i.e., underscore-prefixed attributes may be prefixed with an extension name but they would still be ignored from the spec point of view. |
|
The original issue revolved around the exact meaning of "application-specific". An attempt to describe the remaining question more precisely: When a certain extension allows to define arbitrary vertex attributes (within the constraints of the spec), should these attribute names be prefixed with the name of the extension based on which they have been defined? The specific example of the |
|
I think there are two interpretations here:
The second interpretation effectively reserves the leading underscore as a universal way of adding app-specific attributes to the base spec and to extensions and also implies that attributes used in the |
|
I think that the main reasoning behind the prefixes - namely, disambiguation - is still applicable. The |
|
I think I strongly disagree with the "extension could add a
That said, my preference would be to explicitly exclude attributes with leading underscores (whether prepended by extension names or not) from all specifications so they are always application-specific. This would allow adding custom attributes to all scopes without breaking anything. An extension like |
|
This may have been a misunderstanding. I've not been talking about a
That's exactly what I wanted to make clear. When an extension allows defining new attributes, then the names still have to be prefixed with that extension name. (Whether there's some I'll try to come up with some wording for that - maybe as a new sub-point/section near the "New Attributes" section, saying roughly : ~"When an extension allows defining new attributes, then the names of the attributes that are defined in the scope of this extension have to be prefixed". The latter affects affects at least The more general point of the disambiguation affects |
Sounds good. I'd suggest adding another note about underscores, specifically saying that an underscore-prefixed attribute inside the extension scope, i.e., following the |
|
I tried to add the respective clarifications at https://github.com/javagl/glTF/blob/disambiguate-extension-attribute-names/extensions/ExtensionDevelopment.md#new-attributes |
|
If there is something "fundamentally wrong" in the current state, then it could be added as a comment here. |
Fixes #2111 :
As discussed in the issue and during the 3D Formats calls, the disambiguation between attribute semantic names that are defined by different extensions should happen by prefixing the attribute name with the full name of the extension, followed by a
:colon.This PR adds the corresponding information to extension development process
Updated:
Based on the review feedback, and considering the pending task to describe the extension development process more clearly in general, I moved the part that describes the extension development into a dedicated document. Here's a direct link to new
ExtensionDevelopment.md. (The part that this PR originally aimed at is the "New attributes" section).The main
extensions/README.mdnow only serves as an "entry point": It contains the "Extensions Registry" (i.e. the list of extensions and PRs), and links to the development description in the first paragraph.