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

Adds ModelExperimental3DTileContent, B3dmLoader, removes Cesium3DTileContentFeatureTable #9873

Merged
merged 32 commits into from
Oct 21, 2021

Conversation

sanjeetsuhag
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag commented Oct 13, 2021

Overview

This PR introduces a new Cesium3DTileContent class called ModelExperimental3DTileContent. This class is used when ModelExperimental is enabled and is meant to handle all types of 3D Tile content by passing them to the correct loading function (currently only supports .b3dm and .gltf/.glb).

It also adds a new B3dmLoader, which is a light wrapper around the GtlfLoader, but adds some additional functionality:

  • parsing feature table
  • parsing batch table
  • generating feature metadata
  • handling feature table semantics

The Cesium3DTileContentFeatureTable and all adjacent code has been removed. Additionally, the Batched3DModel3DTileContent and Gltf3DTileContent classes have been restored to their pre-ModelExperimental states.

Task List:

  • Fix states in B3dmLoader
  • Remove unused tests in Batched3DModel3DTileContent and Gltf3DTileContent
  • Adds tests for B3dmLoader
  • Add tests for new functionality in ModelFeatureTable
  • Fix tests for existing stages
  • Improve documentation
  • Add specs for ModelExperimental3DTileContent

Testing

@cesium-concierge
Copy link

Thanks for the pull request @sanjeetsuhag!

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

@sanjeetsuhag sanjeetsuhag marked this pull request as ready for review October 15, 2021 12:58
@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 15, 2021

Getting some weird behavior with picking instanced meshes. Looking into it now. Fixed

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@sanjeetsuhag reviewed the code, a few things need updating. I'll try this out in a Sandcastle next.

Source/Scene/ModelExperimental/B3dmLoader.js Outdated Show resolved Hide resolved
Specs/Scene/ModelExperimental/B3dmLoaderSpec.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor

ptrgags commented Oct 18, 2021

@sanjeetsuhag as a smoke test, I tried loading the NYC dataset in ModelExperimental with this Sandcastle -- it's not showing up:

image

It should look like this:

image

Sandcastle on sandcastle.cesium.com

@sanjeetsuhag
Copy link
Contributor Author

As suspected, the issue above was due to mishandling of the coordinate transforms. The B3dmLoader was applying the RTC transform directly on the node and ModelExperimentalSceneGraph was adjusting the transforms after that. Instead of having to add a "reverse" matrix multiplication step to account for the transform that will be applied later, I instead added an rtcTransform to the loader itself, and it allows simple multiplication of the modelMatrix (that only needs to happen once, not per-node).

The Sandcastle works as expected now.

image

@sanjeetsuhag
Copy link
Contributor Author

@ptrgags I have addressed your feedback.

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@sanjeetsuhag works correctly now! Though two things:

  • looks like B3DMParserSpec is missing
  • also check coverage results, I see that ModelExperimental3DTileContent is only at 68% coverage

Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

Looks good to me, @lilleyse did you want to take a look before I merge?

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Overall looks good and simpler than before. I like how tile content is now only accessed in a few places.

Source/Scene/Gltf3DTileContent.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/B3dmLoader.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/B3dmLoader.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/B3dmLoader.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/B3dmLoader.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelFeatureTable.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/B3dmLoader.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

No comments, look good!

@lilleyse lilleyse merged commit 9503ed1 into main Oct 21, 2021
@lilleyse lilleyse deleted the model-experimental-content branch October 21, 2021 23:48
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.

4 participants