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

removed the deprecations for planes in clipping plane collection and related items #6498

Merged

Conversation

hanbollar
Copy link

@hanbollar hanbollar commented Apr 25, 2018

Fixes #6317

@likangning93 did the changes - i'm not sure what you'd want me to do for line 506 with the reload check so i left that part as is but just removed the additional boolean for if any planes instead of clipping planes were added. - lmk if u want me to leave it this way and i can update the comment properly - or lmk if there's anything specific u want me to do here.

@cesium-concierge
Copy link

Signed CLA is on file.

@hanbollar, thanks for the pull request! Maintainers, we have a signed CLA from @hanbollar, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


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

🌍 🌎 🌏

Copy link
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

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

Sorry for the sluggish response! Just the one comment, and then I think this is good to go.

// But until then, we have no way of telling if they changed since last frame, so we have to do a full udpate.
var refreshFullTexture = this._multipleDirtyPlanes || this._containsUntrackablePlanes;
// But until then, we have no way of telling if they changed since last frame, so we have to do a full update.
var refreshFullTexture = this._multipleDirtyPlanes;
Copy link
Contributor

Choose a reason for hiding this comment

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

When more than one ClippingPlane is updated in between rendered frames, we refresh the entire clipping plane texture instead of just selectively updating a few adjacent pixels.
Looks like with the new changes though we can replace all uses of refreshFullTexture below with just direct access to this._multipleDirtyPlanes.

@likangning93
Copy link
Contributor

Ah wait also update CHANGES.md to indicate the deprecation.

@hanbollar
Copy link
Author

@likangning93 finished updating

@likangning93
Copy link
Contributor

That's all for me. @ggetz or @lilleyse, when either of you has a moment can you take a quick look and merge?

Interesting thing is that vanilla Plane objects will actually still work, they just won't update when linked to sliders etc. like in Sandcastle... heheheheh...

@ggetz
Copy link
Contributor

ggetz commented Apr 27, 2018

Thanks @hanbollar and @likangning93 !

@ggetz ggetz merged commit 5dff881 into CesiumGS:master Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants