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

Fix glTF property reference details #88

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Oct 10, 2023

Fixes KhronosGroup/glTF#2165

Adding the minProperties information was trivial: If it is there, just add the bullet point:

Khronos Wetzel minProperties

(It also handles maxProperties. This is not used in the glTF schema, but not handling it would feel wrong...)

--

Fixes KhronosGroup/glTF#2158

Properly handling that may be a bit more tricky, depending on the exact structure of the input schema, and the (too) important role that the title plays for the current state of wetzel. The corresponding line in the code contained a
// TODO: additionalProperties is really a full schema
and I'm not sure what to do with that one.
I had investigated some options back when this issue first came up, and suggested one possible (!) solution there, which is now implemented here, and which does achieve the desired result:

Khronos Wetzel extension properties

Whether it always yields the desired result for all possible inputs....? I don't know. (Does it matter? Probably not...). However, someone might want to review this.


An aside: The minProperties change did not require an update of the 'golden' output, so there seems to be a lack of test coverage for that. The ... "pragmatic" point of view here is that the linked issues should be fixed in the context of glTF, and this goal should be achieved.

I locally reviewed the outputs that are generated for glTF (i.e. the JsonSchemaReference.adoc and PropertiesReference.adoc), and the looked correct for me. Having another look at these files (and a check whether all this works when it is run from within the glTF spec build process) could be part of a review.

EDIT: @emackey I think that you are the main maintainer of wetzel, maybe you want to have a look ....

@@ -101,7 +101,7 @@ Dictionary object with extension-specific objects.

* **Type**: <<reference-extension,`extension`>>
* **Required**: No
* **Type of each property**: Extension
* **Type of each property**: `object`
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want this change? The glTF extensions aren't just simple JSON objects, they do follow the Extension schema, which is what enables nesting. (For example, the "clearcoat" extension may have a "texture transform" extension nested inside of it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it explicitly allows extras inside of extensions.

Copy link
Contributor Author

@javagl javagl Oct 11, 2023

Choose a reason for hiding this comment

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

Maybe I got confused here at some point, or am overlooking something.

An extension object is defined as a plain JSON object, as of the extension.schema.json. It is only an object, without additional constraints for its properties. Its additionalProperties are defined to be object, too.

From what you said about 'nesting', it sounds like the values in the extension dictionary should be constrained to be ~"something that can (optionally) contain another 'extensions' property" (and maybe an optional 'extras' property). So the values could be required to be a glTFProperty, but this 1. does not seem to be the case, and 2. (from the tip of my head) would not change anything in the meaning, compared to just using object - or does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

You might be right. I've always been under the impression that the extensions dictionary object contains a dictionary of named Extension type objects, but it's been a while since I investigated this and I'm not sure.

Maybe @lexaknyazev can double check? This is a far-reaching change because every type of object in a glTF has an extensions dictionary on it, and I feel like the type of entry in that dictionary has changed here. At the very least, it will produce a lot of diffs across the entire section 5 "Properties Reference."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the nitpicking level, some confusion might be caused by the fact that the type of the extensions property is defined in the extension.schema.json. The schema actually does define the structure of the extensions property (and we might consider renaming that file, if this doesn't cause too much trouble - at least, it would match the name of the extras (not extra) schema file...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants