Implement renderer screenshot automation by cdata · Pull Request #351 · google/model-viewer

cdata · 2019-02-11T22:43:43Z

This change proposes a strategy to automatically collect updated screenshots for third party renderers over time to satisfy the requirements of #358 . This tooling will provide critical support as we approach #309 and #360. Some high level details of the change:

A Filament-based glTF renderer (gltf_renderer) has been introduced
- gltf_renderer is a mash-up of the gltf_viewer and frame_recorder samples in the Filament repo
- Generates a PNG representing a staged model given a glTF, a cmgen-produced IBL, a width and a height
- gltf_renderer is built by applying a patch on top of the Filament project repository and running Filament's build script
- Should serve as a baseline for future staging customizations
Filament-based gltf-renderer can be installed / updated by invoking npm run install-renderers
- Environment is presumed to be bootstrapped for building Filament
- Will eventually build other third-party renderers as we add them
Screenshots can be updated by invoking npm run update-screenshots
- Uses fidelity test configuration to determine what needs to be rendered / updated
- Attempts to build cmgen and gltf_renderer from scratch if they are not available
- Generates IBL artifacts with cmgen if they are not available
- Eventually this command should encompass more renderers as we discover how best to automatically collect their render results
A startup/provisioning script has been introduced to enable an Ubuntu VM or similar environment to produce screenshots
Cloud infrastructure has been provisioned to automatically produce screenshot update PRs based on daily checks performed at midnight (see below for details)

Automation strategy

In order to minimize the effort required to keep screenshots up to date, and also to reduce the likelihood of frustration arising from inconsistencies in local dev environments, cloud infrastructure has been introduced to automate screenshot collection updates.

Virtual infrastructure limitations

Initially, I expected that we could accomplish this as a specialized CI build on Travis. However, Filament requires a minimum OpenGL version of 4.1 to run. Initial investigation of Travis options here proved discouraging. It seems that a lot of standard virtual infrastructure does not support a recent enough version of OpenGL due to limitations of VMWare support in the Mesa GL driver.

In the end I was able to build Filament in Travis, but we are unable to use Travis to render because Filament cannot find a suitable rendering backend.

Enter GCP

Thankfully, the requirements for our automation are pretty basic: provision an environment to build Filament (and other renderers in the future), take some screenshots and then make a PR.

I turned to GCP in the hopes that a VM with attached GPU would offer a suitable version of OpenGL for rendering Filament. And I'm happy to report: it does.

The infrastructure that has been put in place is modeled after this one diagramed in a Google Cloud Scheduler tutorial:

The arrangement specific to our own infrastructure can be described as follows:

A VM instance rendermatic is presumed to have been created with an attached GPU, and with the startup script introduced by this change.
Every day at midnight, a PubSub message is dispatched to awaken rendermatic
PubSub message is observed by a designated Cloud Function, which starts the rendermatic VM
As rendermatic boots up, the startup script runs, producing screenshots and crafting a PR if necessary
As the startup script concludes, rendermatic is powered off
If there was any problem leading to rendermatic not being powered off, a subsequently scheduled PubSub message at 2 AM invokes a final Cloud Function that shuts down the VM

Generated update PRs

The automation infrastructure produces PRs if (and only if) there are changes among the fidelity test goldens checked into the repository. Here is an example of what the automatically crafted PRs look like:

It is expected that a human reviewer will look over the automatically generated PR before it is merged so that we an stay on top of any significant changes that occur in third party renderers over time.

Fixes #358

cdata · 2019-02-14T20:09:08Z

This has been rebased to include the changes (and notably the fidelity test) that landed in #338

smalls · 2019-02-14T20:52:57Z

scripts/filament-screenshot.sh

+fi
+
+if [ -z "$TMPDIR" ]; then
+  TMPDIR="/tmp/";


mktemp -d (with a template, so it might be mktemp -d filament) will be more portable.

Also trap "{ rm -f $TMPDIR; }" EXIT might do cleanup when the script exits (although it's not clear this is the right script, I'm still reading).

I think (after further reading) the tension is between my suggestion of mktemp against the desire to cache a build of filament so every run (especially repeated runs while developing) don't require filament rebuilds.

One way might be to cache the temp directory locally (echo $TMPDIR > .filament-dir, and then in the check use [ -d \cat .filament-dir` ]`.

Mmm, yeah. We could also just do as much diligence as possible and keep filament local to the project directory. That's actually how I wrote this the first time around. I'm not married to putting it in a temp directory. And yah, rebuilding it is a huge pain in the butt.

(and by diligence, I mean maintain a persistent clone of the Filament repo and use scripts to keep it up to date over time)

I've switched us from using a temp dir to maintaining an untracked "renderers" folder in the project repository. There is an install script that can be run to install and/or update Filament (and other renderers in the future).

smalls · 2019-02-14T20:55:24Z

scripts/filament-screenshot.sh

+fi
+
+if [ -f $SCREENSHOT_OUTPUT_PATH ]; then
+  rm $SCREENSHOT_OUTPUT_PATH


Should this be rm -rf?

... although, given that it's a parameter that feels dangerous. This seems like an error condition, not something to fix?

The screenshot is just a file, this might be a misleading variable name

Maybe drop the PATH?

Is SCREENSHOT_OUTPUT_FILE okay?

Yeah even better!

scripts/render-vm-startup-script.sh

smalls · 2019-02-14T22:13:26Z

scripts/render-vm-startup-script.sh

+
+# If Filament is still around, delete it so that we are sure to build it
+# cleanly from scratch
+if [ -d /tmp/filament ]; then


I think switch to mktemp in filament-screenshot.sh means we don't need to remove filament here (although there's a chance it might exist, the trap will probably remove it and the randomness of mktemp will mean we won't use it even if it exists)

.. assuming we go down the mktemp path of course.

After a bit more thought.. the reason I recommended this is that it seemed like a break in the encapsulation. It'd be great if one script knew about setting up the OS (see below), one script knows about compiling filament (and how to delete it), and one script knows how to start filament and capture a screenshot.

Sounds good. I'll refactor.

I have split the startup script into three subscripts that cover:

Provisioning the VM

Installing renderers such as Filament

Taking screenshots and producing a PR if needed

This ended up being a much better approach because - in addition to making the scripts easier to understand - the subscripts can be updated without changing the startup script configured for the VM.

smalls · 2019-02-14T22:19:25Z

scripts/render-vm-startup-script.sh

+    https://api.github.com/repos/googlewebcomponents/model-viewer/pulls
+fi
+
+sudo poweroff


I didn't know about poweroff! My incantation was shutdown now -h.

I was taught that poweroff was the clean way, but after reading the manpage for shutdown I wonder if that makes me a bad person:

The shutdown utility provides an automated shutdown procedure for super-users to nicely notify users when the system is shutting down, saving them from system administrators, hackers, and gurus, who would otherwise not bother with such niceties.

Haha. Well, not with now - it sends a notification, but there's no time to respond to it.

smalls · 2019-02-14T22:19:54Z

scripts/update-screenshots.js

+ * limitations under the License.
+ */
+
+// const {promisify} = require('util');


Remove comments?

smalls · 2019-02-14T22:21:45Z

scripts/update-screenshots.js

+      const filePath = path.resolve(scenarioDirectory, file);
+
+      switch (name) {
+        case 'Filament':


Maybe a default case with an error message in case something other than Filament gets passed in?

smalls · 2019-02-15T00:43:08Z

scripts/render-vm-startup-script.sh

+# Install NPM dependencies and update the screenshots
+npm install
+# NOTE: This will cause Filament to be compiled
+npm run update-screenshots


Capturing the screenshots and running comparisons (which appears to be here down?) seems to have a different purpose than setting up the VM (above).

I might split them into two separate scripts and have an orchestration script to do them both? It won't be functionally different but I think it might be clearer.

👍 Good call, will fix

Co-Authored-By: cdata <chris@scriptolo.gy>

cdata · 2019-02-15T05:49:34Z

I discovered that the Filament renderer does not produce consistently sized PNGs. On so-called high-DPI displays, the PNGs can be a multiple of the dimensions requested by the invoker (screenshots produced on the headless VM are the expected size). I'm working on extending the Filament patch to address this.

cdata · 2019-02-15T07:13:19Z

In 576c64a I expanded FilamentApp to support a callback for configuring the SDL_Window instance. This affords the gltf_renderer app an opportunity to decide if the window should be resized on account of UI scaling factor. There is no commonly accepted notion of UI scaling factor across platforms, so it is inferred by comparing the "display" size (the absolute pixel dimensions of the rendered image) and the "window" size (the requested dimensions of the window in scaled pixels).

Initially, I thought I could address this by deriving a scaling factor from the display density. However, I learned that (on Mac at least) display density does not directly correlate with scaling factor. I took a cue from the remarks section in this part of the SDL docs to land on the strategy I ultimately used for adjusting for high-DPI displays.

smalls · 2019-02-15T14:26:27Z

Good find on the scaling! I've added comments, but looking back through them they're not blockers.

jsantell

Wow, great work!

jsantell · 2019-02-15T18:57:48Z

scripts/update-screenshots.js

+const backgroundImageRe = /background-image\="([^"]+)"/;
+const modelSourceRe = /src\="([^"]+)"/
+
+const updateScreenshots = async (config) => {


config is a shadowed variable here

Thanks. Addressed this in 9bb6afc 👍

smalls · 2019-02-19T18:54:03Z

scripts/render-vm-startup-script.sh

-    https://api.github.com/repos/googlewebcomponents/model-viewer/pulls
-fi
+# Invoke relevant startup sub-scripts
+source $MODEL_VIEWER_CHECKOUT_DIRECTORY/scripts/provision-ubuntu-xenial-vm.sh


Why source instead of just invoking the scripts?

To spare having to set exported environment variables multiple times in different places

cdata force-pushed the automate-filament-screenshot-capture branch 30 times, most recently from bfe47f3 to bc10c7c Compare February 12, 2019 05:57

cdata mentioned this pull request Feb 14, 2019

Add fidelity test for HDR/PMREM #361

Merged

cdata force-pushed the automate-filament-screenshot-capture branch from 528c423 to f28fd8f Compare February 14, 2019 07:29

Chris Joel added 2 commits February 14, 2019 11:36

Implement Filament screenshot automation

a07f469

Rotate Filament IBL, not skybox

b29fcef

cdata force-pushed the automate-filament-screenshot-capture branch from ed8bb82 to b29fcef Compare February 14, 2019 19:36

smalls previously approved these changes Feb 15, 2019

View reviewed changes

Update scripts/render-vm-startup-script.sh

c03a7e5

Co-Authored-By: cdata <chris@scriptolo.gy>

cdata dismissed smalls’s stale review via c03a7e5 February 15, 2019 05:37

cdata and others added 2 commits February 14, 2019 21:58

Update README.md

812e93e

Screenshot size should be display-density-independent

576c64a

smalls previously approved these changes Feb 15, 2019

View reviewed changes

jsantell previously approved these changes Feb 15, 2019

View reviewed changes

cdata dismissed stale reviews from jsantell and smalls via 7c07e49 February 16, 2019 08:17

cdata force-pushed the automate-filament-screenshot-capture branch 8 times, most recently from 517007e to ebf3343 Compare February 16, 2019 22:17

Refactor startup script

9bb6afc

cdata force-pushed the automate-filament-screenshot-capture branch from ebf3343 to 9bb6afc Compare February 16, 2019 22:22

smalls reviewed Feb 19, 2019

View reviewed changes

smalls approved these changes Feb 19, 2019

View reviewed changes

Conversation

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

Uh oh!

Automation strategy

Virtual infrastructure limitations

Enter GCP

Generated update PRs

Uh oh!

cdata commented Feb 14, 2019

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!

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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

smalls commented Feb 15, 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!

cdata commented Feb 11, 2019 •

edited

Loading

cdata commented Feb 15, 2019 •

edited

Loading

cdata commented Feb 15, 2019 •

edited

Loading