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

Improve glTF material export code #13229

Merged
merged 4 commits into from
Nov 14, 2022

Conversation

bghgary
Copy link
Contributor

@bghgary bghgary commented Nov 11, 2022

See https://forum.babylonjs.com/t/texture-wangle-counld-not-save-in-glb/35207/4 for context. This only addresses the first two bullets.

There are a bunch of issues in the existing exporter code:

  1. Textures were being de-duped based on the name.
  2. Baking texture transform is not actually a valid solution when texture transform cannot fit the KHR_texture_transform extension.
  3. Exporting textures go through an unnecessary step of encoding from array buffer to base64 and then back. This is dumb.
  4. Code was a mess. Some weird async constructs and code duplication.

Changes:

  • Log a warning instead of baking the texture when the texture transform isn't center at the origin.
  • Rewrite a chunk of the material export code to de-dupe textures based on whether the texture points to the same internal texture (and mime type).
  • Factor the code.

@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

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.

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 but a lot could go wrong :-) it is funny it does not seem to impact tests ?

@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

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 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/13229/merge/testResults/webgl1/index.html

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

@bghgary bghgary marked this pull request as ready for review November 14, 2022 20:07
@bghgary bghgary merged commit cac7d14 into BabylonJS:master Nov 14, 2022
@bghgary bghgary deleted the gltf-export-materials branch November 14, 2022 20:07
RaananW pushed a commit that referenced this pull request Dec 9, 2022
Former-commit-id: d07c15d22d39ce1ecd93dec34f97dd95b4f14ab3
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.

3 participants