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.1.4 #50

Merged
merged 6 commits into from
Nov 22, 2017
Merged

2.0.0-dev.1.4 #50

merged 6 commits into from
Nov 22, 2017

Conversation

lexaknyazev
Copy link
Member

  • Refined pointers for DUPLICATE_ELEMENTS, UNSUPPORTED_EXTENSION, and UNUSED_EXTENSION_REQUIRED.
  • Add new validation check UNRESERVED_EXTENSION_PREFIX.
  • Small tests cleanup.

@lexaknyazev lexaknyazev added this to the 2.0 milestone Nov 19, 2017
@lexaknyazev lexaknyazev requested a review from emackey November 19, 2017 16:18
@lexaknyazev lexaknyazev changed the title Check unknown extension prefixes, refine pointers, cleanup tests [WIP] 2.0.0-dev.1.4 Nov 20, 2017
@lexaknyazev
Copy link
Member Author

@lexaknyazev
Copy link
Member Author

@donmccurdy This PR should fix #49.

@emackey
Copy link
Member

emackey commented Nov 21, 2017

Sorry for the delay @lexaknyazev... Short week this week, a little swamped here. Will get to this soon as I can.

@donmccurdy
Copy link
Contributor

Alpha cutoff and Firefox/Edge fixes look good to me thanks!

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.

Overall looks great! I asked a few questions, but, feel free to merge this as-is.

ISSUES.md Outdated
|28|UNUSED_EXTENSION_REQUIRED|Unused extension '`%1`' cannot be required.|Error|
|29|UNRESERVED_EXTENSION_PREFIX|Extension uses unreserved extension prefix '`%1`'.|Warning|
|30|NODE_EMPTY|Empty node encountered.|Information|
|31|NON_RELATIVE_URI|Non-relative URI found: `%1`.|Warning|
Copy link
Member

Choose a reason for hiding this comment

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

Do these error numbers do any good for users, given they can change between versions? For example NON_RELATIVE_URI changed from 29 to 31 here. If these aren't stable, any reason not to get rid of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've used them back in a day to keep track on the total amount of recognized issues. They serve no purpose now, so let's remove them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, should we sort messages by code? Current order is semi-random.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorting sounds great.


// https://github.com/KhronosGroup/glTF/blob/master/extensions/Prefixes.md
const List<String> kReservedPrefixes = const <String>[
'KHR_',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we'll need a reminder to update this list from time to time.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you suggest? Adding a comment here or adding a note to Prefixes.md, or both?

Copy link
Member

Choose a reason for hiding this comment

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

We've already got the link here in a comment. I think Prefixes.md should get a note (after this is merged) that any changes there should be copied here (or bump the glTF-Validator maintainers to do it).

<div id="outputZone">
<p><em>Validation is performed locally in your browser. Submitted assets are not uploaded.</em></p>
Copy link
Member

Choose a reason for hiding this comment

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

👍

@lexaknyazev lexaknyazev changed the title [WIP] 2.0.0-dev.1.4 2.0.0-dev.1.4 Nov 22, 2017
@lexaknyazev lexaknyazev merged commit 7c62144 into master Nov 22, 2017
@lexaknyazev lexaknyazev deleted the tweaks branch November 22, 2017 15:20
@emackey
Copy link
Member

emackey commented Nov 22, 2017

Published 2.0.0-dev.1.4 to npm.

@donmccurdy
Copy link
Contributor

https://gltf-viewer.donmccurdy.com/ validation working in Firefox now, thanks!

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.

3 participants