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

Add height-query function to Tileset #783

Open
wants to merge 82 commits into
base: main
Choose a base branch
from
Open

Add height-query function to Tileset #783

wants to merge 82 commits into from

Conversation

joseph-kaile
Copy link
Collaborator

@joseph-kaile joseph-kaile commented Jan 2, 2024

Adds new functionality to Tileset to enable terrain height queries.

Builds on previous cesium-native work for ray / triangle and ray / gltf intersections.

Approach

To find the height at a latitude / longitude, we must first load the terrain data for this location, construct a ray at that coordinate pointing towards the center of the earth, then intersect all the content in the tile, using the closest hit as our discovered height.

Height queries can happen for tiles not selected from a camera, so we introduce a parallel tile loading strategy that builds on the same system. Just as we use a camera frustum for _visitTileIfNeeded, we use our new height query requests and call visitHeightRequests. This discovers which tiles are needed to load and adds them to the same priority queues that the camera-based tiles go to.

Once all relevant tiles are loaded, we ray intersect against all of their contents.

Fixes #636

TO DO

  • Solve priority questions, how does it co-exist with camera tile loads?
  • Reuse addTileToLoadQueue for tiles added from query traversal
  • Performance - How do we know how much time is loading vs. intersecting? Intersect code should be minimal compared to url content requests. How can we verify this?
  • Performance - Are we wasting time creating physics meshes on the game engine side? (Ex. Unreal).
  • Fix incremental findCandidateTiles for additive-refined tilesets.
  • Support for implicit tilesets
  • Support for instanced models Wrote an issue: Add support for height queries against instanced models #948
  • Return all heights, not just the highest Not urgent at this time.
  • Write unit tests
  • Peer review
  • Merge

@joseph-kaile joseph-kaile changed the base branch from main to gltf-ray-intersect January 2, 2024 20:05
@kring
Copy link
Member

kring commented Sep 16, 2024

Some quick performance numbers, using the SingleQuery performance test in https://github.com/CesiumGS/cesium-unreal/tree/terrain-query-test, which queries heights for a 20x20 grid of points in the Denver Hills with 0.001 degrees between each row and column in the grid.

Load: Time to load all of the necessary tiles for the query, starting with a cold cache.
Compute: Time to find the intersections of the rays with the triangles in the loaded tiles.

Tileset Load Compute
Cesium World Terrain 2700ms 30 ms
Google Photorealistic 3D Tiles 9200 ms 20 ms

Currently the second part, finding the intersections, happens in the main thread, which isn't ideal. There's definitely some potential for frame rate hitches there.

I considered moving the computation to a work thread, but ultimately I think it's fast enough that we don't need to do that urgently. Moving it to a worker is mostly straightforward, except we need to make sure the glTF isn't unloaded or modified while the background computation is operating on it. And it's also likely to add some latency to the overall computation. Dispatching to the worker is pretty fast, but if we want to raise the "done" notification in the main thread, that has to wait for the next tick. Which means we might be adding 16ms of overhead to a computation that only takes 20ms (or less if you're not querying 400 positions). So I'm not sure it's justified when users should be able to mostly avoid frame rate hitches just by doing their queries in smaller batches, if necessary.

@kring kring marked this pull request as ready for review September 16, 2024 10:31
@kring
Copy link
Member

kring commented Sep 16, 2024

I believe this is now ready to merge. However, because I've now done a fair bit of work on it, it's probably a good idea to get someone else's eyes on it first.

@kring kring added this to the October 2024 Release milestone Sep 16, 2024
@j9liu j9liu self-requested a review September 19, 2024 14:25
Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

@kring this looks good to me! I didn't look over the tests or the algorithm too closely, but I'm sure I can verify correctness when I look at the corresponding Unity / Unreal PRs.

Most of my comments are doc / understanding related. I'm pushing a commit that addresses some of these, but I wanted to leave some up for input. But none of these have to hold up the PR before the release!

Cesium3DTilesSelection/src/Tileset.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/TilesetContentManager.cpp Outdated Show resolved Hide resolved
* {@link SampleHeightResult::positions} is unchanged from the original input
* height.
*/
std::vector<bool> heightSampled;
Copy link
Contributor

Choose a reason for hiding this comment

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

When I read heightSampled my brain first thinks "oh, this is the height that was sampled," instead of "this was whether the sample actually happened." What do you think of this alternative?

Suggested change
std::vector<bool> heightSampled;
std::vector<bool> sampleSuccessful;

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I kind of prefer sampleSuccess more, since that's briefer.
Other suggestions I could think of are

  • successfulSample
  • successfullySampled
  • heightWasSampled
  • wasHeightChanged / heightWasChanged.

Cesium3DTilesSelection/src/TilesetHeightQuery.h Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/TilesetHeightQuery.h Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/TilesetHeightQuery.h Outdated Show resolved Hide resolved
Comment on lines 16 to 22
* @brief The positions and sampled heights.
*
* The longitudes and latitudes will match the values at the same index in the
* original input positions. Each height will either be the height sampled
* from the tileset at that position, or the original input height if the
* height could not be sampled. To determine which, look at the value of
* {@link SampleHeightResult::heightSampled} at the same index.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief The positions and sampled heights.
*
* The longitudes and latitudes will match the values at the same index in the
* original input positions. Each height will either be the height sampled
* from the tileset at that position, or the original input height if the
* height could not be sampled. To determine which, look at the value of
* {@link SampleHeightResult::heightSampled} at the same index.
* @brief The positions and their sampled heights.
*
* For each resulting position, its longitude and latitude values will match
* values from its input. Its height will either be the height sampled from
* the tileset at that position, or the original input height if the sample
* was unsuccessful. To determine which, look at the value of
* {@link SampleHeightResult::sampleSuccessful} at the same index.

@j9liu
Copy link
Contributor

j9liu commented Sep 26, 2024

One thing I noticed while testing CesiumGS/cesium-unreal#1421: if the tileset is invalid (e.g., invalid ion asset ID), it never completes the async action, and the "Height requests could not complete because the tileset failed to load." message never gets printed. It doesn't have to hold up this PR, since we're low on time, but I think we should address it shortly after, since it will be confusing to the user to not have feedback.

@kring
Copy link
Member

kring commented Sep 27, 2024

if the tileset is invalid (e.g., invalid ion asset ID), it never completes the async action, and the "Height requests could not complete because the tileset failed to load." message never gets printed

I believe I fixed this in this commit, but cesium-unreal wasn't updated to use it yet:
4850751

I'll write a test in cesium-native for this case, though.

@kring
Copy link
Member

kring commented Sep 27, 2024

Thanks for the review @j9liu, this should be ready for another look.

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.

Add a function to compute the terrain height at a point
6 participants