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

Add anisotropy tests to render-fidelity-tests (and update Babylon + Filament to latest) #4535

Merged
merged 31 commits into from
Nov 8, 2023

Conversation

bhouston
Copy link
Contributor

This builds upon my previous PR that upgrades from glTF-Sample-Models to glTF-Sample-Assets: #4534

This PR pulls in additional tests from the new glTF-Sample-Asset repository, specifically 3 new anisotropy tests.

I have also upgraded Babylon.JS and Filament to their latest versions and updated their goldens as well as model-viewer's goldens.

@bhouston
Copy link
Contributor Author

Fixes #4486

@bhouston
Copy link
Contributor Author

I can not reproduce this Github unit test failure:

🎨 Scenario: khronos-AnisotropyBarnLamp
Start to compare model-viewer's golden with other renderers' goldens:

🔍 Comparing <model-viewer> to Filament

  📊 Decibels of root mean square color distance: -5.15

🔍 Comparing <model-viewer> to Babylon

  📊 Decibels of root mean square color distance: -5.35

🔍 Comparing <model-viewer> to gltf-sample-viewer

  📊 Decibels of root mean square color distance: -5.36

🔍 Comparing <model-viewer> to three-gpu-pathtracer

  📊 Decibels of root mean square color distance: -3.46

💾 Recording analysis
start compare model-viewer's golden with model-viewer's screenshot generated from fidelity test:
🚀 Launching browser
🗺  Navigating to http://localhost:9030/packages/render-fidelity-tools/test/renderers/model-viewer/?hide-ui&config=../../config.json&scenario=khronos-AnisotropyBarnLamp
🖌  Rendering khronos-AnisotropyBarnLamp with <model-viewer>
🖼  Capturing screenshot

  📊 Decibels of root mean square color distance: -19.90

When I run this exact same code locally I get this result:

🎨 Scenario: khronos-AnisotropyBarnLamp
Start to compare model-viewer's golden with other renderers' goldens:

🔍 Comparing <model-viewer> to Filament

  📊 Decibels of root mean square color distance: -5.15

🔍 Comparing <model-viewer> to Babylon

  📊 Decibels of root mean square color distance: -5.35

🔍 Comparing <model-viewer> to gltf-sample-viewer

  📊 Decibels of root mean square color distance: -5.36

🔍 Comparing <model-viewer> to three-gpu-pathtracer

  📊 Decibels of root mean square color distance: -3.46

💾 Recording analysis
start compare model-viewer's golden with model-viewer's screenshot generated from fidelity test:
🚀 Launching browser
🗺  Navigating to http://localhost:9030/packages/render-fidelity-tools/test/renderers/model-viewer/?hide-ui&config=../../config.json&scenario=khronos-AnisotropyBarnLamp
(node:83665) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)
🖌  Rendering khronos-AnisotropyBarnLamp with <model-viewer>
🖼  Capturing screenshot

  📊 Decibels of root mean square color distance: -Infinity
💾 Recording configuration
✅ Results recorded to /Users/bhouston/Coding/Work/model-viewer/packages/render-fidelity-tools/test/results

@bhouston
Copy link
Contributor Author

@elalish I removed the anisotropyBarnLamp test so that we pass the fidelity test suite. So I believe this is ready to merge.

elalish
elalish previously approved these changes Oct 25, 2023
Copy link
Collaborator

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it this takes the place of #4534? Not a big deal, but it's a bit easier to review these things if you just push changes to that branch/PR instead of creating a new one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this example from three-gpu-pathtracer? In future we should probably evaluate if three-gpu-pathtracer is stable enough to keep in these tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed it as you requested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure?

@bhouston
Copy link
Contributor Author

@elalish wrote:

I take it this takes the place of #4534? Not a big deal, but it's a bit easier to review these things if you just push changes to that branch/PR instead of creating a new one.

Technically each of these PR's build upon the previous. So if you wanted to merge #4534 first as it is good.

Then we can modify this one, and then once it is in, we can then discuss the next one that adds more models: #4537

@bhouston
Copy link
Contributor Author

I've merged in this other PR into this one: #4537. They both only add tests, so we can reduce the review overhead by having less PRs.

@@ -36,7 +36,7 @@
"@types/puppeteer": "^5.4.6",
"@types/rimraf": "^3.0.1",
"@types/three": "^0.156.0",
"filament": "1.31.5",
"filament": "1.44.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did none of the existing renders change appreciably when these versions were bumped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you merge in the other PR, I can run the fidelity test with "filament" as the target renderer and find out!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, lots of PRs merged - let's see how this does when it's all pulled together!

@bhouston
Copy link
Contributor Author

I've updated the branch and added back the BarnLamp example but I excluded it from "model-viewer" as it doesn't render properly in the Github Action (although it works on my machine.) I then modified the CLI tools so that "npm run test" excludes fidelity tests of the current renderer so that it doesn't check if BarnLamp is good for the default "model-viewer".

So I think if all tests pass, this is now good to merge.

elalish
elalish previously approved these changes Nov 6, 2023
Copy link
Collaborator

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks! @gkjohnson what's the status with three-gpu-pathtracer these days? Are you planning to upkeep it?

@gkjohnson
Copy link
Contributor

This looks great, thanks! @gkjohnson what's the status with three-gpu-pathtracer these days? Are you planning to upkeep it?

I've tried to but browser vendors (mostly Apple and Safari) keep adding api regressions that cause it to break after fixes have been added 😓 The recent blocker is that Apple's recent OS update introduced issues that cause the path tracer to crash hard so I can barely test it at the moment since I don't have easy access to my other machine. The bug is being tracked here.

I'm going to wait this out a bit to see what happens with this fix before diving into some of the new features too much more. Some others are helping to improve the renering on iOS so the project is still moving - but at the moment it's finding a few browser bugs 😅

@elalish
Copy link
Collaborator

elalish commented Nov 8, 2023

@gkjohnson Gotcha, thanks. I'm having trouble even on Chrome, I think because of updating three.js? Do you want to take a look at why we seem to just get blank renders now?

@bhouston
Copy link
Contributor Author

bhouston commented Nov 8, 2023

@elalish could we just commit this as is? Bugs in three-gpu-pathtracer is a bit orthogonal to his PR. And what is great is that @gkjohnson can use the new tests in this PR to further improve his renderer.

Copy link
Collaborator

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks!

@elalish elalish merged commit 25c51e7 into google:master Nov 8, 2023
3 checks passed
@gkjohnson
Copy link
Contributor

gkjohnson commented Nov 9, 2023

@gkjohnson Gotcha, thanks. I'm having trouble even on Chrome, I think because of updating three.js? Do you want to take a look at why we seem to just get blank renders now?

Without knowing your platform it's hard to say. On Mac these issues are operating system-level driver issues that affect all browsers recently introduced with MacOS 14.0. But like I said I can't run things on my machine on Safari or Chrome due to these system regressions and no reported errors. I'm hoping there's some progress from Apple sooner rather than later.

JL-Vidinoti pushed a commit to vidinoti/model-viewer that referenced this pull request Apr 22, 2024
…ilament to latest) (google#4535)

* init submodule for glTF-Sample-Assets.

* fix left over popd in fetch khronos gltf-smaples.

* remove tests from config.json that no longer have corresponding assets in glTF-Sample-Assets

* upgrade babylon + filament, check in their latest goldens for new anistropy tests

* update config.json with the new anisotropy tests.

* add more time for tests.

* add more gltf tests along with goldens for filament,babylon and model-viewer.

* using checkout with submodule support.

* using checkout with submodule support.

* update goldens for model-viewer + filament for khronos-TextureTransformMultiTest.

* update goldens for model-viewer + filament for khronos-TextureTransformMultiTest.

* add more goldens for anisotropy from gltf-sample-viewer and three-gpu-pathtracer

* add more goldens for anisotropy from gltf-sample-viewer and three-gpu-pathtracer

* exclude a few renderers for now to get the PR accepted.

* exclude a few renderers for now to get the PR accepted.

* add gltf-sample viewer goldens for new tests.

* add goldens for AnisotropyDiscTest

* remove barn lamp test for now.

* remove blank golden image.

* remove barn lamp test for now.

* exclude three-gpu-pathtracer from rotation test.

* exclude three-gpu-pathtracer from rotation test.

* remove failing test for now.

* add missing anisotropy goldens, and re-add barn lamp with model-viewer exclusion.

* ensure goldens tests are excluded if renderer is excluded.

* remove blank image.
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

3 participants