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

Add full glTF Validator integration #27

Closed
emackey opened this issue Aug 8, 2017 · 9 comments
Closed

Add full glTF Validator integration #27

emackey opened this issue Aug 8, 2017 · 9 comments

Comments

@emackey
Copy link
Member

emackey commented Aug 8, 2017

I talked briefly with @lexaknyazev about this at the glTF roadmap meeting after SIGGRAPH 2017. Basically, this extension is using VSCode's built-in JSON validator against the glTF schema, but there are lots (dozens, hundreds?) of types of glTF errors (such as binary range out-of-bounds, etc) that can't be caught by simple JSON schema validation alone. To catch these, we should instead run the official glTF Validator from Khronos.

The official validator is written in Dart, and I believe has already been compiled to JS. @lexaknyazev mentioned that an npm package was going to be released soon, and that we could include that as a dependency.

Letting go of VSCode's JSON validation and switching to a custom validator will have some complexity on the VSCode integration side as well. I've looked into it, but it's not immediately clear to me the best path forward. @balefrost expressed some interest, but doesn't have any time to work on this last I checked.

Contributions here are most welcome!

@emackey
Copy link
Member Author

emackey commented Sep 21, 2017

Someday it would be great to run validation as part of a custom glTF Language Server.

https://code.visualstudio.com/blogs/2016/06/27/common-language-protocol

https://code.visualstudio.com/docs/extensions/example-language-server

@lexaknyazev
Copy link

@emackey
First-glance thoughts on steps needed to implement this:

@emackey
Copy link
Member Author

emackey commented Sep 22, 2017

On that first point, this extension already depends on an npm package called json-source-map, and we use it to get a JSON pointer to a data-URI that the user has clicked on in the text editor.

Their API Docs specifically mention RFC 6901.

@emackey
Copy link
Member Author

emackey commented Sep 22, 2017

Would be nice to break out different diagnostic severities, as discussed in KhronosGroup/glTF-Validator#21 (comment)

@emackey
Copy link
Member Author

emackey commented Sep 26, 2017

I got a first cut of this working last night on the validator-preview branch. I'm still treating glTF as json language, but I've added my own language server in addition to the JSON one, and I now have actual output from the validator flowing into the document diagnostics mechanism.

@lexaknyazev
Copy link

Since both VS Code glTF Server and glTF Validator JS API are in pre-release stage, let's establish best practices for their integration.

Here're my first comments on server.ts as of 075d660.

  • Consider using validateString to avoid unnecessary conversions. validateBytes accepts bytes to be able to detect and validate GLB.
    const gltfData = Buffer.from(textDocument.getText());
    const folderName = path.resolve(fileName, '..');
    gltfValidator.validateBytes(baseName, new Uint8Array(gltfData), (uri) =>
  • Do we need to expose GLB-container-only validation API?
  • Validator can report on broken JSON. If the intention is to avoid running validator on invalid input, this check should be done earlier.
    const map = tryGetJsonMap(textDocument);
    if (!map) {
    diagnostics.push(getDiagnostic({ message: 'Error parsing JSON document.' }, {}));
    } else if (result.issues && result.issues.messages) {
  • I'm not sure how TypeScript enums work, but reported severity integer values exactly match DiagnosticSeverity definitions. Maybe this switch isn't needed.
    let severity: DiagnosticSeverity = DiagnosticSeverity.Error;
    switch (info.severity) {
    case 1:
    severity = DiagnosticSeverity.Warning;
    break;
    case 2:
    severity = DiagnosticSeverity.Information;
    break;
    case 3:
    severity = DiagnosticSeverity.Hint;
    break;
    default:
    break;
    }
  • result is an instance of some internal object, use .toString() to get readable representation.
    }, (result) => {
    // Validator's error
    console.warn('glTF Validator had problems.');
    console.warn(result);
    });

@emackey
Copy link
Member Author

emackey commented Nov 8, 2017

@lexaknyazev Thanks for the pre-PR review! I do have plans to add a GLB validation command, especially now that VSCode itself can produce new GLB files, it's needed. Perhaps newly-produced GLBs should auto-validate. I'll look into the other comments here too.

@emackey
Copy link
Member Author

emackey commented Nov 9, 2017

@lexaknyazev Is there a way I can manually trigger the "Validator had problems" failure path? Would like to test how VSCode handles those failures.

@lexaknyazev
Copy link

Try to feed it with nor glTF, neither GLB data.

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

2 participants