Clamping to 3D Tiles

[lilleyse]

Fixes #6080

This adds support for sampling the height of geometry in the scene at a given position. The most common use case for this is to clamp objects to 3D Tiles. Unlike pick, pickPosition, and others this approach works mostly independent of the current camera view.

Added to the public API:

• scene.sampleHeight - takes a cartographic position and returns a height.

Private functions that will have lots of use in the future

• scene.pickFromRay
• scene.pickPositionFromRay
• scene.drillPickFromRay
• scene.drillPickPositionFromRay

Underlying this Scene.js now has a concept of Views where each view contains framebuffers, command lists, cameras, and viewports specific to that view. The pick-ray functions uses a view with a 1x1 orthographic camera.

While working on this I've been thinking a lot about how we might eventually support multiple views on the same screen. I think this PR brings us like 50% there but there is still a lot of work to do. drawingBufferWidth/Height is hard coded almost everywhere...

For reviewing I recommend reading the commits in order. I did a bunch of cleanup as I tried to fully wrap my head around Scene.js. A lot of the log depth changes were added so that derived commands would not thrash when going between rendering the scene with log depth and then rendering the 1x1 orthographic pass.

To do:

• Remove Clamp To Tileset development demo
• Create Sandcastle demo
• Specs
• A lot of manual testing

After:

• Update any links that reference Ground%20Clamping.html

Later PR:

• sampleHeight working for offscreen 3D Tiles and globe
• 2D and CV support
• More log depth thrashing fixes - depth plane, post processing (?), ellipsoids

[cesium-concierge]

Thanks for the pull request @lilleyse!

• ✔️ 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.

Reviewers, don't forget to make sure that:

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

---

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

🌍 🌎 🌏

[lilleyse]

I'm thinking about breaking this out into several PRs to help make reviewing easier: one with general Scene cleanup, one with rendering using separate views, and one with the ray picking additions. Stay tuned...

[lilleyse]

Opened #6957 and #6958. Check those out first before reviewing this.

[pjcozzi]

This is missing the thumbnail? Perhaps gallery/development/Clamp to Tileset.jpg is in the wrong location?

[pjcozzi]

Seems like this will be done a lot - at least for clamping models where we want to exclude them from the pick - would it be better for scene.sampleHeight to take an optional array of primitives to exclude/hide?

Just don't assume show was true when resetting a primitive that was passed in.

[lilleyse]

I was thinking the same thing and was going to leave a comment about that. Added now.

Will need #7024 to be fixed before entities can be accepted in the array.

[pjcozzi]

Should this line until the end of the function also basically be a helper in the Cesium API, e.g.,

scene.clampToHeight(cartesian);

[lilleyse]

I added this helper function but I feel like it is kind of trivial and not deserving of a function in Scene. In general it doesn't seem like we have two functions that perform the same task but with cartesian vs. cartographic inputs.

This is what it looks like, what do you think?

    /**
     * Clamps the given Cartesian position to the scene geometry along the geodetic surface normal.
     * May be used to clamp objects to the globe, 3D Tiles, or primitives in the scene.
     * <p>
     * This function only samples height from globe tiles and 3D Tiles that are rendered in the current view. All other
     * primitives are sampled regardless of visibility.
     * </p>
     *
     * @param {Cartesian3} cartesian The Cartesian position.
     * @param {Object[]} primitivesToExclude A list of primitives and/or entities to exclude when clamping.
     * @param {Cartesian3} [result] An optional object to return the clamped position.
     * @returns {Cartesian3} The modified result parameter or a new Cartesian3 instance if one was not provided.  This may be <code>undefined</code> if there is nothing to clamp to.
     */
    Scene.prototype.clampToHeight = function(cartesian, primitivesToExclude, result) {
        var globe = this.globe;
        var ellipsoid = defined(globe) ? globe.ellipsoid : this.mapProjection.ellipsoid;
        var cartographic = Cartographic.fromCartesian(cartesian, ellipsoid, scratchPickCartographic);
        var height = this.sampleHeight(cartographic, primitivesToExclude);
        if (defined(height)) {
            cartographic.height = height;
            return Cartographic.toCartesian(cartographic, ellipsoid, result);
        }
    };

[pjcozzi]

Seems OK to me. It feels trivial to you, but to the end user I think it is separate enough use cases. If we end up with too many functions on Scene, we can evaluate grouping them into a new sub-object or elsewhere.

[pjcozzi]

Are you sure about hardcoding this value? Seems arbitrary.

[lilleyse]

I just grabbed this directly from OrthographicFrustum. I'll just let it use OrthographicFrustum's default instead of hardcoding here.

Basically it just has to be a large number.

[pjcozzi]

Is it cleaner to put the picking functions in a separate private file just so Scene.js isn't so large?

[lilleyse]

If it's just these two function pickFromRay and drillPick I don't think it helps to separate them out. pickFromRay is calling a bunch of Scene's internal function anyways and it would be less clean to move it to another file.

[pjcozzi]

OK.

[pjcozzi]

What other properties does each object have? Why not just return an array of primitives?

[lilleyse]

This is copied from the pick and drillPick doc.

I don't think the set of properties is standardized. Model has it's own pick object that includes the node, 3D Tiles uses Cesium3DTileFeature, etc.

[pjcozzi]

OK, can this and the other functions provide a bit more context?

[OmarShehata]

A lot of people are asking for pickFromRay functionality. (See here, here, and here).

If it's easy enough to make those functions public I think it'd be worth it.

[pjcozzi]

Let's wait on pickFromRay and friends until we are able to fully think it through.

[hpinkos]

@lilleyse I would like to wait for the next release to merge this in. This is a pretty big change to be merging in at the last minute. Is there any reason it has to make this release?

[mramato]

I agree with @hpinkos. Given the regressions we introduced in traversal last release, I'm very hesitant to merge this before the release on Monday without an extremely good reason.

[lilleyse]

Yes it's a bit close to the release for comfort. But if it helps most of the new functionality is relatively isolated and the more sweeping changes in #7039, #6958, and #6957 were merged earlier in the month and have had a longer time to sit in master.

I am a little worried that #7069 and the related changes I have queued up for this branch are cutting it too close. One option might be to merge this PR without that change and add it for the next release. #6934 (comment) has more context about the problem. @pjcozzi would this be an acceptable approach?

[pjcozzi]

I am a little worried that #7069 and the related changes I have queued up for this branch are cutting it too close. One option might be to merge this PR without that change and add it for the next release. #6934 (comment) has more context about the problem. @pjcozzi would this be an acceptable approach?

Could we at least attempt to review and test that PR? If I am following that is needed so Cesium doesn't render the scene twice for the pick for the common clamping case , right? Feels pretty important and something we knew to address awhile ago.

[mramato]

Could we at least attempt to review and test that PR?

It's already been reviewed/tested/merged, I didn't share @lilleyse's reservation about the change (though I'm the one that originally recommended so I'm clearly biased).

[mramato]

@lilleyse please bump when this is ready for full testing. (I assume that will be today?)

[lilleyse]

Thought dump because I'm at bit of an impasse for getting the excluded objects feature to be robust and run in a single render.

Technicals aside, there's a major design problem with the approach. objectsToExclude can be an array of anything, entities or primitives, but it is passed to a function in Scene which shouldn't have any association with the entity API.

Then the thinking is, offload that logic to the app instead. That was the original approach I tried but it is cumbersome:

scene.postRender.addEventListener(function() {
    entity.show = false;
    var height = scene.sampleHeight(cartographic);
    entity.show = true;
});

This only works now that #7069 is merged. If #7069 is reverted I don't think it's possible to chain (pre/post)(Render/Update) events to replicate this behavior if sampleHeight is called every frame. The code would have to look like:

scene.postRender.addEventListener(function() {
    entity.show = false;
    viewer._dataSourceDisplay.update(viewer.clock.currentTime);
    var height = scene.sampleHeight(cartographic);
    entity.show = true;
});

Which looks pretty bad.

The reasons for reverting #7069 are:

• Breaks drill picking Drill picking completely broken #7081. (Though there is an easy fix for that).
• If an entity calls sampleHeight or clampToHeight as part of its CallbackProperty it will trigger an infinite loop. I feel like this will be a common use case.

Some alternative approaches that we might need to take:

• Make sampleHeight and clampToHeight operate as if they are drill picks and return multiple results. The app is responsible for weeding out results for objects they want to exlude. This is most inline with the design behind pick and drillPick, which don't take an objectsToExclude array either. In the common case where you are clamping an object to a surface this will incur two or more scene draws.
• Let sampleHeight and clampToHeight take an objectsToExclude array and use drill pick internally (basically this branch reverted back a few commits). Accept that this is probably a bad design decision and that it still incurs two scene draws in the common case, but at least is user friendly.
• Add sampleHeight and clampToHeight functions to Viewer and take an objectsToExclude array. Since Viewer has access to entities it can do the show/hide updates internally and avoid all of the issues here. It would render the scene only once.

The last option is the most user friendly and efficient, but is it bad practice to be adding functions like this to Viewer?

[pjcozzi]

Sorry I don't have the time to digest all of the above, but just some drive-by comments:

• Even though Scene doesn't know about entities, it can certainty set a show property for anything, likewise it could know about any new general interface that could be defined at the Scene level that entities could implement
• I would still want much of the implementation to live in Scene, but from the user's perspective I thought the trend was to move more functions to Viewer so there is less indirection for the end user. There could be an issue about that somewhere.

[lilleyse]

I reverted back to the older (slower) approach. I don't think I'll have a good fix in time, but with the hope of improving this later here are some of the approaches I tried:

• Showing/hiding entities in Viewer or Scene. I couldn't figure out how to avoid the double update problem if something calls sampleHeight from within its callback property.
• Get the list of primitives associated with an entity and hide them in Scene. This worked for simple things like models but got much more complicated for geometry instances, points, and billboards.
• Filter draw commands. .owner usually doesn't link back to the entity.

[lilleyse]

I talked with @pjcozzi offline about the approach, and he's ok with merging this now. I'm going to open an issue to address the performance concerns.

Otherwise I addressed all of @pjcozzi's comments. @bagnell could you do a final pass over the code?

[bagnell]

Looks good to me.

[cesium-concierge]

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/d/msg/cesium-dev/XdogF4bIbNQ/9WdgDqs9BwAJ
https://groups.google.com/d/msg/cesium-dev/4NldMgzFfGU/BBMghq1HBgAJ
https://groups.google.com/d/msg/cesium-dev/apwGMLySnoY/ab-GTjIRBwAJ

If this issue affects any of these threads, please post a comment like the following:

The issue at #6934 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

---

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

🌍 🌎 🌏