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

2.0.0-dev.2.2 #83

Merged
merged 12 commits into from
Jun 1, 2018
Merged

2.0.0-dev.2.2 #83

merged 12 commits into from
Jun 1, 2018

Conversation

lexaknyazev
Copy link
Member

Other planned features for this PR (please tell if you'd prefer something else):

  • KHR_lights
  • KHR_texture_transform
  • DDS images recognition

/cc @emackey @donmccurdy

@donmccurdy
Copy link
Contributor

Looks good so far!

@lexaknyazev
Copy link
Member Author

Since KHR_lights extension has been delayed, I'd propose to release this.

@lexaknyazev lexaknyazev requested review from emackey and donmccurdy May 27, 2018 16:46
Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

This is looking great. Left some small comments.

@lexaknyazev
Copy link
Member Author

The recent commit adds a new check re: KhronosGroup/glTF#1345 for all byte-based inputs (command-line, web drag-n-drop, validateBytes node API).

@emackey Is it possible to check document's encoding from VS Code to ensure that it's UTF-8 without BOM? As soon as an asset becomes a string there's no way to check that from the validator.

/cc @bghgary

@emackey
Copy link
Member

emackey commented May 31, 2018

Is it possible to check document's encoding from VS Code to ensure that it's UTF-8 without BOM?

It does not appear so. There are UI controls for changing the encoding, including adding or removing the BOM, but those controls don't appear to have an associated API. To make matters worse, the interactive / real-time validation of glTF happens in a "Language Server" which is a separate process from the rest of the extension, and it is passed a copy of the document's contents over the Language Server Protocol, outside of our control. I can't imagine that protocol allowing the BOM to slip through. The API on the language server side is just querying the copy that has been passed over to the separate process.

One piece of good news though, users can already run the validator directly on a file (typically by right-clicking in VSCode's File Explorer). This is an extra step but is the only way to get VSCode to save out the full JSON conformance report, and is the only way to validate a .glb. When this option is used, only the filename is passed from VSCode to the validator, so it is up to the validator to load the raw file, and a BOM would then be detected and flagged in the report.

@lexaknyazev
Copy link
Member Author

lexaknyazev commented May 31, 2018

One piece of good news though, users can already run the validator directly on a file (typically by right-clicking in VSCode's File Explorer).

I can see those commands only in the tab's context menu:
image

But not in the Explorer pane:
image

Is it intended?


Not a very strong opinion, but is it possible to open a full validation report in a new tab before showing "Save" dialog (maybe behind a flag for some time)? Users can always save it in a file afterwards.

Here's current UX with UTF-8 BOM files:

  1. User sees an asset that renders OK and has no issues in "Problems" section.
  2. Just to be sure, user clicks "glTF: Validate ..." command from the right-click menu.
  3. A popup shows "glTF Validator found 1 errors." (Could we use singular form here?)
  4. Clicking on that popup does nothing, user has to click precisely on "Save Report As...".
  5. One more click to save the report file.
  6. The report opens in a new tab.

@lexaknyazev
Copy link
Member Author

There are UI controls for changing the encoding, including adding or removing the BOM, but those controls don't appear to have an associated API.

Could we ask VS Code to at least provide that information (e.g., with getter TextDocument.encoding)? In that case, glTF language server could add a validation message similar to validator's (ideally with a quick fix to re-save the asset without BOM).

/cc @najadojo

@emackey
Copy link
Member

emackey commented May 31, 2018

But not in the Explorer pane

There's a minor known issue where the right-click options only show up in the folder view (you have to select "Open Folder..." and pick the top of the sample model repo or some other tree with glTFs in it). The "Open Editors" list doesn't do it, I'm not sure why not. I don't think we have a GitHub issue on that actually, so I'll write one.

Here's current UX with UTF-8 BOM files

I agree that's bad UX. I'm not sure I agree that we should be nitpicking the user's BOMs. Several editors, source control systems, and other toolchains impose their own rules for BOMs or lack thereof, and I don't quite see the need for .gltf to be bothered by the presence of a stray one.

Could we ask VS Code to at least provide that information

If there are solid reasons to be warning the user about such things, then yes, I think that's the right approach here.

@lexaknyazev
Copy link
Member Author

I'm not sure I agree that we should be nitpicking the user's BOMs.

Please see the beginning of KhronosGroup/glTF#1345.

@bghgary
Copy link

bghgary commented May 31, 2018

I'm not sure I agree that we should be nitpicking the user's BOMs.

Please see the beginning of KhronosGroup/glTF#1345.

FYI, I opened the issue because we found out recently that some of our apps reject glTF files that have a BOM. I am proposing to the app teams that they relax this specific case since none of the implementations that I've seen elsewhere enforce this.

@lexaknyazev
Copy link
Member Author

A quick web search of "JSON BOM" shows lots of friction in different projects. So I'd prefer that we keep the restriction in the spec, especially since even JSON RFC mentions that.

@emackey
Copy link
Member

emackey commented May 31, 2018

So, readers should be tolerant of BOMs, but writers should endeavor not to write BOMs. Just like the FCC's warning stickers (must not generate radio interference... must tolerate any received radio interference). Since one of the main uses of the validator is to check the work of the exporters, I suppose it does make sense for it to say something about the BOM if found.

On the VSCode side, there's no API, and I don't expect it to be a quick add since we would need it to propagate over to the language server. The protocol there only does utf-8 and doesn't appear to preserve information about the source file's encoding or BOM status.

One possibility, and this is a bit of a stretch, but the language server does have access to the filename. So in theory, it could access the actual file directly, to check for a BOM, but do the rest of the validation on the document contents, which may contain unsaved edits. The BOM check could even happen out-of-band with the rest of the validation, as it likely wouldn't need to happen as frequently.

@bghgary
Copy link

bghgary commented May 31, 2018

Agreed, not advocating changing the restriction in the spec. I'll respond to your question in KhronosGroup/glTF#1345.

@lexaknyazev
Copy link
Member Author

@emackey
I just thought that VS Code may provide that metadata as a part of TextDocument interface but since it doesn't let's move on.

I'll integrate your UnusedObjects model into tests and merge this PR afterwards.

@lexaknyazev lexaknyazev changed the title [WIP] 2.0.0-dev.2.2 2.0.0-dev.2.2 Jun 1, 2018
@lexaknyazev lexaknyazev merged commit a2ec886 into master Jun 1, 2018
@lexaknyazev lexaknyazev deleted the 2.0.0-dev.2.2 branch June 1, 2018 11:41
@lexaknyazev
Copy link
Member Author

Online version updated.

@emackey
Copy link
Member

emackey commented Jun 1, 2018

Published to npm.

@donmccurdy
Copy link
Contributor

Having some issues in my browser build:

echo "window.gltfValidator = require('gltf-validator');" | browserify - > lib/gltf-validator.js

Not sure why this fails and the online version is OK? Result:

chrome firefox
screen shot 2018-06-02 at 10 50 18 am screen shot 2018-06-02 at 10 50 27 am

@lexaknyazev
Copy link
Member Author

Not sure why this fails and the online version is OK?

The node package uses a couple tricks to work both in node vm and in browser - sometimes something goes wrong; I'll investigate that asap. The online drag-n-drop version is built end-to-end with Dart toolkit, so it's unaffected.

@lexaknyazev
Copy link
Member Author

lexaknyazev commented Jun 2, 2018

Found the issue. It's a bug in the building process (which has changed in this release).

@lexaknyazev
Copy link
Member Author

@donmccurdy
Do you need the fix immediately? I'd like to make couple more updates before issuing a new release.

@donmccurdy
Copy link
Contributor

There’s no rush 👍

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.

4 participants