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 ClippingPlaneCollection.clone #6872

Merged
merged 3 commits into from Aug 3, 2018
Merged

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Aug 1, 2018

The presence of ClippingPlaneCollection.clone was causing a new ClippingPlaneCollection to be created every frame when it was the value of a ConstantProperty for a ModelGraphics.
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 and PrimitiveCollection 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.

@cesium-concierge
Copy link

Thanks for the pull request @hpinkos!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos hpinkos force-pushed the remove-clipping-collection-clone branch from 1d85782 to e02c825 Compare August 1, 2018 21:28
@lilleyse lilleyse requested a review from ggetz August 2, 2018 13:32
@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 2, 2018

@mramato can you review please?

@mramato
Copy link
Member

mramato commented Aug 2, 2018

@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.

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 2, 2018

@ggetz can you review?

Copy link
Contributor

@ggetz ggetz left a 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.

Copy link
Contributor

@ggetz ggetz left a 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.

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 3, 2018

@ggetz The model thing is already a bug in master, the bug of creating an new clipping plane collection every frame in ModelVisualizer just happened to make it work for model entities. I wrote up this issue: #6878
I can start looking at it, but if you have any suggestions for how to fix it that would be appreciated.

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 clone wasn't being used anywhere for the 3d tiles clipping stuff. Can you check to see if that exists in master as well?

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 3, 2018

Nevermind, I had a typo. I'll figure out what's going on with the model

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 3, 2018

Oh good, it's just a bug in the sandcastle example

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 3, 2018

@ggetz anything else?

@ggetz
Copy link
Contributor

ggetz commented Aug 3, 2018

Thanks @hpinkos The model edge clipping is resolved!

However, there's still a problem with tilesets. If I chose the BIM model and use maximumScreenSpaceError : 2, I can see the smoke stacks aren't getting clipped correctly, however the edges are still highlighted in the correct places. Maybe this is an issue with discard in the shader? Are you seeing the same?

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 3, 2018

@ggetz opened #6879

Anything else?

@ggetz
Copy link
Contributor

ggetz commented Aug 3, 2018

Other than that, this looks good. Thanks @hpinkos !

@ggetz ggetz merged commit c59efc9 into master Aug 3, 2018
@ggetz ggetz deleted the remove-clipping-collection-clone branch August 3, 2018 17:28
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.

None yet

4 participants