Make Geocoder terrain-aware #6876
Conversation
Previously, if a GeocoderService returned a single point, the geocoder would slam into the ground as close as possible. If the service returned a rectangle, it would only work with when the rectangle height did not include large elevation changes (and slam into the ground otherwise). This change queries the 4 corners and center of the rectangle (if the terrain supports it) and adjusts the geocoder final position height to be above terrain.
|
Thanks for the pull request @mramato!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
Yep, definitely doesn't work in 2D or Columbus View, thanks @cesium-concierge! |
|
OK, updated CHANGES and fixed 2D/CV, this is ready for review. |
|
@bagnell since this is largely camera related, can you review? |
|
👍 |
| if (defined(availability)) { | ||
| var cartographics; | ||
|
|
||
| cartographics = [ |
There was a problem hiding this comment.
@mramato These don't need to be on two separate lines but oh well. And shouldn't you use result parameters throughout this? I know it won't really impact performance but it's still a good habit
There was a problem hiding this comment.
We use result parameters as a necessary evil for performance in hot areas of the code. Not using them makes the code cleaner and is perfectly OK if performance is not going to be an issue.
|
This give drastically different result when there is and is not terrain. I think it's because you're multiplying the max height by 2. I think you want to add a constant height to it instead. Without terrain: With terrain: |
|
The pelias geocoder sometimes returns a bounding box where You should apply the same offset you did for a point destination. |
| // directly into the ground, even when not using terrain. | ||
| if (destination instanceof Cartesian3) { | ||
| var carto = ellipsoid.cartesianToCartographic(destination); | ||
| var offset = CesiumMath.toRadians(0.001); |
There was a problem hiding this comment.
I'm not convinced this is the right thing to do. The rectangle covers a larger area closer to the equator than the poles so the camera destination will have a greater height at the equator. From experimenting, the camera height at the equator is around 500m and at the poles it's 350m.
There was a problem hiding this comment.
Yeah, I agree. I was just moving code over from PeliasGeocoder and the thought just didn't occur to me. Instead I'm going to apply a fixed offset of 500m to any single point rather than expanding it into a rectangle at all. I'll still query the terrain at that single point, so it will still adjust properly and this will make for consistent zooming with and without terrain.
Good catch. |
Previously, if a GeocoderService returned a single point, the geocoder would slam into the ground as close as possible. If the service returned a rectangle, it would only work with when the rectangle height did not include large elevation changes (and slam into the ground otherwise).
This change queries the 4 corners and center of the rectangle (if the terrain supports it) and adjusts the geocoder final position height to be above terrain.
This is easy to test by zooming to
Grand Canyon, AZorMount Everest, Chinawith terrain on. In master the behavior is terrible.We could eventually pull some of this logic out into a separate helper function, but I figured I'd start small and we can do that if we need to do similar calculations elsewhere.
Fixes #6854