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

Move Gltf-Pipeline to Source/Scene #9699

Merged
merged 4 commits into from
Jul 30, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Jul 29, 2021

Part of #9473.

Since there were eslint errors throughout I had to add a line to .eslintignore. Should these be fixed in Gltf-Pipeline? As far as I could tell they were all in one file so it shouldn't be to big of a fix.

Should there be a CHANGES.md line for this?

@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ Signed CLA found.
  • ❔ 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.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Jul 29, 2021

Build issues look to be due to prettier no longer ignoring GltfPipeline and something else with the tsconfig stage that I'm looking into.

@lilleyse
Copy link
Contributor

Thanks, once you have the tooling issues sorted out this should be good to merge. Eventually we'd like to use npm instead. Can you open an issue for that?

I checked over all the files to make sure gltf-pipeline wasn't getting into the CesiumJS documentation. I noticed that some functions weren't marked as @private. I pushed a fix in gltf-pipeline here: CesiumGS/gltf-pipeline@93f69ec and made the same fix here: c0be363

Should there be a CHANGES.md line for this?

Nope, this is all internal so no CHANGES.md entry required.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Jul 30, 2021

It looks like I was using an outdated version of gltf-pipeline. To revert the prettier changes I added a line in 3764981 to .prettierignore and copied the files over from the latest commit in gltf-pipeline, which looks like it added a few lines for meshopt in removeUnusedElements.js.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Jul 30, 2021

@lilleyse I had to restart CI for a bunch of spec errors that I couldn't reproduce locally with our Travis build steps and it worked the second time. I think this is good to merge.

@lilleyse lilleyse merged commit c8f576b into npm-third-party-staging Jul 30, 2021
@lilleyse lilleyse deleted the move-gltf-pipeline branch July 30, 2021 19:59
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