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

Clamping to 3D Tiles #6934

Merged
merged 42 commits into from Sep 28, 2018

Conversation

Projects
None yet
7 participants
@lilleyse
Contributor

lilleyse commented Aug 17, 2018

steps

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

This comment has been minimized.

Show comment
Hide comment
@cesium-concierge

cesium-concierge Aug 17, 2018

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.

🌍 🌎 🌏

cesium-concierge commented Aug 17, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Aug 22, 2018

Contributor

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...

Contributor

lilleyse commented Aug 22, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Aug 27, 2018

Contributor

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

Contributor

lilleyse commented Aug 27, 2018

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

@lilleyse lilleyse referenced this pull request Sep 7, 2018

Closed

Clamp to a 3D Tileset #6080

@@ -0,0 +1,130 @@
<!DOCTYPE html>

This comment has been minimized.

@pjcozzi

pjcozzi Sep 10, 2018

Member

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

@pjcozzi

pjcozzi Sep 10, 2018

Member

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

Show outdated Hide outdated Apps/Sandcastle/gallery/Clamp to 3D Tiles.html Outdated
Show outdated Hide outdated Apps/Sandcastle/gallery/Clamp to 3D Tiles.html Outdated
Show outdated Hide outdated Source/Scene/Scene.js Outdated
Show outdated Hide outdated Source/Scene/Scene.js Outdated
Show outdated Hide outdated Source/Scene/Scene.js Outdated
@OmarShehata

This comment has been minimized.

Show comment
Hide comment
@OmarShehata

OmarShehata Sep 11, 2018

Contributor

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.

Contributor

OmarShehata commented Sep 11, 2018

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

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Sep 12, 2018

Member

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

Member

pjcozzi commented Sep 12, 2018

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

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Sep 15, 2018

Member

@lilleyse is this ready for another review?

Member

pjcozzi commented Sep 15, 2018

@lilleyse is this ready for another review?

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 15, 2018

Contributor

@lilleyse is this ready for another review?

Yup, just finished this up.

The pick pass is writing translucent depth like in #7039 so don't merge this PR until that one is resolved. It seems to be a tradeoff of bugs but I think writing depth is the right move.

Contributor

lilleyse commented Sep 15, 2018

@lilleyse is this ready for another review?

Yup, just finished this up.

The pick pass is writing translucent depth like in #7039 so don't merge this PR until that one is resolved. It seems to be a tradeoff of bugs but I think writing depth is the right move.

@hpinkos

This comment has been minimized.

Show comment
Hide comment
@hpinkos

hpinkos Sep 25, 2018

Contributor

@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?

Contributor

hpinkos commented Sep 25, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@mramato

mramato Sep 25, 2018

Member

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.

Member

mramato commented Sep 25, 2018

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 added some commits Sep 25, 2018

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 26, 2018

Contributor

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?

Contributor

lilleyse commented Sep 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Sep 26, 2018

Member

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.

Member

pjcozzi commented Sep 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@mramato

mramato Sep 26, 2018

Member

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).

Member

mramato commented Sep 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@mramato

mramato Sep 26, 2018

Member

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

Member

mramato commented Sep 26, 2018

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

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 26, 2018

Contributor

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 #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?

Contributor

lilleyse commented Sep 26, 2018

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 #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

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Sep 27, 2018

Member

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.
Member

pjcozzi commented Sep 27, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 28, 2018

Contributor

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.
Contributor

lilleyse commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 28, 2018

Contributor

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?

Contributor

lilleyse commented Sep 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Sep 28, 2018

Member

Looks good to me.

Member

bagnell commented Sep 28, 2018

Looks good to me.

@bagnell bagnell merged commit d7b8104 into master Sep 28, 2018

5 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deployment Deployed
Details
npm package Deployed
Details
zip file Deployed
Details
@cesium-concierge

This comment has been minimized.

Show comment
Hide comment
@cesium-concierge

cesium-concierge Sep 28, 2018

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.

🌍 🌎 🌏

cesium-concierge commented Sep 28, 2018

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.

🌍 🌎 🌏

@bagnell bagnell deleted the pick-ray-2 branch Sep 28, 2018

@lilleyse lilleyse referenced this pull request Oct 3, 2018

Open

Clamp to 3D Tiles most detailed #7115

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment