-
Notifications
You must be signed in to change notification settings - Fork 3.7k
2d picking fixes #13098
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
base: main
Are you sure you want to change the base?
2d picking fixes #13098
Conversation
|
Thank you for the pull request, @mzschwartz5! ✅ We can confirm we have a CLA on file for you. |
7216a00 to
8b877ba
Compare
jjhembd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mzschwartz5, this is a huge improvement. Thanks for the testing Sandcastle, and the in-depth discussion in #13083.
I only have a comment about naming, and a question about loop performance.
| for (let i = 0, len = tilesRendered.length; i < len; ++i) { | ||
| tileFunction(tilesRendered[i]); | ||
| const tilesRendered = this._tilesRenderedThisFrame; | ||
| for (const tile of tilesRendered) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, how many tiles are in this list? If it's a "small" number, then for..of is probably ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another instance where I don't think it's a hot path for performance, but it's an easy change to make, so might as well change it.
packages/engine/Source/Workers/incrementallyBuildTerrainPicker.js
Outdated
Show resolved
Hide resolved
packages/engine/Source/Workers/incrementallyBuildTerrainPicker.js
Outdated
Show resolved
Hide resolved
| GeographicProjection, | ||
| } from "../../index.js"; | ||
|
|
||
| describe("Core/TerrainMeshSpec", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always good to see more specs!
ggetz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the code is looking great. Thanks @mzschwartz5 for tracking down all the fixes here! I just have one question regarding precision. Otherwise this all looks good to me.
|
|
||
| // Right near the antimeridian, in 2D/CV modes, the vertex positions may correspond to the other side of the world from the ray origin. | ||
| // Compare to the ray origin and adjust accordingly. | ||
| const worldWidth = CesiumMath.TWO_PI * projection.ellipsoid.maximumRadius; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to use a spherical approximation and not the true ellipsoid circumference. Is that adequate for the use case here, or should the true circumference be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.... I need to think about this. It does seem to "work" in all my testing but is it correct?
Perhaps the reason it's valid (at least in the testing I've done) is because our two common projections (mercator and web geographic) are cylindrical projection, where this math is exact.
It might not be valid for other projections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, conveniently those are the only two 2D projections we support. 😄 So maybe good enough, and just worth a comment explaining why.
If in doubt, using a root mean square approximation should be accurate enough for most real world applications since the radii are generally close in value. But that could be overkill given the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the issue is that MapProjection is an interface technically, so people could in theory extend it with their own projection implementations that may not be cylindrical.
In practice this hasn't happened in 15+ years so... (there are plenty of places that assume our two projections are the ONLY projections, for better or for worse).
Will leave a comment though.
| [], | ||
| ); | ||
|
|
||
| // Mock out the dependency on TerrainPicker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious—What's the motivation on mocking this? Is the TerrainPicker difficult to construct or is there another source of friction?
Generally speaking, it'd preferable to call through this function call, even if you are spying on it. (Though this doesn't need to prevent this PR from being merged—Just adding the unit test is already an improvement.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my thought process was that these tests aren't supposed to be testing the terrain picker ray intersection logic, they're testing TerrainMesh.
Otherwise it's more of an integration test than a unit test. For what it's worth, it is also difficult to construct TerrainPicker - fully constructing things would require having real mesh data to give it. Once you start down that rabbit hole, where do you stop?
| ); | ||
|
|
||
| mesh.updateExaggeration(2.0, 1.0); | ||
| const expected2DTransform = mesh.getTransform( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While calculating a value rather than hardcoding it can make tests more resilient, in this case the function computing the expected value is part of the system under test. Depending on it may lead to cases where there's a bug somewhere in TerrainMesh but this unit test still passes. I'd opt to hard code it, or compute it outside of TerrainMesh operations.
(Again, this doesn't need to prevent this PR from being merged—Just adding the unit test is already an improvement.)
Description
There are various issues with terrain picking in 2D/CV scene modes. Some of these have existed for a very long time, and some are recent regressions from #9961.
All of them manifest as
undefinedpick results when performingglobe.pick. This PR addresses nearly all of the issues that manifest this way. They're all documented here, but tl;dr-QuadtreePrimitiveonly tracks the tiles rendered in a given render call.camera.pickRay()does not account for that wrap.TerrainPickerregression 1:globe.getHeight()picks in 3D mode, even when the scene mode is 2D/CV. This causes theTerrainPickerto cache and use a stale transform for the incorrect scene mode.TerrainPickerregression 2: vertex positions were passed to workers as 32 bit floats. This seems to have caused some precision issues in 2D mode (maybe in 3D mode too?), so I switched toFloat64Array.Unfixed issues:
TerrainPicker2D mode are not good. I don't think the mock terrain provider can even provide data in the 2D coordinate system, but it would be a lot of work to learn how to enhance it.Issue number and link
Primary issue
Also came up here
Testing plan
I've been using this sandcastle to test: it picks as you move the mouse, and draws a point wherever there results are undefined.
After these changes, it should mostly draw nothing (except near the poles)!
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change