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

Fix clipping plane issue with child tiles with transforms #6616

Closed
wants to merge 2 commits into from

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented May 22, 2018

Fixes #6600

Part of the problem may be that only the root's transform is used in the Cesium3DTile/contentVisibility function. I vaguely remember the reasoning behind this came up as a PR comment but I can't find the thread.

This is intentional - The tile intersection calculations were correct (turned on debugBoundingVolume in the below example to illustrate). The clipping planes' modelMatrix is relative to the root tile, so we should use that to determine the planes' location in world space.

The problem was that each tile's model shared a reference to the same clippingPlaneCollection (to save memory), and therefore couldn't account for additional child transforms. Added a private additional matrix to model which each tile sets individually so they can continue to share the reference to the clippingPlaneCollection.

Sandcastle example

@ggetz ggetz requested a review from lilleyse May 22, 2018 21:07
@cesium-concierge
Copy link

Signed CLA is on file.

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


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

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

lilleyse commented May 22, 2018

It looks like the root tile is no longer getting gradually clipped in the demo.

@@ -3009,7 +3012,8 @@ define([
if (!defined(clippingPlanes)) {
return Matrix4.IDENTITY;
}
return Matrix4.multiply(model._modelViewMatrix, clippingPlanes.modelMatrix, scratchClippingPlaneMatrix);
var clippingPlanesModelMatrix = Matrix4.multiply(model._clippingPlaneModelMatrix, clippingPlanes.modelMatrix, scratchClippingPlaneMatrix);
Copy link
Contributor

@lilleyse lilleyse May 22, 2018

Choose a reason for hiding this comment

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

modelViewMatrix already incorporates computedTransform, so this might be double applying it and leading to unexpected results.

@ggetz
Copy link
Contributor Author

ggetz commented May 23, 2018

@lilleyse Tweaked to use the proper transforms for i3dm and b3dm.

b3dm needs the inverse of the relative transform from the root tile.
i3dm needs the root tile transform to be applied to the modelInstanceCollection model.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 25, 2018

@ggetz @lilleyse what's the status here?

@lilleyse
Copy link
Contributor

Still working on a fix for the case here: #6600 (comment)

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 15, 2018

Related to or replaced by #7034? @lilleyse

@OmarShehata
Copy link
Contributor

I built off the solution here, so I'd say replaced by #7304.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 15, 2018

@ggetz please close if you are OK with that.

@ggetz ggetz closed this Sep 17, 2018
@ggetz ggetz deleted the clipping-planes-child-fix branch September 17, 2018 13:54
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.

6 participants