Fidelity testing proposal by cdata · Pull Request #305 · google/model-viewer

cdata · 2019-01-23T23:21:23Z

`<model-viewer>` fidelity testing proposal

This change proposes a regime for measuring, analyzing and comparing the fidelity of renders produced by <model-viewer> and renders produced by selected reference model viewer implementations, as well as aggregating analyses in order to track significant changes over time.

Highlights of the change include:

Fidelity checks are configured with JSON (related types)
References recorded as manually staged / saved screenshots (example)
- As of this change, reference renderers include: Filament and iOS Quick Look
- Filament's GLTF viewer was adapted for staging consistency, see this fork for details
- An approximation of Quick Look's skybox has been created for staging <model-viewer>
I created a new model to be used as a litmus test for alpha blending behavior
A metric for computing perceived color distance is used to compare pixels between renders
- The core implementation is adapted from a third-party library called pixelmatch
npm run check-fidelity reports the current working tree's fidelity relative to configured references
npm run compare-fidelity $GIT_REF reports the fidelity of a Git ref, announcing notable changes or regressions compared to the current working tree
When a CI build completes, npm run check-fidelity is now run at the end
- Results also "compared" to most recent tagged release (no additional build step required)
- The threshold for alerting of a fidelity regression is when there is >=10% increased dissonance in any test at any threshold compared to the last release
When a release tag is created, fidelity artifacts are checked in to gh-pages branch as it is deployed
- Paves the way for future static dashboard to visualize progress over time (filed Static site dashboard visualizing fidelity progress over time #306 to track)
A dev-time tool is introduced that visualizes render results next to references, and supports interactive, visual analysis of how any given render compares to another

TODO

Document procedure for capturing goldens
Document steps to create a new test scenario
Add brief description to each scenario describing what it tests

Potential future work

Static dashboard that visualizes progress over time Static site dashboard visualizing fidelity progress over time #306
Expanded support for additional web-based reference viewers Support multiple "live" references per fidelity test scenario #308
Include discrete PBR tests against reference ray tracer Add discrete PBR cases to fidelity test suite #309

Examples

`npm run check-fidelity`

`npm run compare-fidelity $SOME_REF`

Artifacts produced by `check-fidelity`

Visual comparison tool

Fixes #289

jsantell

Great work! This will be valuable to have as glTF matures and as the renderers we are interested in change subjectively, and as we improve our own rendering, it'll be great to have a log of the deltas.

Other note, I think we could use something like a small README in a few dirs (src/test/fidelity, test/fidelity or maybe just test/) as the project grows. Something that I'm curious about, for a contributor, are these any actionable steps they should take when submitting a PR? What do I do with the information of a delta? Or is this more only for our infrastructure, and we will take care of interpreting what these deltas mean?

jsantell · 2019-01-24T18:40:13Z

rollup.config.js

+    },
+    plugins,
+    onwarn,
+  },


Build times are already longer than I'd prefer, I don't think we need to rebuild the image comparison on every change during development.

Current build times:

time npm run dev > @google/model-viewer@0.0.7 build /home/jsantell/Dev/model-viewer > tsc && rollup -c ./lib/model-viewer.js → ./dist/model-viewer.js... (!) Circular dependency: lib/model-viewer-base.js -> lib/three-components/Renderer.js -> lib/model-viewer-base.js created ./dist/model-viewer.js in 2.5s ./lib/test/index.js → ./dist/unit-tests.js... (!) Circular dependency: lib/model-viewer-base.js -> lib/three-components/Renderer.js -> lib/model-viewer-base.js created ./dist/unit-tests.js in 1.9s ./examples/dependencies/index.js → ./examples/built/dependencies.js... created ./examples/built/dependencies.js in 694ms real 0m10.352s user 0m18.311s sys 0m0.663s

Unintentional for it to rebuild on every change. Will fix if I see it doing that locally. But, the watch files are configured as:

include: '{lib/test/fidelity/**,lib/third_party/**}'

So it shouldn't be doing that 🤷‍♂️

As far as build times are concerned, we should always strive to reduce them. But, this item should only build once unless it is being actively dev'd on.

Confirmed, it only builds these new targets when I change files that correspond to the configuration.

jsantell · 2019-01-24T18:46:07Z

rollup.config.js

+    output: {
+      file: './dist/image-comparison-app.js',
+      sourcemap: true,
+      format: 'umd',


I don't think we gain anything from UMDifying these builds

jsantell · 2019-01-24T18:48:04Z

package.json

    "resize-observer-polyfill": "^1.5.0",
+    "rimraf": "^2.6.2",
    "rollup": "^0.66.0",
    "rollup-plugin-cleanup": "^3.0.0-beta.1",


With these new dependencies, I can definitely see the image comparison being a separate (in repo?) module in the future

Yeah, probably. But, I think it should probably bake a bit before we consider splitting it out into its own thing.

jsantell · 2019-01-24T18:49:55Z

src/test/fidelity/common.ts

+      for (let y = 0; y < height; ++y) {
+        for (let x = 0; x < width; ++x) {
+          const index = y * width + x;
+          const position = index * 4;


I don't think COMPONENTS_PER_PIXEL should be a constant (IMO one of the few things that can get away with being a magic number in some contexts, e.g. 255) but should be consistent

Something to think about, but implicit meaning is almost always worse for readability. 255 at least has the context that a Uint8ClampedArray only holds uints in the range 0..255. 4 can't even say that much.

go/tott/538

smalls · 2019-01-24T20:30:26Z

scripts/compare-fidelity-results.js

+            `"${slug}/${key}" @ threshold ${threshold}`;
+        const percentage = `${(delta * 100).toFixed(2)}%`;
+
+        if (delta > ALERT_THRESHOLD) {


Should this be Math.abs(delta)?

(and "decreased" on the next line changed)

No, because it's only bad if the delta is positive. Unless I flipped things around somewhere by accident... either way, the intended behavior is sign-dependent.

Oh, right.. a decrease is good?

I might restructure so the code is (very bad pseudo-code and wording):

if (delta > ALERT) alert else if (delta > 0) warn('the sources are more different'); else "great! the two sources are less different"

👍 WIll fix

Oh, right.. a decrease is good?

Negative deltas = more similarity than before

smalls · 2019-01-24T20:36:03Z

scripts/compare-fidelity-results.js

+              comparisonConstraints} decreased by ${percentage}!`);
+        } else if (Math.abs(delta) > 0) {
+          const changed =
+              delta > 0 ? 'decreased' : delta < 0 ? 'increased' : 'changed';


The third case seemed odd in code, although as I wrote it up it makes sense: delta==0, so changed='changed', so the output text is ".. changed by 0.00%".

The code would be clearer if instead of 'changed' this was 'didn''t change', maybe.

Oh, yeah, this is vestigial actually. Re-reading, the third condition can never be reached (a pre-condition of this path is that delta is not 0). Originally, it was logging every delta (even 0%), and I thought it was too noisy. I'll just remove the third condition.

smalls · 2019-01-24T20:53:59Z

src/test/fidelity/common.ts

+        const position = index * 4;
+        const delta =
+            colorDelta(candidateImage, goldenImage, position, position);
+        const bool = (delta < thresholdSquared ? 1 : 0) * 255;


Is bool short for Boolean, or something else?

Yes, it is short for boolean. Will fix.

(originally shortened because it overlaps w/ the TypeScript type of the same name)

smalls · 2019-01-24T20:59:32Z

src/test/fidelity/image-comparison-worker.ts

+        this.candidateContext = candidateCanvas.getContext('2d');
+        this.goldenCanvas = goldenCanvas;
+        this.goldenContext = goldenCanvas.getContext('2d');
+        this.booleanCanvas = booleanCanvas;


boolean (as in booleanCanvas) feels like a type, rather than a comparison algorithm - although I think you are referring to the comparison algorithm. Maybe a different name that'd be more descriptive?

Good call. Open to suggestions. I'll try to think of a better name.

Naming's hard.

The goal is to mark pixels that differ, without shading according to the amount of difference (which the deltaCanvas does), right?

Making it longer might help.. booleanComparisonCanvas, or comparisonAsBooleanCanvas (vs comparisonAsGradient, for the currently delta, maybe).

Or just toss a comment on the variable declaration, that'd work too.

We decided to change this to blackWhite* throughout

smalls · 2019-01-25T17:43:43Z

test/fidelity/README.md

+are intend to compare to include:
+
+ - [iOS Quick Look](https://developer.apple.com/arkit/gallery/)
+ - [Filament GLTF Viewer](https://github.com/google/filament/blob/master/samples/gltf_viewer.cpp)


nit: I think the preferred capitalization is "glTF" (even when in a title): https://www.khronos.org/gltf/

👍 Will fix

smalls · 2019-01-25T17:45:53Z

test/fidelity/pbr-spheres/index.html

+    <li>Top left: metallic 1, specular 1, roughness 0</li>
+    <li>Top right: metallic 1, specular 1, roughness 1</li>
+    <li>Bottom right: metallic 0, specular 0, roughness 0</li>
+    <li>Bottom left: metaalic 0, specular 0, roughness 0</li>


👍 will fix

smalls · 2019-01-25T17:47:13Z

Nice, I dig the README.md!

cdata · 2019-01-25T17:50:37Z

@jsantell in response to your questions, I have added a brief blurb on analyzing Travis build results to our wiki: https://github.com/GoogleWebComponents/model-viewer/wiki/Understanding-Travis-builds

Fidelity testing prototype

db8c6b0

cdata requested review from jsantell and smalls January 23, 2019 23:22

This was referenced Jan 24, 2019

Static site dashboard visualizing fidelity progress over time #306

Closed

Support multiple "live" references per fidelity test scenario #308

Closed

jsantell previously approved these changes Jan 24, 2019

View reviewed changes

smalls previously approved these changes Jan 24, 2019

View reviewed changes

cdata mentioned this pull request Jan 25, 2019

Add discrete PBR cases to fidelity test suite #309

Closed

cdata dismissed stale reviews from smalls and jsantell via 45472a1 January 25, 2019 17:21

cdata force-pushed the fidelity-test branch from 45472a1 to ff4543f Compare January 25, 2019 17:23

Document fidelity testing methodology

25e85e6

cdata force-pushed the fidelity-test branch from ff4543f to 25e85e6 Compare January 25, 2019 17:27

smalls reviewed Jan 25, 2019

View reviewed changes

smalls previously approved these changes Jan 25, 2019

View reviewed changes

Updates / cleanup per PR feedback

50b1bb5

cdata dismissed smalls’s stale review via 50b1bb5 January 25, 2019 18:30

smalls approved these changes Jan 25, 2019

View reviewed changes

cdata merged commit 1426922 into master Jan 25, 2019

cdata deleted the fidelity-test branch January 25, 2019 21:17

cdata mentioned this pull request Feb 20, 2019

Add Khronos glTF sample models to fidelity test suite #375

Merged

Conversation

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

Uh oh!

<model-viewer> fidelity testing proposal

TODO

Potential future work

Examples

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!

cdata Jan 24, 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!

smalls Jan 24, 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!

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 commented Jan 25, 2019

Uh oh!

cdata commented Jan 25, 2019

cdata commented Jan 23, 2019 •

edited

Loading

`<model-viewer>` fidelity testing proposal

cdata Jan 24, 2019 •

edited

Loading

smalls Jan 24, 2019 •

edited

Loading