Skip to content
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

Fix 3D Tiles depth picking #5770

Merged
merged 12 commits into from
Aug 29, 2017
Merged

Fix 3D Tiles depth picking #5770

merged 12 commits into from
Aug 29, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Aug 23, 2017

Fixes #5676, #5731, and #5683.

  • Fixes picking depth on 3D Tiles.
  • Adds classificationType property to GroundPrimitive and ClassificationPrimitive which specifies whether to classify terrain, 3D Tiles, or both.

@pjcozzi pjcozzi requested a review from lilleyse August 23, 2017 23:42
@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 23, 2017

Looks OK to me at quick glance. Just one request: can you add - or update - a Sandcastle example that shows the differences between the different enum types, e.g., a polyline going over only terrain, only a tileset, or both.

@lilleyse can you review and verify each issue this fixed, and merge when ready? Thanks!

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and I confirmed the issues are fixed.

* @type {Number}
* @constant
*/
CESIUM_3D_TILES : 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency should this be renamed to CESIUM_3D_TILE to match Pass.CESIUM_3D_TILE and Pass.CESIUM_3D_TILE_CLASSIFICATION?

@bagnell
Copy link
Contributor Author

bagnell commented Aug 24, 2017

@lilleyse This is ready for another look.

@lilleyse
Copy link
Contributor

Could the rectangle in the demo be smaller so it doesn't cover the entire tileset? Maybe just the blue area:

area

@bagnell
Copy link
Contributor Author

bagnell commented Aug 28, 2017

@lilleyse This is ready.

@bagnell
Copy link
Contributor Author

bagnell commented Aug 28, 2017

@lilleyse Updated after our offline discussion.

@lilleyse
Copy link
Contributor

Works well now.

@pjcozzi instead of offsetting the depth plane we now just render the depth plane after the 3D Tiles pass. A tileset on the opposite side of the globe would probably not be rendered at all due to SSE. Can you think of possible edge cases?

@lilleyse
Copy link
Contributor

I'm going to merge, but if anyone has issues with the approach we can go back to offsetting the depth plane.

@lilleyse lilleyse merged commit 41827bb into master Aug 29, 2017
@lilleyse lilleyse deleted the 3d-tiles-depth branch August 29, 2017 19:18
@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 31, 2017

@pjcozzi instead of offsetting the depth plane we now just render the depth plane after the 3D Tiles pass. A tileset on the opposite side of the globe would probably not be rendered at all due to SSE. Can you think of possible edge cases?

Doesn't this mean that most apps will need to enable depthTestAgainstTerrain?

@lilleyse
Copy link
Contributor

They shouldn't need to. The main side effect with moving the depth plane after 3D Tiles classification was that tilesets might show through the opposite side of the globe when depthTestAgainstTerrain is false. But we figured that a tileset on the opposite side side would not get rendered at all because its root would either not be visible or would not meet SSE.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 31, 2017

Maybe this isn't a common case, but what if I have a label or a 3D model in drawn in the area of a 3D tileset, then I take a 45 degree horizon or top down view, wouldn't the depth values of the tileset be gone and the label/model would shown in front?

@lilleyse
Copy link
Contributor

I'm pretty sure that case is fixed with #5789. Now the depth plane doesn't overwrite the 3D Tiles depth.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 31, 2017

OK, sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants