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

Update snapshots and test configs #16126

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RaananW
Copy link
Member

@RaananW RaananW commented Jan 30, 2025

There was a change in lighting lately, but our tests didn't catch the changes due to the configuration of color-difference per pixel (threshold was 10%). This threshold was now reduced to 3%, and the number of changed pixels was lowered to 1% (strict!).
This resulted in many tests slightly failing.

All font-based tests have received a higher trheshold and other tests as well, depending on the test in hand.

Please review each of those, and check if it is in your responsibility. If it is, please check if the change makes sense or if something is wrong!

I also had to update a few playgrounds that simply didn't work correctly.

Note that some tests are still failing - deliberatly. I wanted to ask the people who added them to go over them and see what the reason for such a high change in pixels.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 30, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 30, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 30, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16126/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 30, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16126/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16126/merge/?snapshot=refs/pull/16126/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 30, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 30, 2025

@sebavan
Copy link
Member

sebavan commented Jan 30, 2025

cc @MiiBond to look into the pbr based tests who look different following the latest changes. Goal is to validate they are not regressions but expected changes.

@Popov72
Copy link
Contributor

Popov72 commented Jan 30, 2025

WebGL2 tests fail

I tested “water-material”, “FrameGraph nrge depth of field” and “GLTF Extension KHR_materials_volume with attenuation” locally, after making the same modifications as in your PR (threshold = 0.03, maxDiffPixelRatio = 1% - I rebuilt the sources by running npm run build:test-tools): these tests worked.

They also worked in WebGPU, except for “GLTF Extension KHR_materials_volume with attenuation”, which had a ratio of 3% different pixels. As this test is not flagged as an error by the CI in WebGPU mode, I'd tend to think that the errors we get for these tests are due to the difference between the backend we use on the CI and the backend used when generating the expected screenshot (usually Chrome or Edge on a desktop browser). What do you think?

WebGPU tests that fail

You can update the “terrain-erosion” image, it seems to be outdated, and the differences are not due to the latest changes in lighting conditions, I can see a difference with a version as old as 7.30.0.

"FrameGraph glow layer“ and "FrameGraph highlight layer" tests: the base model (”NeonPipe.gltf") renders slightly differently between WebGL and WebGPU. You can see this by browsing https://playground.babylonjs.com/#9YU4C5#2 and https://playground.babylonjs.com/?webgpu#9YU4C5#2 and switching between tabs: we can see differences on the inscription (front of the plate), and on the sides of the neon (the elbows). I think we should try to find out why we have such differences (at least on the inscription, where it's quite visible), but I don't have the time right now, I'll try to have a look at it next week. [...] I had a look, it's a problem with anisotropy, I will create a PR for it.

@MiiBond
Copy link
Contributor

MiiBond commented Jan 30, 2025

Regarding the coat visualization test, the change is expected. Here's an example of what's going on.
The base layer is a rough metal and we've recently changed the default behaviour to better match raytraced results:
nocoat_compare

And with a coat:
coat_compare

@MiiBond
Copy link
Contributor

MiiBond commented Jan 30, 2025

The glTF Animation Node test also exhibits some expected differences. The cubes all have roughness=1 and metallic=1 so the change is more pronounced.

@sebavan
Copy link
Member

sebavan commented Jan 30, 2025

Thanks to @MiiBond, we validated the PBR changes and they all look closer to ray tracing :-)

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.

5 participants