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

Fixed tile triangle picking #8390

Merged
merged 3 commits into from
Dec 19, 2019
Merged

Fixed tile triangle picking #8390

merged 3 commits into from
Dec 19, 2019

Conversation

IanLilleyT
Copy link
Contributor

The old tile picking code returned the first occurring triangle intersection, but it should be finding the closest triangle intersection. This PR fixes that.

Sandcastle

Click at arrow:
Screen Shot 2019-11-13 at 3 51 51 PM
Old behavior:
Screen Shot 2019-11-13 at 3 51 13 PM
New behavior:
Screen Shot 2019-11-13 at 3 51 40 PM

@cesium-concierge
Copy link

Thanks for the pull request @IanLilleyT!

  • ✔️ Signed CLA found.
  • 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.
  • Works (or fails gracefully) in IE11.

@lilleyse
Copy link
Contributor

The code looks good. Can you add an entry to CHANGES.md and add a test. For the test maybe model it after https://github.com/AnalyticalGraphicsInc/cesium/blob/7dd9bbb45dc2475eed6316124ee24034a088a29f/Specs/Scene/GlobeSurfaceTileSpec.js#L286

@IanLilleyT
Copy link
Contributor Author

@lilleyse ready for review

@lilleyse
Copy link
Contributor

lilleyse commented Dec 19, 2019

I did some performance testing to make sure this change didn't introduce too much slowness. Not the most scientific test, I just clicked around a bunch of times and averaged the result. Both tests were on built versions of Cesium.

This branch Sandcastle

10.650000000168802
2.1549999983108137
1.6600000017206185
1.3450000005832408
3.1449999987671617
6.38500000059139
3.280000000813743
13.504999998986023
14.585000000806758
7.570000001578592
3.5449999995762482
2.639999998791609
2.7300000001559965
23.05500000147731
8.679999999003485
3.1449999987671617
2.40000000121654
3.3000000003085006
21.26499999940279
9.569999998348067

Average: 7.2304999999687

Master Sandcastle

5.420000001322478
12.865000000601867
10.935000002064044
7.324999998672865
1.524999999674037
1.5050000001792796
1.069999998435378
5.430000001069857
11.41500000085216
6.965000000491273
2.8450000027078204
15.204999999696156
12.47999999759486
14.244999998481944
12.930000000778819
3.3149999981105793
4.354999997303821
2.870000000257278
0.9299999983340967
1.4300000002549496

Average: 6.7529999998442

So as expected it's a bit slower, about 1 ms on average for Cesium World Terrain, but it's also accurate now. @IanLilleyT is going to open a separate issue for general picking slowness, especially with detailed terrain like ArcGISTiledElevationTerrainProvider.

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