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

Use draft/2020-12 schemas #2031

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Conversation

lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Sep 22, 2021

The new JSON schemas are used only in the Appendix A for now. Changes:

  • Switched draft-04 to draft/2020-12
  • Added $id
  • "enum": [ <value> ] -> "const": <value>
  • URIs use iri-reference format.
  • minimum: number + exclusiveMinimum: bool -> exclusiveMinimum: number

Updated the description of camera.perspective.yfov (in both places), see #1806.

@lexaknyazev lexaknyazev merged commit e2bd348 into KhronosGroup:jon-adoc Sep 23, 2021
@lexaknyazev lexaknyazev deleted the schemas branch September 23, 2021 07:49
@javagl
Copy link
Contributor

javagl commented Sep 23, 2021

Sorry for not reviewing and commenting quickly enough on some of the recent PRs. It's sometimes hard to allocate the time for a "proper" review on a short term notice, although I know that the pace is currently very high...

It's hard to stay up to date with the draft versions of the JSON schema (I hope that they manage to settle for a "JSON Schema 1.0.0-final" at some point...). But from looking at http://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values , I think that enum should still be valid, and I wonder why the change of "enum": [ <value> ] -> "const": <value> was done.

The way how enums are specified in the current glTF schema underwent some iterations (also because of the JSON Schema Draft changes). It's hard to make this consistent for arbitrary types of enums. And by "types", I mean the difference between integer and string, but also whether there is a "closed" set of valid values (like perspective/orthographic for camera), or an "open" set of valid values (like the image MIME type).

(Is there even a sensible way to explicitly differentiate between the two? I think the semantics of anyOf also changed between the draft versions, and our current use is rather a "workaround" for some of the aforementioned difficulties of modeling enums cleanly...).


The change of

  • minimum: number + exclusiveMinimum: bool -> exclusiveMinimum: number

is also a bit surprising. I think that the original structure using a number and a bool was a nice way of modeling the different aspects independently:

  • The minimum value
  • Whether the minimum value is exclusive or not

When there is only a minimum or a exclusiveMinimum, then this has some implications that I consider a bit undesirable:

  • It changes the types. This will affect downstream pipelines (code generators), likely causing all sorts of incompatibilities...
  • It raises the question of how to handle the case that minimum AND exclusiveMinimum are given...
  • From quickly (!) looking over the diff, there seems to be a small inconsistency, with using exclusiveMinimum as a number here and (still) using exclusiveMinimum as a bool here

Of course, the latter could trivially be fixed. But I'd be in favor to not change the type of this field, but to keep the separate number+bool fields instead. I might be convinced otherwise, but ... only with some effort, because I think that there are relatively strong reasons to not change this, unless it is strictly necessary...

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Sep 23, 2021

  • Our anyOf workaround was implemented to add descriptions to integer enum values. const is basically a syntactic sugar for enum with only one element.

  • The exclusiveMinimum update, which happened in draft-06, is described as:

    changed from a boolean to a number to be consistent with the principle of keyword independence

    Of course, both properties cannot be present at the same time anymore.

  • There is no inconsistency between the mentioned links as they point to different schema versions (old draft-04 files are still in place).

@javagl
Copy link
Contributor

javagl commented Sep 23, 2021

As usual, when I'm "surprised" by something that you do, it's an error or lack of understanding on my side. Thanks for the clarification 👍

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.

2 participants