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

Remove old b3dm/i3dm/pnts content classes #10635

Merged
merged 6 commits into from
Aug 4, 2022
Merged

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Aug 4, 2022

As part of #10346, this PR removes Batched3DModel3DTileContent, Instanced3DModel3DTileContent and PointCloud3DTileContent. Model3DTileContent will be used instead.

A couple of notes:

  • I also removed specs for b3dm classification, as Make ModelExperimental handle classification #10623 will handle this
  • I disabled the tests for Cesium3DTileBatchTable until we can consolidate that with ModelFeatureTable, since the tests used b3dm files which no longer use the old style of batch table (only .vctr and .geom do)

@j9liu could you review?

@ptrgags ptrgags requested a review from j9liu August 4, 2022 14:50
@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ptrgags
Copy link
Contributor Author

ptrgags commented Aug 4, 2022

@j9liu I checked the Sandcastles and they're all good except for the ones with the BIM model (since that change is only on main). I'm going to merge main -> replace-model then replace-model -> remove-1.0-contents so we can better test those Sandcastles.

@ptrgags
Copy link
Contributor Author

ptrgags commented Aug 4, 2022

@j9liu okay, the Sandcastles with the BIM tileset should work correctly now!

Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptrgags looks good for the most part, just a few comments!

I'll run through the Sandcastles locally as well to make sure no new bugs came up

Specs/Scene/Cesium3DTilesetSpec.js Show resolved Hide resolved
Specs/Cesium3DTilesTester.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/Model3DTileContent.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/Model3DTileContent.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Aug 4, 2022

@j9liu oops forgot to mention the PR is ready for review again!

@j9liu
Copy link
Contributor

j9liu commented Aug 4, 2022

I found a regression but I don't think it has to hold up this PR. In the 3D Tiles Formats sandcastle, PointCloudDraco isn't properly responding to the "Debug Colorize" option in the inspector:
draco point

For reference, this is what it does on sandcastle.com right now:

draco point main

Everything else looks good. Thanks @ptrgags !

@j9liu j9liu merged commit 9f62c9a into replace-model Aug 4, 2022
@j9liu j9liu deleted the remove-1.0-contents branch August 4, 2022 15:59
@ptrgags ptrgags mentioned this pull request Aug 5, 2022
4 tasks
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