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 new prepass to compute the specularity-glossiness map whatever the material type #12523

Merged
merged 11 commits into from May 30, 2022

Conversation

Mannns
Copy link

@Mannns Mannns commented May 13, 2022

This is the PR following this issue:
https://forum.babylonjs.com/t/prepass-get-accurate-reflectance-information/30351

The aim is to compute reflectance and glossiness information whatever the material type (standard, PBR, PBR Specular-Glossiness, PBR Metallic-Roughness). The architecture is based on the other PrePasses. The translation model from Metallic-Roughness to Specular-Glossiness is based on: https://marmoset.co/posts/pbr-texture-conversion.

@azure-pipelines
Copy link

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.

@Mannns Mannns changed the title add new prepass to compute the specularity-glossiness map whatever the material type Add new prepass to compute the specularity-glossiness map whatever the material type May 13, 2022
@azure-pipelines
Copy link

@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

Copy link
Member

@CraigFeldspar CraigFeldspar left a comment

Choose a reason for hiding this comment

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

This new SPECULARGLOSSINESS texture seems to be an improvement of our existing reflectivity texture, as it carries the same info in the default shader, and takes both workflows into account inside the PBR shader.

Shouldn't it then be more interesting to replace the content of the REFLECTIVITY texture instead of creating a new one ?
WDYT @sebavan ?

packages/dev/core/src/Shaders/default.fragment.fx Outdated Show resolved Hide resolved
…e already computed specularEnvironmentRo in pbr.frag
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12523/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

As @CraigFeldspar mentionned, I am all for merging both PrePass targets if possible ???

packages/dev/core/src/Shaders/pbr.fragment.fx Outdated Show resolved Hide resolved
mpoinsel added 2 commits May 17, 2022 10:03
…in pbrFrag. Changes in tests to avoid 1.0 default specularity of mirror and then reduce visibility of SSR artifacts
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

Copy link
Member

@CraigFeldspar CraigFeldspar left a comment

Choose a reason for hiding this comment

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

LGTM, nice fixes on SSR, thanks for that

…ss and metallicRoughness case without altering original behavior
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

…ssiness and standard materials whith specColor only
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Only a couple nits and good to go !!!

Thanks a lot

packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

LGTM

@sebavan
Copy link
Member

sebavan commented May 24, 2022

@Popov72 and @CraigFeldspar can you have a last eye ball at this PR ? thanks a ton

Copy link
Member

@CraigFeldspar CraigFeldspar left a comment

Choose a reason for hiding this comment

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

LGTM

@sebavan
Copy link
Member

sebavan commented May 26, 2022

@Popov72 do you see any issues before I merge ?

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

I trust you, @sebavan and @CraigFeldspar about the computations!

packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Rendering/geometryBufferRenderer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Shaders/geometry.fragment.fx Outdated Show resolved Hide resolved
packages/dev/core/src/Shaders/geometry.fragment.fx Outdated Show resolved Hide resolved
packages/dev/core/src/Shaders/pbr.fragment.fx Show resolved Hide resolved
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

@Popov72
Copy link
Contributor

Popov72 commented May 30, 2022

All good for me, thanks!

@sebavan sebavan merged commit b575ac8 into BabylonJS:master May 30, 2022
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

4 participants