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

Replace 'gltf_enumNames' with 'oneOf', and other schema cleanup #891

Closed
emackey opened this issue Mar 22, 2017 · 7 comments
Closed

Replace 'gltf_enumNames' with 'oneOf', and other schema cleanup #891

emackey opened this issue Mar 22, 2017 · 7 comments

Comments

@emackey
Copy link
Member

emackey commented Mar 22, 2017

I encountered a couple issues with the structure of the glTF schema while attempting to import it into my gltf-vscode extension. I'd like to propose some structural changes (at least to 2.0, although they would be completely non-breaking if applied retroactively to 1.0 as well).

First, and primarily, the use of gltf_enumNames is non-standard and not compatible with JSON validators such as the one in vscode. For example, texture.schema.json in both 1.0 and 2.0 (draft) contains the following lines:

        "format": {
            "type" : "integer",
            "description" : "The texture's format.",
            "enum" : [6406, 6407, 6408, 6409, 6410],
            "gltf_enumNames" : ["ALPHA", "RGB", "RGBA", "LUMINANCE", "LUMINANCE_ALPHA"],
            "default" : 6408,
            "gltf_detailedDescription" : "The texture's format.  Valid values correspond to WebGL enums: `6406` (ALPHA), `6407` (RGB), `6408` (RGBA), `6409` (LUMINANCE), and `6410` (LUMINANCE_ALPHA).",
            "gltf_webgl" : "`texImage2D()` format parameter"
        },

The JSON validator has two problems with this: (1) It can't understand the descriptions given for the individual enum names, and (2) the use of "type" : "integer" causes it to suggest 0 when no default value is supplied (as opposed to suggesting the first enum value). (Well I picked a bad example here since there is a default value for this one, but there are other places like technique parameters where no default is supplied).

I ended up writing a node.js schema transformation script, that takes the glTF schema and transforms its enums to look similar to this:

        "format" : {
            "description" : "The texture's format.",
            "default" : 6408,
            "gltf_detailedDescription" : "The texture's format.  Valid values correspond to WebGL enums: `6406` (ALPHA), `6407` (RGB), `6408` (RGBA), `6409` (LUMINANCE), and `6410` (LUMINANCE_ALPHA).",
            "gltf_webgl" : "`texImage2D()` format parameter",
            "oneOf" : [
                {
                    "enum" : [6406],
                    "description" : "ALPHA"
                },
                {
                    "enum" : [6407],
                    "description" : "RGB"
                },
                {
                    "enum" : [6408],
                    "description" : "RGBA"
                },
                {
                    "enum" : [6409],
                    "description" : "LUMINANCE"
                },
                {
                    "enum" : [6410],
                    "description" : "LUMINANCE_ALPHA"
                }
            ]
        },

The old enums have been converted into a oneOf array with individual descriptions. This isn't specific to VSCode, it's official (at least for JSON schema v4) and other schemas are doing this outside of vscode use cases.

The field type has been removed, to prevent zero being suggested as a default value when the schema does not offer a default. And in this particular case, gltf_detailedDescription could also be removed as it appears redundant now. The oneOf construct provides per-enum-value descriptions that can be understood by validators that don't otherwise speak glTF.

statesdemo1

If people want this change, I can open a PR. As mentioned I have a script that can make this change for all enums at once. But, it reformats probably better than half of the schema files, and would likely cause merge conflicts on other open PRs. We could also wait until more outstanding PRs are merged, and then make the change.

@emackey
Copy link
Member Author

emackey commented Mar 22, 2017

I have no idea what implications this may have for CesiumGS/wetzel#4 @HowardWolosky

@HowardWolosky
Copy link

Thanks for the heads-up. I'd be in favor of the change. Anything to further standardize on existing official json-schema paradigms, the better, especially if it means we get the benefit of tools that understand generic json schemas (like your code plugin). There would be some impact to wetzel, but not terribly significant (less than a day's work), and it would mean that the resulting documentation would likely read a bit cleaner too. I won't start on the wetzel change though until I hear broader consensus that this will go in.

@pjcozzi
Copy link
Member

pjcozzi commented Mar 22, 2017

I also agree with this change. I only did this in JSON schema 3 because I don't think there were other options.

Does anyone want to make this change an open a PR into the 2.0 branch?

@sbtron
Copy link
Contributor

sbtron commented Mar 27, 2017

@robertlong since you have a typescript generator based on schema, would be good to get your input on whether you will be able to update the generator to support the oneOf approach for enums proposed above?

@robertlong
Copy link
Contributor

@sbtron My script is pretty small. I depend on json-schema-to-typescript to output the schema.

The author implemented named enums with an extension to the JSON Schema spec like you currently have.

In the README the author states that oneOf is not expressible in TypeScript and I tend to agree.

I'd vote against using oneOf for named enums.

@sbtron
Copy link
Contributor

sbtron commented Apr 4, 2017

Thanks. Looks like anyOf should be ok according to the readme. We are planning on using anyOf.

@emackey
Copy link
Member Author

emackey commented Apr 5, 2017

I just tested anyOf in gltf-vscode, it seems to work fine.

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

No branches or pull requests

6 participants