BabylonJS / Babylon.js

Babylon.js is a powerful, beautiful, simple, and open game and rendering engine packed into a friendly JavaScript framework.
http://www.babylonjs.com
Apache License 2.0
22.76k stars 3.39k forks source link

Improve hasVertexAlpha handling #15153

Closed bghgary closed 1 month ago

bghgary commented 1 month ago

See https://forum.babylonjs.com/t/babylon-objexport-obj-mesh-serialization-bug/50856.

This is a fix for the first problem indicated in the forum post. The issue is that the vertex alpha flag is inconsistently handled between mesh, geometry, and vertex data causing mesh.flipFaces to create new vertex data with the wrong stride for colors. This change makes the flag more consistent across the board.

bghgary commented 1 month ago

I will also point out that mesh.flipFaces probably shouldn't be extracting every single vertex buffer just to flip the indices/normals. It would be better if mesh.flipFaces only changes the indices and normals without modifying other vertex buffers.

bjsplat commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

bghgary commented 1 month ago

I split the abstract class code into its own PR: #15160.

bjsplat commented 1 month ago

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 commented 1 month ago

Snapshot stored with reference name: refs/pull/15153/merge

Test environment: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15153/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15153/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/15153/merge https://sandbox.babylonjs.com/?snapshot=refs/pull/15153/merge https://gui.babylonjs.com/?snapshot=refs/pull/15153/merge https://nme.babylonjs.com/?snapshot=refs/pull/15153/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/15153/merge#BCU1XR#0

bjsplat commented 1 month ago

Visualization tests for WebGPU (Experimental)

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

bjsplat commented 1 month ago

WebGL2 visualization test reporter:

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

bjsplat commented 1 month ago

Visualization tests for WebGPU (Experimental)

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

bjsplat commented 1 month ago

WebGL2 visualization test reporter:

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

bjsplat commented 1 month ago

Visualization tests for WebGL 1 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/15153/merge/testResults/webgl1/index.html

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

bjsplat commented 1 month ago

Visualization tests for WebGL 1 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/15153/merge/testResults/webgl1/index.html

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

bghgary commented 1 month ago

After the CI reported some issues and some discussions with @sebavan, we decided that this is not the right way to fix this issue. It means #15160 is not strictly necessary. We may consider reverting it.

The new fix is #15162.