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

Version 2.0.0-dev.3.3 #148

Merged
merged 11 commits into from Dec 2, 2020
Merged

Version 2.0.0-dev.3.3 #148

merged 11 commits into from Dec 2, 2020

Conversation

lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Sep 25, 2020

2.0.0-dev.3.3

New Features

  • Added support for new PBR extensions: KHR_materials_clearcoat, KHR_materials_transmission, and KHR_materials_sheen.

  • Added CAMERA_YFOV_GEQUAL_PI validation warning (Camera perspective yfov has no upper limit. #140).

  • Added new SKIN_IBM_ACCESSOR_WITH_BYTESTRIDE error for IBM accessors using buffer views with defined byteStride.

  • Added new IMAGE_BUFFER_VIEW_WITH_BYTESTRIDE error for images using buffer views with defined byteStride.

  • Added new vendor prefixes: ANIMECH, GRIFFEL, MAXAR, MPEG, PANDA3D, PTC, SEIN, SPECTRUM, TRYON, and VRMC.

Changes

Bugfixes

  • Fixed an endless loop on assets with a node loop within a scene (Validator gets stuck in endless loop #141).

  • Fixed building all targets at once (pub run grinder with no args).

  • Fixed missing JS API error messages when the validator is compiled in debug mode.

  • Web drag-n-drop tool now shows a correct message when no glTF assets were provided.

Integration updates

@lexaknyazev lexaknyazev added this to the 2.0 milestone Sep 25, 2020
@donmccurdy
Copy link
Contributor

Unfortunately it'll be 1-2 weeks before I'm able to review this — if others have time to review in the meantime, please feel free to merge this without me.

Should we fail in a case when an npm version is called without options.externalResourceFunction but the asset uses external files and their validation was requested with options.validateAccessorsData: true?

Because validateAccessorsData defaults to true and externalResourceFunction defaults to undefined, it seems unfortunate that validation would always fail with its default options... Maybe providing externalResourceFunction should simply cause accessor data to be validated, and the validateAccessorsData option can be removed?

@emackey
Copy link
Member

emackey commented Sep 28, 2020

Maybe providing externalResourceFunction should simply cause accessor data to be validated, and the validateAccessorsData option can be removed?

I like this idea. Basically if the external data can be validated (either because it's GLB or because a function was supplied), then there's no reason to skip validation of it.

If such validation doesn't happen, perhaps there could even be a warning, that the model wasn't completely submitted to the validator?

@lexaknyazev
Copy link
Member Author

then there's no reason to skip validation of it

I think there were some reasons initially. The main was the assumption that binary data validation could be too slow but it looks like even with all the extra checks added in the previous update it's still fast enough.

If such validation doesn't happen, perhaps there could even be a warning, that the model wasn't completely submitted to the validator?

How should such warning be reported? Since it doesn't apply to the asset itself, we'd need to adjust the report schema to have an extra flag, like it has truncated for reports with too many messages. Web version would have an extra visual warning that not all external resources were submitted (as some users drag-n-drop only the JSON file).

@emackey
Copy link
Member

emackey commented Sep 29, 2020

How should such warning be reported?

I'm flexible on this. We do have some warnings that are not asset-related, primarily I'm thinking of the "extension not supported by the validator" warning. But yes, this could be a case for a new "truncated" type of warning.

Basically my thinking is, if a user has gone to the trouble to run the validator, they should be warned if it wasn't a completely successful validation of all parts of the glTF, including the external resources. I'm open to ideas though.

@lexaknyazev
Copy link
Member Author

Fair point. We could attribute the lack of externalResourceFunction to resources that actually need it rather than to the report in general.

@emackey
Copy link
Member

emackey commented Sep 29, 2020

Ah, we could apply this warning using JSON pointers for each of the external resource(s) affected, that sounds good to me.

@emackey
Copy link
Member

emackey commented Sep 30, 2020

@donmccurdy Are you OK with where this conversation ended?

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.

Small suggestion here, but this all looks great. I tested some of the new warnings and messages in VSCode.

ISSUES.md Outdated Show resolved Hide resolved
@lexaknyazev
Copy link
Member Author

@emackey
Is sheen extension schema ready to be added here?

@emackey
Copy link
Member

emackey commented Oct 8, 2020 via email

@lexaknyazev
Copy link
Member Author

Added KHR_materials_sheen support.

@donmccurdy
Copy link
Contributor

Are you OK with where this conversation ended?

A warning on external resources that couldn't be validated sounds good to me, yes. 👍

@emackey
Copy link
Member

emackey commented Nov 17, 2020

Let's add the UX3D extension prefix here and call this one done. ✅

@lexaknyazev
Copy link
Member Author

@emackey
Let me update the SDK constraints to keep CI green since we're not fully compatible with 2.12 yet.

@emackey
Copy link
Member

emackey commented Dec 1, 2020

Anything I can do to help this along? Do we just need a tighter range on the SDK listed in pubspec.yaml?

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.

Travis has been running for almost 2 hours, is it stuck?

.travis.yml Show resolved Hide resolved
web/scripts/validator.dart Show resolved Hide resolved
@lexaknyazev lexaknyazev merged commit 0768951 into master Dec 2, 2020
@lexaknyazev lexaknyazev deleted the dev branch December 2, 2020 19:40
@emackey
Copy link
Member

emackey commented Dec 2, 2020

Published to npm.

@lexaknyazev
Copy link
Member Author

Drag-n-drop tool updated.

@hypnosnhendricks
Copy link

How could I have tracked the development of SKIN_IBM_ACCESSOR_WITH_BYTESTRIDE from when it was a concept to implementation to release? Our GLB assets are currently running into this newly added error & I would like to be able to track progress of any new errors being added to the validation tool before they make it into release. My github skills are basic at best. Thanks!

@lexaknyazev
Copy link
Member Author

@hypnosnhendricks
The easiest way would be to subscribe to the repo updates as described in the GitHub docs.

As for that particular validation message, its omission before this release was a bug because the spec defines byteStride only for vertex attributes.

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.

None yet

4 participants