-
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
Remove ClippingPlaneCollection.clone #6872
Conversation
Thanks for the pull request @hpinkos!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
1d85782
to
e02c825
Compare
@mramato can you review please? |
@ggetz or @likangning93 are more familiar with this code, so they should probably be the ones to review. We should make sure there wasn't a good reason as to why this was added in the first place. |
@ggetz can you review? |
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.
clone
was there because the content of every model used to share the clipping plane collection's list of planes, but needed to apply their own modelMatrix transform, but we moved away from that in #6201.
I agree the breaking change should be fine, I doubt many people are using that function, if any.
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.
The edge styling toggle no longer works on model entities, and after testing, I think the modelMatrix property of the tile content is not getting assigned correctly as I thought. Try lowering the SSE on the tileset so more tiles are loaded.
@ggetz The model thing is already a bug in master, the bug of creating an new clipping plane collection every frame in I'm not sure what problem you found related to tile content, but I really don't think that's related to this PR since |
Nevermind, I had a typo. I'll figure out what's going on with the model |
Oh good, it's just a bug in the sandcastle example |
@ggetz anything else? |
Thanks @hpinkos The model edge clipping is resolved! However, there's still a problem with tilesets. If I chose the BIM model and use |
Other than that, this looks good. Thanks @hpinkos ! |
The presence of
ClippingPlaneCollection.clone
was causing a newClippingPlaneCollection
to be created every frame when it was thevalue
of aConstantProperty
for aModelGraphics
.This is also why we had to do this crazy
CallbackProperty
thing in the model clipping planes Sandcastle example.None of our other collections like
BillboardCollection
andPrimitiveCollection
have a clone method, and we weren't using it anywhere in our code, so I thought the best thing to do is remove it. I made it a breaking change instead of deprecating it because I don't think many (if any) of our users were using this function either.