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

Various improvements related to height queries against an ellipsoid #1000

Merged
merged 9 commits into from
Nov 21, 2024

Conversation

kring
Copy link
Member

@kring kring commented Nov 19, 2024

  • Don't let EllipsoidTilesetLoader create tiles past level 30, because the integers used for our QuadtreeTileID will overflow at level 31.
  • Tightened the tolerance of IntersectionTests::rayTriangleParametric, allowing it to find intersections with smaller triangles (such as the ones generated by EllipsoidTilesetLoader for a level 30 tile).
  • Fixed a bug that could cause GltfUtilities::intersectRayGltfModel to crash when the model contains a primitive whose position accessor does not have min/max values (such as the ones created by EllipsoidTilesetLoader).
  • Added new ITilesetHeightQuery interface. It may be optionally implemented by a TilesetContentLoader in order to provide a more efficient way to query heights. For example, EllipsoidTilesetLoader simply returns 0.0. Or, a tileset that has a corresponding web service to query heights could use that, instead of downloading and sampling tiles.

Fixes #994

EllipsoidTilesetLoader can conceptually create tiles of any level. But
because we use a uint32_t for the tile X and Y coordinates, and because
tile level 31 has 2*2^31=2^32 tiles, we end up overflowing the uint32_t
and producing nonsense tile coordinates. So this change prevents the
loader from creating tiles past level 30.
EllipsoidTilesetLoader was passing a size to the constructor for the
std::vector used to store the indices, and then it was also adding the
indices by calling `insert`. That resulted in a giant block of useless
(0,0,0) indices, followed by the useful ones created by the insert.

This change uses `reserve` instead of passing a size to the constructor.
IntersectionTests::rayTriangleParametric was returning "no intersection"
when `det < Math::Epsilon6`. This was causing it to miss intersections
in very small tiles. So I changed it to `Epsilon8`.
The min and max properties of a glTF accessor are optional. But
`intersectRayScenePrimitive` in `GltfUtilities` was assuming the
primitive's position accessor had them, and it would crash if they did
not.
@kring kring changed the title Various improvements related to height query against an ellipsoid Various improvements related to height queries against an ellipsoid Nov 19, 2024
@j9liu j9liu self-requested a review November 19, 2024 14:16
@kring kring added this to the December 2024 Release milestone Nov 20, 2024
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 great to me! I'll give this a try in Unity and let you know how it goes

I just had one nitpick about naming. It doesn't have to hold up the PR but I feel like it would be a nice change re: API design

@j9liu
Copy link
Contributor

j9liu commented Nov 21, 2024

Thanks @kring ! Just tried this out in Unity and confirmed that sampling ellipsoid heights no longer causes a crash. Merging!

@j9liu j9liu merged commit 9571f32 into main Nov 21, 2024
20 checks passed
@j9liu j9liu deleted the ellipsoid-height-queries branch November 21, 2024 19:26
kring added a commit that referenced this pull request Nov 21, 2024
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.

SampleHeightMostDetailed crashes for procedural ellipsoid tileset
2 participants