Rendering Engine: Fix alpha support #16144
Conversation
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16144/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/16144/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/16144/merge#BCU1XR#0 |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
@RaananW is there an easy way to flag some functions to error out if they are used within the repo itself ? so that we could be sure to not make any mistake by using them in the future. |
It doesn't, but it seems the problem is with NVidia GPU. Even Khronos Sample Viewer doesn't display it correctly on my computer (RTX 3080), contrary to what Ed Mackey says in KhronosGroup/glTF-Sample-Assets#146:
In Babylon with the PR:
I will create an issue in the Chromium repo about this bug. |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
Sorry, I wasn't expecting this change to fix the middle two issues. I'm only worried about the first one. I don't think it makes sense for the middle two in practice. I would not expect assets that use floating point boundaries to work. |
Any example in this PR? So I will understand the use case? |
Material needAlphaBlending() and needAlphaTesting() are only kept for back compat but should never be called from within the framework as we need to rely on their replacements needAlphaBlendingForMesh and needAlphaTestingForMesh. So we want to keep the old ones in but prevent their use internally. |
Set them to be deprecated. We can then enable a no-deprecated API rule on eslint. |
Perfect !!! cc @Popov72 |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
I took Gary's WIP PR (#15573) as a base, but decided to go another way: keep all the existing code and simply honor the
transparencyModevalue when it's set.This is because the existing code is quite complicated and has a lot of interconnections. I felt that trying to simplify it was too complicated and error-prone, so I made changes to simply use the
transparencyModeparameter when set and keep the existing code-path otherwise.At some point, we could deprecate the existing code and keep only the code related to
transparencyMode. In addition, I think it's easier to document the breaking change:transparencyMode, it will always be honored, whatever values you give to other properties (like mesh visibility, material alpha, etc)transparencyModeis null, no change is made to the existing behavior, which is fully compatible with the past.Other changes (from Gary's PR):
_shouldTurnAlphaTestOnhas been renamed toneedAlphaTestingForMesh. This is not a breaking change, since_shouldTurnAlphaTestOnwas protected, and more in line with the existingneedAlphaBlendingForMeshmethod.needAlphaTestinghave been replaced byneedAlphaTestingForMesh: the behavior might change because of this, but it's not a breaking change but a bug fix.Regarding failed visualization tests (3):
createScenefunction).transparencyMode=OPAQUEand define certain properties that allow blending in the current code path, whereas with the PR, the material is no longer transparent. To correct the tests, I settransparencyMode=ALPHABLENDanduseAlphaFromAlbedoTexture=false. Indeed, whenuseAlphaFromAlbedoTexture=true(as was the case in the test), the alpha of the albedo texture is taken into account whentransparencyMode=ALPHABLEND(as expected), but not in the actual code path whentransparencyMode=OPAQUE. So the images were different if I didn't setuseAlphaFromAlbedoTexturetofalse.cc @sebavan and @bghgary