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

Unclear messages in corner cases #68

Closed
lexaknyazev opened this issue Nov 30, 2017 · 9 comments
Closed

Unclear messages in corner cases #68

lexaknyazev opened this issue Nov 30, 2017 · 9 comments

Comments

@lexaknyazev
Copy link

Output on totally empty file shouldn't be attributed to glTF Validator:

severity: 'Error'
message: 'Error parsing JSON document.'
at: '1,1'
source: 'glTF Validator'
code: 'undefined'

Output on an asset with version 2.1:

{
    "asset": {
        "version": "2.1"
    }
}
severity: 'Warning'
message: 'Matches multiple schemas when only one must validate.'
at: '1,1'
source: ''
code: 'undefined'
@emackey
Copy link
Member

emackey commented Nov 30, 2017

The second message is "expected" although an unfortunate side-effect of an early design decision. Early on I wanted this project to handle both glTF 1.0 and 2.0 schemas, but, VSCode's JSON validation has but a single contribution point, and there's no way for a VSCode extension at runtime to control which of two different schemas gets applied.

Thus, "the Chooser" was born. Behold and weep:

{
    "$schema" : "http://json-schema.org/draft-04/schema",
    "title" : "glTF",
    "type" : "object",
    "description" : "The root object for a glTF asset.",
    "oneOf" : [
        {
            "$ref" : "gltf-2.0/glTF.schema.json"
        },
        {
            "$ref" : "gltf-1.0/glTF.schema.json"
        }
    ]
}

With this construct in play, a glTF file in VSCode must match enough of one of the glTF schemas that VSCode's built-in JSON validation can figure out which of the two schemas is being used. For "real-world" files, this isn't a problem, because almost every child of root is an object in 1.0 and an array in 2.0. So the moment any real content is added to the file, the choice of schema becomes clear.

Although we've added the official validator, I still need to keep the "dumb" JSON schema validation around for now, because it supplies hover-descriptions, enum auto-completion, etc., and it's the only way to validate glTF 1.0.

Sometimes, if you commit a simple JSON schema violation in VSCode, you'll see duplicate error messages, one marked source: 'glTF Validator' and another marked source: '' from the JSON validator. This is basically unavoidable, and the correct action is to fix the broken glTF.


As for the first message, that one comes directly from server.ts on this line. The "glTF Language Server" built-in to this extension is attempting to parse the JSON document, which it needs to do to gather a map of JSON pointers to document locations. It won't even call the glTF Validator if this parsing fails, and instead just reports its own error, since it knows it wouldn't be able to make heads or tails of anything reported by the validator. This one does get marked source: 'glTF Validator' even though it came from the preparations just prior to launching the validator.

If needed, we could change the source on this to JSON.parse or similar, to keep the validator free of blame for these errors. Is that desirable?

@lexaknyazev
Copy link
Author

Could 1.0 schemas include a simple regex (or even a const value) for asset.version?

So the moment any real content is added to the file, the choice of schema becomes clear.

extensions is an object in both versions, so there could be a real-world example looking like this:

{
    "asset": {
        "version": "2.0"
    },
    "extensionsUsed": [
        "EXT_demo"
    ],
    "extensions": {
        "EXT_demo": {}
    }
}

If needed, we could change the source on this to JSON.parse or similar, to keep the validator free of blame for these errors. Is that desirable?

No strong opinion here. Users don't have to precisely understand internal layers of validation routines.
However I see some value in glTF Validator-attributed output being the same across different runtime environments.

@emackey
Copy link
Member

emackey commented Nov 30, 2017

Could 1.0 schemas include a simple regex (or even a const value) for asset.version?

The 1.0 schema didn't require asset or version to be present at all, unfortunately.

@lexaknyazev
Copy link
Author

lexaknyazev commented Nov 30, 2017

@emackey
Copy link
Member

emackey commented Nov 30, 2017

I tried hand-editing my copy of the glTF 1.0 schema to have a pattern added to version, like so:

        "version" : {
            "type" : "string",
            "pattern": "^1",
            "description" : "The API version.",
            "default" : "1.0.3"
        }

Unfortunately this doesn't seem to work. Your EXT_demo example above still claims to match both 1.0 and 2.0 schemas, even after this edit.

@lexaknyazev
Copy link
Author

That's because our 2.0 regex doesn't care about actual version number. Tweaking both schemas helps:

v1

"version" : {
    "type" : "string",
    "description" : "The glTF version.",
    "pattern" : "^1"
}

v2

"version" : {
    "type" : "string",
    "description" : "The glTF version that this asset targets.",
    "pattern" : "^2\\.[0-9]+$"
},

Your snippet above seems to be from asset.profile.

@emackey
Copy link
Member

emackey commented Nov 30, 2017

Your snippet above seems to be from asset.profile.

Doh. That's the problem.

@emackey
Copy link
Member

emackey commented Nov 30, 2017

Fixing this upstream in KhronosGroup/glTF#1168.

@emackey
Copy link
Member

emackey commented Dec 1, 2017

Published as 2.1.2

@emackey emackey closed this as completed Dec 1, 2017
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

No branches or pull requests

2 participants