Switched Filament fidelity rendering from native to JS#682
Switched Filament fidelity rendering from native to JS#682
Conversation
cdata
left a comment
There was a problem hiding this comment.
Generally speaking, this looks like the route we want to take our fidelity testing on for the foreseeable future.
We should avoid checking in the cmgen binary. We could try downloading the tarball from their releases (after parsing the version from our own package.json. I'm happy to help layer this on to the PR.
Should the <model-viewer> goldens have changed as much as they did? Seems like they got lighter...
| @@ -0,0 +1,180 @@ | |||
| <!-- | |||
There was a problem hiding this comment.
This file needs a lot of work to bring it in line with the rest of the code base. I'm happy to tag team with you on it, but I'll leave the feedback below in either case.
| body { margin: 0; overflow: hidden; } | ||
| #container { position: relative; height: 100%; } | ||
| canvas { position: absolute; width: 100%; height: 100%; } | ||
| #messages { position: absolute; width: 100%; height: 100%; padding-left: 10px; } |
There was a problem hiding this comment.
We don't really have a strict CSS style guide, but I would say we should follow roughly this format:
selector.one,
#selector-two {
property: one;
property: two;
}This is consistent with what most documentation on the subject looks like, and also the Google style guide.
| <pre id="messages"></pre> | ||
| </div> | ||
| <script src="../../../node_modules/filament/filament.js"></script> | ||
| <script src="https://unpkg.com/gltumble"></script> |
There was a problem hiding this comment.
External packages should be expressed as devDependencies in our package.json file. Scripts on unpkg.com all map to NPM packages by name, so you can automatically add this dependency with npm install --save-dev gltumble.
From there it can be included similarly to how you include filament.js above.
That said, ideally both of these dependencies would be importable as modules. This isn't always possible, but we should check and see what is possible to make sure.
| </div> | ||
| <script src="../../../node_modules/filament/filament.js"></script> | ||
| <script src="https://unpkg.com/gltumble"></script> | ||
| <script> |
There was a problem hiding this comment.
This whole script needs to be externalized and converted to TypeScript.
| <script src="https://unpkg.com/gltumble"></script> | ||
| <script> | ||
| function getUrlParams() { | ||
| let params = {}; |
There was a problem hiding this comment.
Prefer const, because JavaScript is dumb
| const BG_IMAGE_URL = assetPath('spruit_sunrise_2k.jpg'); | ||
| const HDR_BG_IMAGE_URL = assetPath('spruit_sunrise_2k.hdr'); | ||
| const BG_IMAGE_URL = assetPath('spruit_sunrise_2k_LDR.jpg'); | ||
| const HDR_BG_IMAGE_URL = assetPath('spruit_sunrise_2k_HDR.hdr'); |
There was a problem hiding this comment.
Is LDR/HDR redundant? In other words, isn't a .jpg implicitly LDR? And isn't a .hdr implicitly, well... HDR?
There was a problem hiding this comment.
I did this because the caching is done by filename sans extension. Plus it's really easy to overlook the extension and think they're the same file otherwise.
| deviceScaleFactor: DEVICE_PIXEL_RATIO | ||
| } | ||
| }, | ||
| headless: headless |
There was a problem hiding this comment.
Why is headless set to false for Filament? (this has implications for our automation)
There was a problem hiding this comment.
In fact we had a similar issue even using their native code. Basically it doesn't work headless and I asked Phillip and he filed a bug with Chrome. It sounds like the emulation of the GPU isn't working with filament's shaders.
| slug: string, dimensions: Dimensions, | ||
| outputPath: string = | ||
| path.join(this.config.outputDirectory, slug, 'model-viewer.png')) { | ||
| const DEVICE_PIXEL_RATIO = slug.includes('Filament') ? 1 : 2; |
There was a problem hiding this comment.
Why is this different across the renderers? This seems like maybe something we should tweak in the App class we wrote for rendering with Filament.
There was a problem hiding this comment.
I don't know, that's how it was before. Seemed to be required to keep things working as is. Agreed that it would be nice to remove.
| await page.goto(url); | ||
|
|
||
| console.log(`🖌 Rendering ${slug} with <model-viewer>`); | ||
| const name = slug.includes('Filament') ? 'Filament' : '<model-viewer>'; |
There was a problem hiding this comment.
Just FYI Array.prototype.includes doesn't work in IE11 (no action required here).
| "web-component-tester": "^6.9.2" | ||
| }, | ||
| "dependencies": { | ||
| "filament": "^1.3.0", |
There was a problem hiding this comment.
Did part of your comment get cut off?
|
Closed as a subset of #713 |
Fixes #434
Fixes #515
Fixes #673
Technically #673 will only be closed once we update to the next Filament release, whenever that comes out, since they've now fixed their bug. Here I've switched from filament's native renderer to their JS version, which is a bit simpler and already has gltfio. I've also removed all the code to support building and running the native version of Filament.
I also updated our fidelity tests, switching from gltf to glb and removing some less useful ones. We're blocked on a few of the khronos tests because they don't have GLB versions and FilamentJS has a bug with opening GLTFs in another directory. That Filament bug is also already fixed, so we should be able to add them back in once we update (I only removed the config entries, so it should be quick to put them back).