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

3D Tileset getHeight, and keeping camera above 3D Tiles #11581

Merged
merged 31 commits into from
Jan 10, 2024
Merged

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Oct 23, 2023

Fixes #11288, and sets the groundwork for #7044.

This PR includes 3D Tiles in camera "collision", which previously applied only to terrain, to 3D Tiles as well. It does not seek to refine or significantly alter how the camera collision itself works. Rather, this extends what is already in place for terrain to work with 3D Tiles.

It implements a getHeight function for 3D tilesets which copies tile data to the CPU for testing intersections, similar to the approach we take for terrain, intended for internal use such as the camera collision, and in a follow up PR clamping to 3D Tiles.

I also exposed a private pick function on 3D tilesets and on model primitives. This is an implementation detail of getHeight, but is useful for testing and internal development purposes. An example of how it works for tilesets is available in the development 3D Tiles Picking sandcastle example.

By default, since viewers default to using WebGL 2, the picking implementation will require WebGL 2 to copy buffer data at runtime. To be compatible with WebGL 1, we take an approach similar to what we do for 2D projections of models and 3D Tiles where we allow a tileset option, enablePick, that keeps a copy of the loaded data on the CPU in a typed array. This is not enabled by default because it's an additional additional memory overhead.

The behavior is enabled by default for all tilesets, but the disableCameraCollision options for a tileset can be used to turn it off. The biggest reason for this would be a case where the user is navigating inside of a 3D tileset, as illustrated in the "3D Tiles Interior" sandcastle example.

Testing Plan

  • Try moving camera on & under terrain to make sure terrain camera control haven't regressed
  • Try moving camera on & under 3D Tileset to make sure the camera controls are consistant with terrain. Sandcastles to check in particular:
    • "Google Photorealistic 3D Tiles"
    • "3D Tiles Next S2 Globe"
  • Ensure you can turn off the behavior for indoor tilesets, see the "3D Tiles Interior" Sandcastle
  • Confirm nothing crashes when switching between 2D/3D/CV
  • Confirm nothing crashes when using a WebGL 1 context

TODO:

  • Debug sporadic intersection failures
  • Confirm 2D/Columbus view
  • Instanced models
  • Unit tests
  • Update CHANGES.md

@cesium-concierge
Copy link

Thanks for the pull request @ggetz!

  • ❕ There was an error checking the CLA! If this is your first contribution, please send in a Contributor License Agreement.
    • Maintainers, this was the error I ran into while attempting to process the CLA check. Please resolve it to continue CLA checking.
    • You'll need to manually check for a submitted CLA
    Error: The service is currently unavailable.
    
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ggetz ggetz changed the title 3D Tileset `getHeight, and keeping camera above 3D Tiles 3D Tileset getHeight, and keeping camera above 3D Tiles Oct 23, 2023
@ggetz ggetz mentioned this pull request Nov 3, 2023
4 tasks
@ggetz ggetz marked this pull request as ready for review November 9, 2023 19:47
Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Hi @ggetz, I just have 2 preliminary comments for now--one minor, one about the API.

@jjhembd jjhembd mentioned this pull request Nov 30, 2023
3 tasks
@ggetz
Copy link
Contributor Author

ggetz commented Dec 6, 2023

Thanks @jjhembd! I've added a bullet to incorporate exaggeration once #11655 is merged. Other than than, this is update and ready to go after that change.

@ggetz ggetz mentioned this pull request Dec 12, 2023
6 tasks
@ggetz
Copy link
Contributor Author

ggetz commented Dec 14, 2023

@jjhembd In the interest of moving things along, I've omitted incorporating 3D Tiles Exaggeration in this PR. Instead, you should ensure that the pick function factors in the exaggeration values you've added in your PR.

@jjspace Could you please take a look and review this PR? It's already been reviewed for the overall approach, but still needs a final pass on the code and on testing.

@ggetz
Copy link
Contributor Author

ggetz commented Jan 4, 2024

@jjhembd I've added support for vertical exaggeration to this feature. Could you please re-review?

@ggetz
Copy link
Contributor Author

ggetz commented Jan 8, 2024

Hi @jjhembd, just reminder to review.

Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Thanks @ggetz, I went through all the code and left a few minor notes.
I still need to run the tests locally and try out the Sandcastle.

packages/engine/Source/Scene/Model/pickModel.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Cesium3DTilesetTraversal.js Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@jjhembd
Copy link
Contributor

jjhembd commented Jan 9, 2024

As a follow-up on the remaining items from my review:

  • Specs run successfully on my machine (including the new pickModelSpec)
  • New picking Sandcastle works (mostly) as expected

I noticed one strange behavior when picking on the "Bentley BIM" model. Some parts of the tileset are picked by the Scene.pickPosition call, but not by the tileset.pick call. See the below screenshot, where I enlarged the red circles from the first call to make the difference more clear. (Here is the modified local Sandcastle)

image

After I move the camera slightly, both pick calls work well on the problematic feature. However, after moving the camera, some of the reported properties are missing--I no longer see the "assembly" and "category" properties in the pop-up.

@ggetz
Copy link
Contributor Author

ggetz commented Jan 9, 2024

I noticed one strange behavior when picking on the "Bentley BIM" model. Some parts of the tileset are picked by the Scene.pickPosition call, but not by the tileset.pick call. See the below screenshot, where I enlarged the red circles from the first call to make the difference more clear.

Interesting. I can reproduce at a very specific camera angle and zoom level (that can be difficult to replicate). From what I can tell, these are instanced parts of the model. However, the instanced node are quite similar to what is being tested in the instanced example.

I also cannot find another tileset with similar issues.

Finally, it is not returning an incorrect position, but rather undefined, meaning it should not interfere with camera collisions.

However, after moving the camera, some of the reported properties are missing--I no longer see the "assembly" and "category" properties in the pop-up.

I don't think this is altered behavior in this PR. I can replicate in the last official release.

I'm thinking this may be unique to the Bentley powerplant tileset. @jjhembd Let me know if you agree and if this is is a good place.

@jjhembd
Copy link
Contributor

jjhembd commented Jan 10, 2024

I'm thinking this may be unique to the Bentley powerplant tileset. @jjhembd Let me know if you agree and if this is is a good place.

Yes, I agree those minor quirks don't look like they are related to this PR. I wonder if there might be an existing issue in camera.getPickRay that affects close zooms.

I have one question about a doc, and there are a couple failing specs. But other than that, this should be ready to go!

@ggetz
Copy link
Contributor Author

ggetz commented Jan 10, 2024

Thanks @jjhembd! This should be ready.

Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @ggetz !

@jjhembd jjhembd merged commit 2602d03 into main Jan 10, 2024
9 checks passed
@jjhembd jjhembd deleted the tileset-get-height branch January 10, 2024 17:00
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.

Keep camera from going under 3D tiles
3 participants