Add Khronos glTF sample models to fidelity test suite by cdata · Pull Request #375 · google/model-viewer

cdata · 2019-02-18T22:19:53Z

Khronos glTF sample fidelity tests

This change proposes the addition of several tests to the fidelity suit, all based on Khronos glTF sample models. The highlights of this change include:

27 new fidelity tests, all based on Khronos glTF sample models
- Some tests omitted, due to outright failure (see Failure to render glTFs with animations #371) or negligible testing value
- See below for observations, analysis and remarks related to initial results
Screenshot update automation expanded to include support for updating <model-viewer> "master" screenshots
- Used to test for regressions against our own accepted renderings
- npm run update-screenshots will update Filament and <model-viewer "master" at the same time
Filament-based gltf_renderer updated to use the same model framing strategy as
- Puts us on a good path for keeping staging in line as we evolve <model-viewer> going forward
- See below for additional details
A new command npm run checkout-samples will clone and/or update Khronos samples directly from Github
- Runs automatically during npm install and npm ci

🛂Note for reviewers: these tests bring attention to a noticeable lighting / saturation difference between Filament and <model-viewer> for many of the cases. This is currently expected, and is assumed to be due to default lighting, tonemapping and exposure configurations across both renderers. For relevant background discussion, please refer to #338, where PMREM support was introduced.

Analysis of initial results

16bpc PNG textures

The first model to show a disparity is Antique Camera:

`<model-viewer>`	Filament `gltf_renderer`

I dug into this problem and discovered that Filament was failing to load some of the model's textures because it was using a third-party library that did not support loading 16bpc PNGs. A fix for this issue has already landed in Filament, so thankfully this particular disparity should be gone by the time we update the screenshots again.

Vertex colors

The next stand-out issue is that for an as-yet undiagnosed reason, Filament does not support vertex colors in glTFs yet. This issue is already documented by the Filament project (google/filament#528). Here are the results for the two fidelity tests that check for vertex color support:

`<model-viewer>`	Filament `gltf_renderer`

Texture transforms

The Khronos body includes a sample model that tests support for the KHR_texture_transform extension. Filament shows an incorrect result for scale, transform and rotation (subsequent discussion on Filament project suggests this will be fixed by changing their glTF loader):

`<model-viewer>`	Filament `gltf_renderer`

Multiple UVs

One of the notable gaps in <model-viewer>'s glTF support that is uncovered by these tests is the lack of multiple UV support (#372 filed to track). Filament's render matches the expected result in this case:

`<model-viewer>`	Filament `gltf_renderer`

Normal tangents

The glTF samples include two test cases (1, 2) related to normal tangents. The mirrored case tests whether the tangents supplied by the model are used (as opposed to the tangents being calculated by the renderer). The mirrored case is notably incorrect in Filament (already documented in google/filament#528):

`<model-viewer>`	Filament `gltf_renderer`

Unlit materials

These tests uncovered that <model-viewer> completely fails to do the right thing with unlit materials (#374 filed to track). Thankfully, @jsantell has already put a fix (#377) up for consideration. Here is an example of the current rendering failure (Filament's render result is what we expect):

`<model-viewer>`	Filament `gltf_renderer`

Alpha blending

The Khronos models include a test case for alpha blending. These render results surprised me: <model-viewer> seems to yield an incorrect result in all columns except for the second one (filed #381 to track). Filament renders the expected result:

`<model-viewer>`	Filament `gltf_renderer`

Other inconsistencies

There were a few render results where there were obvious inconsistencies or rendering artifacts, but the cause is less certain.

🛂Reviewers: please consider the following cases and speculate about what might be causing these inconsistencies and whether we can/should take action to address them:

Environment Test

The bottom row seems very different. <model-viewer> seems closer to the result shown in the Khronos source repo:

`<model-viewer>`	Filament `gltf_renderer`

Orientation Test

Particularly I want to call out the weird flecks of white on our render:

`<model-viewer>`	Filament `gltf_renderer`

Staging models in `gltf_renderer`

In our initial effort to compare <model-viewer> to other renderers, we hand-crafted tests and manually captured screenshots. In the course of doing this, we were lucky that Filament and <model-viewer> happened to stage certain models with near-pixel-perfect camera projection and relative positioning. For example, consider this fidelity test added in #361 :

Example of coincidentally similar staging

Unfortunately, this coincidence of staging strategies is just that: a coincidence. After adding the battery of Khronos sample model tests, it became immediately obvious that many types of models were not staged the same way by both renderers.

So, this change includes a modification of our Filament-based gltf_renderer. Model staging has been updated to closely match the strategy used by <model-viewer>'s own ModelScene:

https://github.com/GoogleWebComponents/model-viewer/blob/917251cb8fd1f65508078895862f22ac0504567a/src/three-components/ModelScene.js#L163-L204

https://github.com/GoogleWebComponents/model-viewer/blob/917251cb8fd1f65508078895862f22ac0504567a/src/three-components/ModelScene.js#L214-L240

Original, inconsistent staging	Updated staging strategy

Our Filament screen captures will now be staged as expected. Hopefully this change will allow us to easily/coherently adapt our Filament staging over time as we change <model-viewer> as well.

Follow-on work

The following issues have been filed during the course of work on this change, and represent follow-on work towards improved compatibility/conformance of <model-viewer> and consistency across renderers:

Failure to render glTFs with animations #371 <model-viewer> fails to render glTFs w/ animations
Support for models with multiple sets of UV coordinates #372 Support for multiple UVs
- Should be addressed in Three.js via GLTFLoader does not support texCoord fields mrdoob/three.js#12608
Bad render results for Alpha Blend Mode test model #381 Fix rendering for alpha blend modes
Bad render results for KHR_materials_unlit Khronos test model #374 Fix rendering for unlit models
- Change already available for consideration courtesy of @jsantell : Do not apply environment maps to unlit models. Fixes #374 #377
Pixelated PMREM textures #379 To address pixelation in environment maps
Samples do not support 16bpc PNG textures filament#832 Support 16bpc PNGs in Filament's gltf_viewer
- Fixed as of Update stb and add tnt/README. filament#835
Shared relevant compatibility results with Filament project here

Fixes #360

donmccurdy · 2019-02-20T15:46:14Z

Excited to see these tests set up, thanks! :)

Texture transforms

The description suggests Filament is correct on offset/scale and incorrect on rotation, but I think all three are wrong – looks like Filament doesn't implement that extension yet.

Multiple UVs

ThreeJS does support multiple UVs for the most common case – AO maps. But arbitrary textures can't be assigned to arbitrary UV maps, which is what this test shows, so that's a known issue.

Alpha blending

This is unexpected – I've commented on #381.

Overall – the staging looks similar on these, but the lighting does not. I can't tell from the PR if that's expected. In particular <model-viewer/> seems heavily front-lit, maybe? Are we matching lighting in <model-viewer/> or Filament, or is that a known caveat for now?

cdata · 2019-02-20T16:50:54Z

The description suggests Filament is correct on offset/scale and incorrect on rotation, but I think all three are wrong – looks like Filament doesn't implement that extension yet.

Good call, will fix the description.

arbitrary textures can't be assigned to arbitrary UV maps, which is what this test shows, so that's a known issue.

Is there an issue we can reference from here?

This is unexpected – I've commented on #381.

👍

Overall – the staging looks similar on these, but the lighting does not. I can't tell from the PR if that's expected.

Yeah, definitely. There was a bit of background discussion that I should have linked to that occurred during the PMREM change: #338

I'll include a brief summary in the PR description.

donmccurdy · 2019-02-20T17:30:12Z

arbitrary textures can't be assigned to arbitrary UV maps, which is what this test shows, so that's a known issue.

Is there an issue we can reference from here?

See: mrdoob/three.js#12608

jsantell

Looks great, amazing work here @cdata

jsantell · 2019-02-20T17:41:11Z

scripts/model-viewer-screenshot.js

+  process.exit(1);
+}
+
+screenshotCreator


For top level promise usage, I like to wrap an async iife (async function(){ ... })() to use async await.. which now I mention it, it's been awhile, I think node has native async awaits now.

It does. Will fix 👍

jsantell · 2019-02-20T17:41:40Z

scripts/install-third-party-renderers.sh

 pushd $FILAMENT_DIR

-git fetch --depth=1 origin
+#git fetch --depth=1 origin


Remove or add comment?

Removed by accident (couldn't git fetch from the commuter ferry 🙃)

jsantell · 2019-02-20T17:43:14Z

package.json

    "compare-fidelity": "./scripts/compare-fidelity-to-ref.sh",
    "install-renderers": "./scripts/install-third-party-renderers.sh",
    "update-screenshots": "node ./scripts/update-screenshots.js",
+    "checkout-samples": "./scripts/checkout-khronos-gltf-samples.sh",


nit, maybe get-samples or fetch-samples is a bit more obvious at what this does

Will fix 👍

jsantell · 2019-02-20T17:43:46Z

scripts/checkout-khronos-gltf-samples.sh

+REPO_URL=https://github.com/KhronosGroup/glTF-Sample-Models.git
+CLONE_PATH=./examples/assets/glTF-Sample-Models
+
+if [ ! -d "$CLONE_PATH/.git" ]; then


Maybe a warning here, or in the README, that this will pull down a large amount of data

Good call, will fix 👍

There is a warning in both places now.

smalls · 2019-02-21T19:08:52Z

README.md

 * `npm test` - Runs tests.
 * `npm run check-fidelity` - Compare rendering to third-party renderers
 * `npm run install-renderers` - Install or update third-party renderers
+* `npm run fetch-samples` - Create or update a local copy of sample models. This is needed for running fidelity checks and updating screenshots. Warning: the samples are hundreds of megabytes large in aggregate!


If this is required, does it make sense to merge fetch-samples in with (perhaps) install-renderers (another long, expensive prep step) into a single script named something like prepare-fidelity?

Or, if the thinking is that this is always run in npm install, maybe mention that (or leave it out entirely?)

I guess for factorization purposes it would be nice to keep these things separated. For usability purposes, we probably want a single command.

It's easy enough for us to make a combined command and just not document the sub-commands.

I'm also happy to leave it out entirely.

Agreed re factorization, I think particularly the backend scripts (the .sh or .js files) having those split and encapsulated helps to understand the code.

I like both a combined commands and undocumented sub-commands (I've used a - prefix to connotate internal sub-commands) or leaving that sub-commands out entirely.

The thing I like most is having the combined command for usability.

Not a blocker to a merge, though.

Added a command called npm run bootstrap-fidelity-dev that checks out samples and builds filament in one go. Removed docs for the individual fetch / install tasks.

cdata force-pushed the add-khronos-sample-models branch 2 times, most recently from 7164807 to ba08948 Compare February 20, 2019 03:54

cdata changed the base branch from automate-filament-screenshot-capture to master February 20, 2019 03:54

cdata force-pushed the add-khronos-sample-models branch from ba08948 to 046b433 Compare February 20, 2019 04:55

Add Khronos glTF sample models to fidelity suite

917251c

cdata force-pushed the add-khronos-sample-models branch from 046b433 to 917251c Compare February 20, 2019 05:27

cdata changed the title ~~(WIP) Add khronos sample models~~ Add khronos sample models to fidelity test suite Feb 20, 2019

cdata requested review from johnpallett, jsantell, mrdoob and smalls February 20, 2019 08:20

cdata changed the title ~~Add khronos sample models to fidelity test suite~~ Add Khronos glTF sample models to fidelity test suite Feb 20, 2019

cdata mentioned this pull request Feb 20, 2019

glTF support is missing features google/filament#528

Closed

jsantell previously approved these changes Feb 20, 2019

View reviewed changes

cdata dismissed jsantell’s stale review via 90ab4f0 February 20, 2019 23:44

cdata force-pushed the add-khronos-sample-models branch from 90ab4f0 to f34260a Compare February 21, 2019 00:47

smalls reviewed Feb 21, 2019

View reviewed changes

smalls previously approved these changes Feb 21, 2019

View reviewed changes

cdata dismissed smalls’s stale review via 0e8c1f4 February 21, 2019 23:47

cdata force-pushed the add-khronos-sample-models branch from f34260a to 0e8c1f4 Compare February 21, 2019 23:47

cdata requested review from jsantell and smalls February 22, 2019 01:47

Updates per feedback

579cd37

cdata force-pushed the add-khronos-sample-models branch from 0e8c1f4 to 579cd37 Compare February 22, 2019 01:57

smalls approved these changes Feb 22, 2019

View reviewed changes

cdata merged commit 3255878 into master Feb 22, 2019

cdata deleted the add-khronos-sample-models branch February 22, 2019 17:55

cdata mentioned this pull request Feb 22, 2019

Do not apply environment maps to unlit models. Fixes #374 #377

Merged

Conversation

cdata commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Khronos glTF sample fidelity tests

Analysis of initial results

16bpc PNG textures

Vertex colors

Texture transforms

Multiple UVs

Normal tangents

Unlit materials

Alpha blending

Other inconsistencies

Staging models in gltf_renderer

Follow-on work

Uh oh!

donmccurdy commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdata commented Feb 20, 2019

Uh oh!

donmccurdy commented Feb 20, 2019

Uh oh!

jsantell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smalls Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cdata commented Feb 18, 2019 •

edited

Loading

Staging models in `gltf_renderer`

donmccurdy commented Feb 20, 2019 •

edited

Loading

smalls Feb 21, 2019 •

edited

Loading