Allow applications to control terrain adjustment suspension

[gberaudo]

Fixes: #5999.
Fixes: #5837.

With this simple change, terrain adjustment suspension can be activated/disactivated at will by the application. The default behaviour stays unchanged.

In sandcastle, the code can be tested with:

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;
var globe = scene.globe;

var controller = scene.screenSpaceCameraController;
controller.minimumZoomDistance = 3000;

var cesiumTerrainProviderMeshes = new Cesium.CesiumTerrainProvider({
    url : 'https://assets.agi.com/stk-terrain/v1/tilesets/world/tiles'
});
viewer.terrainProvider = cesiumTerrainProviderMeshes;

setInterval(function() {
    var carto = Cesium.Cartographic.fromCartesian(scene.camera.position);
    console.log(carto.height - globe.getHeight(carto));
}, 10);

scene.camera.enableTerrainAdjustmentWhenLoading = true;

This PR fixes issues where the camera sinks under the terrain, and after (a long) time jumps above the terrain.

[cesium-concierge]

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

---

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

🌍 🌎 🌏

[hpinkos]

Thanks @gberaudo! @mramato, @bagnell does this change look good to you?

[mramato]

@gberaudo to continue our conversation from the other issue, I'm trying to understand how this helps things. Won't it let the camera just get stuck under terrain instead?

[gberaudo]

Hi @mramato,

With scene.camera.enableTerrainAdjustmentWhenLoading = false (the default), the behaviour is exactly the same as current one: adjustment only happens when all terrain has been loaded.

With scene.camera.enableTerrainAdjustmentWhenLoading = true, adjustment occurs on each frame: the camera will never be under the terrain and will not jump after all terrain is loaded (but the drawback is that the final position of the camera can be higher than expected due to terrain LOD height differences). This is the same behaviour as 1.5 year ago before @bagnell changed it.

The improvement is that the application can choose which mode it wants and change it at will. For example, it could be false on cold start up and then switched to true when the user is expected to navigate. Or it could be enabled false when flying and true otherwise (would be great in combination of #5993).

Does it clarify?

[gberaudo]

Hi @mramato, have you been able to look again into this PR?
We have been using it in production and it was great.

I have rebased the PR to fix the conflict in CHANGES.md.

[ggetz]

@mramato Would you be able to take another look? This fixes #5837 which has been brought up often on the Forum.

@gberaudo You'll need to merge and update CHANGES.md again.

[gberaudo]

Done @ggetz. Thanks for the follow up.

[mramato]

Thanks @gberaudo, sorry for the delay here, I've had almost no time to work on Cesium at all the last few months.

I'm a bit confused by your example, why are you setting controller.minimumZoomDistance = 3000;? That's the reason for the jump in master because it doesn't let you get more than 3000 meters close to the terrain. Setting this value to a smaller number makes the jump go away entirely (except for making sure the camera isn't below terrain once loading is done).

My primary issue with this change is that with the new camera mode creates other bugs, such as causing camera.setView and camera.flyTo calls to end up at the incorrect location when lower resolution tiles are are a higher elevation than higher resolution tiles (which is a very common case). This also creates a bug where you put the camera "on the ground" and after a few seconds the camera is hundreds of meters in the air, not because the camera jumped, but because the ground moved down.

We certainly want to make Cesium's camera more usable, but I'm not entire convinced this is the right way to do it. I would be interested to hear what @bagnell (I did talk to him a little offline), @emackey , and @pjcozzi think.

I still want to better understand the fundamental use case (I assume it's get rid of the "jump"?). If we do decide to take this change, I think we can come up with a friendlier name for the property and we would also need to document potential issues, like the setView/flyTo I mention above.

[pjcozzi]

I agree with @mramato that this is too much of a workaround without a holistic view of the camera. Perhaps @bagnell can chime in with some guidance on the fundamental issues in #5999/#5837 or we could pass on this until someone has enough time to dive deep?

[gberaudo]

@mramato, I used 3000m so that the issue is immediately visible.
In one of our applications we are using 300m:

• while the terrain is loading the camera goes under 300m;
• when all terrain finish loading, the camera is fixed at 300m producing the jump.

To be optimal, the functionality I propose require collaboration from the application (to handle flyTo/setView near the ground). That being said, in our application I simply set enableTerrainAdjustmentWhenLoading = true and the client is happy with it: no more jumping and no sinking under the terrain (which were viewed as a bug/regression).

A solution, that would not require application collaboration might be to:

• take into account the camera position to load first the terrain below it;
• suspend adjustement only when the terrain below the camera is not loaded.
  However:
• it is a core change with possible implications;
• it may produce holes in the terrain (if there are adjacent tiles of very different LOD).

@pjcozzi, in comparison, the proposed change is straightforward and immediately brings some improvement to the persons affected by the current behaviour. In the meanwhile, I am relying on the Camptocamp fork at https://www.npmjs.com/package/@camptocamp/cesium which contains an equivalent workaround.

[cesium-concierge]

Thanks again for the pull request!

Looks like this pull request hasn't been updated in 30 days since I last commented.

To keep things tidy should this be closed? Perhaps keep the branch and submit an issue?

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

🌍 🌎 🌏

[ediebold]

I know this may be dead now, but I just wanted to provide a use case for why this fix could be useful.

In our application we store "views", which are basically a saved camera location and rotation (and also loads relevant imagery layers and entities). The idea being that a user can save a "view", and then load it up again and see the exact same scene. At the moment, this doesn't work because the camera jumps to the location, but then gets juggled up and down as the terrain loads in, ending up in a different spot. This is particularly troublesome when a view is setup to be at a certain height right next to a set of 3d tiles or a model, since it can end up looking well above or below the desired part of the model.

[mramato]

At the moment, this doesn't work because the camera jumps to the location, but then gets juggled up and down as the terrain loads in, ending up in a different spot. This is particularly troublesome when a view is setup to be at a certain height right next to a set of 3d tiles or a model, since it can end up looking well above or below the desired part of the model.

@ediebold The specific behavior you describe was a bug and should be fixed in the with the next release of Cesium on Monday. See #6989 The camera was never supposed to jump while terrain was being loaded in, only once all terrain tiles are loaded (and only if the camera was underground in a completely loaded scene). If you have a chance to try out master before the release, it would be great if you could confirm the fixed behavior for us.

[cesium-concierge]

Thanks again for your contribution @gberaudo!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

---

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

🌍 🌎 🌏

[hpinkos]

@mramato @bagnell what's the plan for this PR?

[cesium-concierge]

Thanks again for your contribution @gberaudo!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

---

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

🌍 🌎 🌏

[cesium-concierge]

Thanks again for your contribution @gberaudo!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[hpinkos]

Thanks for the pull request @gberaudo, but I'm going to close this. We've had other PRs merged, like #6989, that seem to have already addressed the issue you were seeing. If you are still seeing problems, let us know and we can try to work together to figure out the best solution =) Thanks again!

[bbehling-trimble]

Has this fixed been merged into a recent Cesium release?

[hpinkos]

@bbehling-trimble no, this change was not merged. However, we had other significant improvements for streaming and loading terrain added in the CesiumJS 1.55 release. Learn more in this blog post: https://cesium.com/blog/2019/03/06/terrain-rendering-improvements/

[hpinkos]

@bbehling-trimble and other fixes related to the camera going under terrain were merged in #6989

[bbehling-trimble]

I upgraded to v1.55 and I'm still able to get the camera under the surface

[hpinkos]

@bbehling-trimble add any comments to #5837 please. Thanks!