Geocoder Camera Fly To doesn't consider Z position when a terrain provider exists

c3-victor-mao commented 6 months ago

Feature

Context

While implementing a Custom Geocoder in Cesium 1.115, we noticed the Geocoder Camera FlyTo would always zoom in very close to the ground. With the normal Camera FlyTo, we are able to specify a Cartesian3 with a Z position for the camera so that all our objects would be in the camera's final field of view. We wanted the same for the Custom Geocoder but noticed that the GeocoderViewModel.js code would always place the camera height at the terrain provider elevation plus DEFAULT_HEIGHT (1000m) without considering the Cartesian3 Z-position.

Code References

Method where the GeoCoder flies to the destination: GeocoderViewModel.js#L365
Method where Cesium always adds DEFAULT_HEIGHT to the terrain provider elevation: GeocoderViewModel.js#L359

Feature Request

If the terrain provider is removed, then the Z-position is respected. However, is there a way to be able to position the camera at a certain Z-position, even if there is a terrain provider?

Maybe something like this for GeocoderViewModel.js#L356-L362:

return sampleTerrainMostDetailed(terrainProvider, [cartographic]).then(
    function (positionOnTerrain) {
      // const originalHeight = cartographic.height;
      cartographic = positionOnTerrain[0];
      // if (originalHeight <= positionOnTerrain[0].height) {
      cartographic.height += DEFAULT_HEIGHT;
      // } else {
      //  cartographic.height += originalHeight;
      // }
      return cartographic;
    }
  );

Or maybe this can be a flag passed into the Geocoder?

If this is a valid request, then we'd be happy to open a PR for this with your direction :)

ggetz commented 6 months ago

Thanks for the suggestion @c3-victor-mao. Would it make sense for your use case to be able to set DEFAULT_HEIGHT directly? Exposing that property may be the simplest solution in my mind.

c3-victor-mao commented 6 months ago

@ggetz thanks for the response! After thinking and discussing with my team, here are our thoughts:

No Terrain Provider:

Current Behavior: GeoCoder flies to the (X, Y, Z + DEFAULT_HEIGHT) position. This makes sense!
In this case, we think it would be handy to allow users to set DEFAULT_HEIGHT for flexibility.

Terrain Provider:

Current Behavior: GeoCoder flies to the (X, Y, terrain provider elevation + DEFAULT_HEIGHT) position.
Proposed Behavior: GeoCoder flies to the (X, Y, max(terrain provider elevation, Z) + DEFAULT_HEIGHT) position.
In this case, we also think it would be handy to allow users to set DEFAULT_HEIGHT for flexibility.

The ability to set DEFAULT_HEIGHT would solve technically solve our use case in that we could set the DEFAULT_HEIGHT to the desired Z-position above the terrain provider elevation before the Fly To action is executed. But it seems a bit tedious to do so before the GeoCoder Camera FlyTo.

What are your thoughts on the proposed behavior?

CesiumGS / cesium