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

Fixed terrain tile culling problems when under ellipsoid #8397

Merged
merged 7 commits into from
Dec 11, 2019

Conversation

IanLilleyT
Copy link
Contributor

Terrain tiles are culled when the horizon blocks them from view, as explained in blog part 1 and blog part 2. But there's a problem when tiles are under the ellipsoid. The second blog post says:

Also, please keep in mind an important caveat when using this approach. In the real world, objects occluded by the WGS84 ellipsoid are not necessarily occluded by the real surface of the Earth. This is because the Earth’s surface is actually slightly below the ellipsoid in parts of the world. Depending on your application, using WGS84 as the occluding volume may be acceptable, or you may need to use a more conservative ellipsoid.

Unfortunately, there are downsides to a more conservative ellipsoid: It increases the number of false positive draw calls and not all terrain providers have the up-front information of global min/max heights.

Instead, this PR does conservative ellipsoids per-tile. So if the tile's minimum height is below 0, it recomputes the tile's horizon occlusion point relative to a shrunk ellipsoid. This shrunk ellipsoid is shrunk just enough to be below the tile's lowest point, and no lower. During the tile culling pass it tests the tile's shrunk horizon occlusion point relative to its shrunk ellipsoid.

As shown in the picture below, the original horizon occlusion point is considered to be blocked by the horizon under existing code. In this PR, the new horizon occlusion point is relative to the shrunk ellipsoid and so is considered visible. (Side note: the shrunk horizon occlusion point would be in a slightly different position than the original. I forgot to draw it.)

IMG_20191114_140300

And here's the fix in action in Death Valley:

Before:
Screen Shot 2019-11-14 at 3 46 20 PM
After:
Screen Shot 2019-11-14 at 3 46 27 PM

Before:
Screen Shot 2019-11-14 at 3 48 43 PM

After:
Screen Shot 2019-11-14 at 3 48 35 PM

A much better test case is if you manually change the heightOffset of a heightmap terrain provider to be -10000.0. This makes all tiles go under the ellipsoid and the culling is seriously broken. These problems are fixed in this PR and the correct number of tiles are rendered when checking the cesium inspector.

I'm happy to hear thoughts on this approach and the implementation details. All occurrences of computeHorizonCullingPoint have been replaced with computeHorizonCullingPointPossiblyUnderEllipsoid and GlobeSurfaceTileProvider now calls isScaledSpacePointVisiblePossiblyUnderEllipsoid. An alternative could be to do if (minimumHeight < 0.0) { computeHorizonCullingPointUnderEllipsoid } else { computeHorizonCullingPoint }, but this would happen in several areas so I thought it would be better to make the Possibly version. Also, I considered each tile storing its own shrunk EllipsoidalOccluder, but thought it wouldn't be worth the memory overhead. However, this would avoid the kind-of-funky changes to EllipsoidalOccluder. Besides the API changes there's also a small runtime cost with this new approach: each tile that is under the ellipsoid has to recalculate the camera position in scaled space every frame. It's not especially heavy math so I'm not too concerned.

So let me know what you think! I'll add tests once the design is more finalized.

@IanLilleyT IanLilleyT requested review from lilleyse and kring November 14, 2019 21:45
@cesium-concierge
Copy link

Thanks for the pull request @IanLilleyT!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@IanLilleyT
Copy link
Contributor Author

@kring if you have any thoughts

@kring
Copy link
Member

kring commented Nov 16, 2019

This looks like a good change to me, @IanLilleyT! It should be safe, in the sense that it can only result in fewer tiles being culled. And the ellipsoid adjustment should be small for normal terrains (highly exaggerated ones I'm less sure about), so it shouldn't lead to noticeably more tiles being rendered, either.

I guess you probably already realized this, but it's still imperfect even with this change, though. It's still possible for tiles to be improperly culled. Here's a sketch to illustrate the problem:

image

Tile A is entirely above the ellipsoid, so no ellipsoid adjustment will happen. But tiles B, C, and D are below the ellipsoid. The camera is situated such that the ellipsoid is deemed to block the camera's view of tile A, so it gets culled. But in reality, A should be visible. This situation is admittedly a bit contrived, though, and this PR doesn't do anything to make it worse. I also think it'd be quite challenging to address this. It would almost certainly need to be done as part of the terrain processing rather than as a runtime tweak like in this PR.

So :shipit:!

@IanLilleyT
Copy link
Contributor Author

IanLilleyT commented Nov 16, 2019

Hmm, maybe a hybrid approach. Find the min loaded terrain tile, similar to #8398, and use that. Would have to see what performance is like but it would definitely solve these edge case culling problems. Could even get a speedup when all tiles are above the earth, and use that larger ellipsoid to horizon cull more aggressively. Haven't fully thought it through though.

@IanLilleyT
Copy link
Contributor Author

IanLilleyT commented Dec 3, 2019

Some more thoughts on this

The only way to horizon cull 100% correctly is to test each tile against the min tile's ellipsoid (see #8397 (comment)). There a few tricky parts about doing this:

  • The minimum rendered tile is not known until all tiles have been visited by the quadtree traversal, but the visibility check is part of the quadtree traversal, sooo.. you could use the previous frame's min tile, but what happens when a new tile enters view that is lower than previous frame's min? Maybe it could use its own ellipsoid in that case.
  • Need some way to recompute each tile's shrunk-ellipsoid occlusion point relative to the smallest ellipsoid, cheaply. The code for generating the occlusion point isn't fast enough to do per-tile, per-frame. Maybe we can precompute some of the values at tile-load-time that can get us partly there, such as cosAlpha, sinAlpha, and magnitude, and then be very careful with the math. Even with these optimizations I'm guessing isScaledSpacePointVisible will get at least two times slower. Maybe only do all this when the min tile changes? I don't know how often that happens.

@IanLilleyT IanLilleyT force-pushed the underEllipsoidCullingFix_TerrainTiles branch from b053e7e to 435bce5 Compare December 4, 2019 23:36
@IanLilleyT
Copy link
Contributor Author

@lilleyse ready for review

I refactored the EllipsoidalOccluder code and added some tests.

I also messed around for a while trying to trigger the pathological culling case, but couldn't get it. You would need a distant tile with a very abrupt height change. But the more distant a tile, the more likely it has some lower heights in it. So for now I'm moving forward with the original approach.

scratchCameraPositionInScaledSpaceShrunk
);

var distanceToLimbInScaledSpaceSquaredShrunk = Cartesian3.magnitudeSquared(cameraPositionInScaledSpaceShrunk) - 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks mostly similar to the code in the cameraPosition setter, aside from the minimumHeight. Can a shared function be used instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unrolled for performance since it's called once per tile per frame (at least for tiles that are partly under ellipsoid). I added a comment to clarify.

@IanLilleyT
Copy link
Contributor Author

@lilleyse ready for another review. I was going to change the name to isScaledSpaceTilePointVisible
and computeTileHorizonCullingPoint instead of isScaledSpacePointVisiblePossiblyUnderEllipsoid and computeHorizonCullingPointPossiblyUnderEllipsoid, but I thought it would be misleading since nothing is forcing these functions to be used with tiles. It's a private function so we can do another PR if a better name comes up.

@lilleyse lilleyse force-pushed the underEllipsoidCullingFix_TerrainTiles branch from f027939 to f8525f7 Compare December 10, 2019 22:41
@lilleyse
Copy link
Contributor

Tested a few other sites, this seems to have fixed most of the culling bugs. Thanks @IanLilleyT

@lilleyse lilleyse merged commit 2c488cc into master Dec 11, 2019
@lilleyse lilleyse deleted the underEllipsoidCullingFix_TerrainTiles branch December 11, 2019 00:41
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.

4 participants