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

Use meshoptimizer npm module #9762

Merged
merged 1 commit into from
Aug 27, 2021
Merged

Use meshoptimizer npm module #9762

merged 1 commit into from
Aug 27, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Aug 27, 2021

This is part of #9473. Uses the npm module for meshoptimizer instead of storing the files in the repo. This change does not appear to affect build size (as expected, since only the decoder is included in the import).

This also fixes #9751, but I'm not sure where the console.warn call came from - the code inside if (WebAssembly.validate(detector)) { ... } still gets hit, but on the npm version there's no warning message. Was the warning part of an older release of meshopt (cc @sanjeetsuhag)? This shouldn't be an issue but I thought I'd make note of it.

Tested in Sandcastle with these changes merged into model-experimental-meshopt. I pushed the merge to that branch.

(edit my comment to see the local Sandcastle.. I think the link is too big to use regular formatting)

@ebogo1 ebogo1 requested a review from lilleyse August 27, 2021 18:55
@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Changes to third party files were made.
    • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@lilleyse
Copy link
Contributor

Looks good to me.

I converted the Avocado model with gltfpack:

./gltfpack -i ~/Code/glTF-Sample-Models/2.0/Avocado/glTF/Avocado.gltf -o ~/Desktop/avocado-meshopt.glb -cc

Then loaded it in this sancastle

@lilleyse lilleyse merged commit de176fb into main Aug 27, 2021
@lilleyse lilleyse deleted the npm-meshopt branch August 27, 2021 21:04
@lilleyse
Copy link
Contributor

I realized after the fact that meshoptimizer.js was not added to .gitignore. I pushed a fix to main: 44b1fb4

@ebogo1
Copy link
Contributor Author

ebogo1 commented Aug 28, 2021

@lilleyse Ah my mistake, thanks for catching!

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.

Warning: meshopt_decoder is using experimental SIMD support
3 participants