-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Thanks for the pull request @sanjeetsuhag!
Reviewers, don't forget to make sure that:
|
|
There was a problem hiding this 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.
@sanjeetsuhag as a smoke test, I tried loading the NYC dataset in ModelExperimental with this Sandcastle -- it's not showing up: It should look like this: |
As suspected, the issue above was due to mishandling of the coordinate transforms. The The Sandcastle works as expected now. |
@ptrgags I have addressed your feedback. |
There was a problem hiding this 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
There was a problem hiding this 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?
There was a problem hiding this 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/ModelExperimental/ModelExperimental3DTileContent.js
Outdated
Show resolved
Hide resolved
No comments, look good! |
Overview
This PR introduces a new
Cesium3DTileContent
class calledModelExperimental3DTileContent
. This class is used whenModelExperimental
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 theGtlfLoader
, but adds some additional functionality:The
Cesium3DTileContentFeatureTable
and all adjacent code has been removed. Additionally, theBatched3DModel3DTileContent
andGltf3DTileContent
classes have been restored to their pre-ModelExperimental
states.Task List:
B3dmLoader
Batched3DModel3DTileContent
andGltf3DTileContent
B3dmLoader
ModelFeatureTable
ModelExperimental3DTileContent
Testing