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

Stop baking LH to RH in glTF serializer #13909

Merged
merged 10 commits into from
Jun 7, 2023

Conversation

bghgary
Copy link
Contributor

@bghgary bghgary commented May 26, 2023

Fixes #13891

See https://forum.babylonjs.com/t/glb-export-fails/40911

We used to try to bake the LH to RH conversion into the data. This change makes it such that it just puts a negative scale at the root instead. Doing the data conversion is too complex and probably isn't super beneficial to users.

@bjsplat
Copy link
Collaborator

bjsplat commented May 26, 2023

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 May 26, 2023

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 May 26, 2023

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 May 26, 2023

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 May 27, 2023

@bjsplat
Copy link
Collaborator

bjsplat commented May 27, 2023

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

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

@bjsplat
Copy link
Collaborator

bjsplat commented May 27, 2023

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/13909/merge/testResults/webgl2/index.html

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

@bghgary bghgary marked this pull request as ready for review June 2, 2023 20:11
@bghgary bghgary requested a review from sebavan June 2, 2023 23:10
@bghgary
Copy link
Contributor Author

bghgary commented Jun 2, 2023

@sebavan I made some more changes to fix tests if you can take a look. Once reviewed, it should be ready to 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.

Our life would be so much easier without this crappy handedness thing!

packages/dev/serializers/src/glTF/2.0/glTFExporter.ts Outdated Show resolved Hide resolved
packages/dev/serializers/src/glTF/2.0/glTFExporter.ts Outdated Show resolved Hide resolved
packages/dev/serializers/src/glTF/2.0/glTFExporter.ts Outdated Show resolved Hide resolved
@bjsplat
Copy link
Collaborator

bjsplat commented Jun 5, 2023

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/13909/merge/testResults/webgl2/index.html

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

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 5, 2023

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

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

1 similar comment
@bjsplat
Copy link
Collaborator

bjsplat commented Jun 5, 2023

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

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

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 5, 2023

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/13909/merge/testResults/webgl2/index.html

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

@sebavan sebavan enabled auto-merge June 7, 2023 21:16
@sebavan sebavan merged commit e45c425 into BabylonJS:master Jun 7, 2023
@bghgary bghgary deleted the gltf-serializer-rh-fixes branch June 7, 2023 23:11
@HoferMarkus
Copy link
Contributor

Hi @bghgary,
this change seems to break certain gltf exports, see this PG example.
Seems like all faces are flipped when starting the gltf export via inspector.

image

In versions before 6.7.0 it looks correct.

image

@bghgary
Copy link
Contributor Author

bghgary commented Oct 19, 2023

Thanks for the report! Do you mind filing a new issue for tracking purposes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GLB export fails for models with skeletons and child to a transform node
6 participants