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

Update normative language #1997

Merged
merged 7 commits into from
Jul 20, 2021
Merged

Conversation

lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Jun 28, 2021

General updates

  • Use BCP 14 normative language.
  • Properly list all external references.
  • Expand the Samplers section.
  • Expand material texture descriptions.
  • Reduce emphasis on OpenGL.
  • Minor language edits.

Specific updates

  • Restrict certain PNG and JPEG variations.
  • Full rewrite of JSON encoding section, relaxing some of previously-unenforced restrictions.
  • Explicitly allow IRIs.
  • Explicitly disallow bufferView.byteStride for buffer views that do not contain vertex attributes.
  • Elaborate on image media types handling.
  • Restore accidentally removed vertex color usage.
  • Discourage use of zero-length GLB BIN chunks.

@javagl
Copy link
Contributor

javagl commented Jun 28, 2021

I started a review of the changes, but it's pretty large. I locally took some notes (a bit of nitpicking, and some things that I wondered about, but where, at some point, I have to assume that they have been sorted out in certain issues, and the changes here are just the result of a larger discussion). Two things for the process:

@lexaknyazev
Copy link
Member Author

Since inline comments naturally support threading, I'd prefer to use them since the diff is indeed large (although most of it is trivial). Feel free to ask about any confusing changes.

@oddhack
Copy link
Contributor

oddhack commented Jun 29, 2021

I started running through this and soon noticed a lot of omission of definite & indefinite articles (such as "the keyword", "a buffer", "an object") where they're grammatically appropriate. On the grand scale of things this is more jarring to read than a spec bug, and I didn't try to enumerate them past the first few examples. But it is still worth addressing.

@lexaknyazev
Copy link
Member Author

I started running through this and soon noticed a lot of omission of definite & indefinite articles (such as "the keyword", "a buffer", "an object") where they're grammatically appropriate.

You're right. This is a long-standing issue since the very first glTF drafts.

@javagl
Copy link
Contributor

javagl commented Jun 29, 2021

The missing articles are something that I also noticed, but saw it in the same way as oddhack: It's usually not relevant for the correctness or preciseness of the spec itself.

But to be concrete: Should/could this be addressed in this review? These could be many small comments, and it may cause some noise that distracts from possibly more important aspects that are actually supposed to be reviewed here.

Another option would be to really focus on the normative/spec aspect here, and do a dedicated pass with editorial changes later. The corresponding PR could probably be reviewed quickly and easily, because it would just insert some "a"s and "the"s here and there, without any other changes.

(This is somehow related to the question about the timeline. I think it was mentioned in the calls (or maybe the mail thread?), but: When is the the deadline for this update?)

@neiltrevett
Copy link
Collaborator

neiltrevett commented Jun 30, 2021

When is the the deadline for this update?

We have two deadlines:

  1. Any IP-impacting changes must be implemented and agreed by the meeting next week (July 7th) so we can vote to submit the specification for ratification. No IP-impacting changes would then be permitted without restarting the ratification review
  2. Any non-IP-impacting changes (e.g. formatting, clarifications, informative additions, adding missing articles that don't change meaning) can be made at any time during the 6-week ratification review and beyond, up to when we formally release the specification, or submit it to JTC 1, which ever is the sooner - latest early September.

Neil

Copy link
Contributor

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Reviewed as far as the beginning of the Geometry section – will try to review further soon!

specification/2.0/Specification.adoc Outdated Show resolved Hide resolved

The following documents are referenced by normative sections of the specification:

==== External Specifications
Copy link
Contributor

Choose a reason for hiding this comment

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

In rendered form it's difficult to find something quickly among these references, because the keywords are somewhere in the middle of the line. Would it be appropriate to put the title first, rather than the source, or prefix with a keyword like "JSON"?

specification/2.0/Specification.adoc Show resolved Hide resolved
Implementations should use the image type pattern matching algorithm from the https://mimesniff.spec.whatwg.org/#matching-an-image-type-pattern[MIME Sniffing Standard] to detect PNG and JPEG images as file extensions may be unavailable in some contexts.
====
[[file-extensions-and-media-types]]
== File Extensions and Media Types
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why the use of a .gltf extension for JSON glTF files is not required — we don't want to prohibit use cases like REST APIs, or perhaps like .vrm.

But, is it worth including some language that normatively prohibits a JSON glTF file using a .glb prefix, or a binary GLB file using .gltf? Such mixups would be unfortunate.

specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
@@ -572,20 +735,26 @@ The following example defines two buffer views: the first is an ELEMENT_ARRAY_BU
}
----

When a buffer view is used for vertex attribute data, it may have a `byteStride` property. This property defines the stride in bytes between each vertex.
When a buffer view is used for vertex attribute data, it **MAY** have a `byteStride` property. This property defines the stride in bytes between each vertex. Buffer views with other types of data **MUST NOT** not define `byteStride`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be valid for an extension to define a new "kind" of data that supports interleaving? Trying to accommodate EXT_mesh_gpu_instancing instance attributes and perhaps future extensions here.

@javagl
Copy link
Contributor

javagl commented Jun 30, 2021

I went through the remaining document. (Sorry, some comments are "nitpicking" about formatting, but I hope that they are not too distracting).

One (inlined) point that might relevant for the "normativeness" is: In how far can the Filtering behavior be prescribed (e.g. the necessity to be able to generate MipMaps)?

Other, general points:

  • Sometimes, the wording is "instanced", and sometimes "instantiated". Do we make a difference here?
  • The connection between the component types (and the "normalized" flag) and the corresponding values (like 5126 for float) is not really clear, now that the references to the constants like GL_FLOAT have been omitted...

This may just be a lack of understanding on my side:

  • I never deeply dug through the maths of PBR, but... when talking about "metal" and "non-metal" here with the "metal" value being in [0,1], then I wonder: Is 0.0001 already "metal", or is 0.9999 still a "non-metal"? (Maybe this becomes clear in the appendix, but I didn't know what to do with this information, at this point, from a specification perspective...)
  • It's nice that the sRGB computations are illustrated with an example here, but ... someone who's reading that will most likely have NO idea where these numbers came from. There's a link to some IEC spec, for ~175 bucks, so that may be safe from a "normative-ness" standpoint. But someone who tries to understand and implement that may have to do some websearches (and eventually stumble over http://davengrace.com/cgi-bin/cspace.pl ...)

Formatting/linking issues:

Copy link
Contributor

@javagl javagl left a comment

Choose a reason for hiding this comment

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

To summarize: I did not find any obvious issues. Some details are discussed in the inlined and PR comments.

specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Show resolved Hide resolved
specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Show resolved Hide resolved
specification/2.0/Specification.adoc Show resolved Hide resolved
specification/2.0/Specification.adoc Show resolved Hide resolved
specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@javagl javagl left a comment

Choose a reason for hiding this comment

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

I did another quick pass, focussing on the changes, and think that they indeed add clarity. I cannot say much about the fallback behavior for the MipMapping on a technical level, but reducing the requirement from MUST to SHOULD IMHO makes sense, because it's usually beyond the control of the client.

No further comments for now, except for a detail regarding the ROWS * SIZE_OF_COMPONENT part (inlined).

specification/2.0/Specification.adoc Show resolved Hide resolved
@lexaknyazev lexaknyazev marked this pull request as ready for review July 8, 2021 16:52
@lexaknyazev lexaknyazev merged commit 94ed681 into KhronosGroup:jon-adoc Jul 20, 2021
@lexaknyazev lexaknyazev deleted the adoc-update branch July 20, 2021 13:16
@lexaknyazev
Copy link
Member Author

Let's continue the refinements in #1901.

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.

6 participants