improve sanity check for geodesic distance calc #7457

nmschulte · 2019-01-03T06:02:33Z

https://groups.google.com/forum/#!topic/cesium-dev/uTaQKBSzxCU

Based upon the discussion linked above, I think this is a small improvement to the current logic. The commit message has the details and this description has more: https://en.wikipedia.org/wiki/Geodesics_on_an_ellipsoid#Behavior_of_geodesics

(commit message)

guard Vincenty's inverse computation (surface distance) by the cut locus specific to the ellipsoid (assumed as oblate given min/max radii)

(Wikipedia extract; emphasis mine)

Gauss (1828) showed that, on any surface, geodesics and geodesic circle intersect at right angles. The red line is the cut locus, the locus of points which have multiple (two in this case) shortest geodesics from A. On a sphere, the cut locus is a point. On an oblate ellipsoid (shown here), it is a segment of the circle of latitude centered on the point antipodal to A, φ = −φ₁. The longitudinal extent of cut locus is approximately λ₁₂ ∈ [π − f π cosφ₁, π + f π cosφ₁]. If A lies on the equator, φ₁ = 0, this relation is exact and as a consequence the equator is only a shortest geodesic if |λ₁₂| ≤ (1 − f)π. For a prolate ellipsoid, the cut locus is a segment of the anti-meridian centered on the point antipodal to A, λ₁₂ = π, and this means that meridional geodesics stop being shortest paths before the antipodal point is reached.

guard Vincenty's inverse computation (surface distance) by the cut locus specific to the ellipsoid (assumed as oblate given min/max radii)

cesium-concierge · 2019-01-03T06:02:34Z

Thanks for the pull request @nmschulte!

✔️ 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.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

nmschulte · 2019-01-03T06:04:33Z

Source/Core/EllipsoidGeodesic.js

@@ -171,14 +171,18 @@ define([
        ellipsoidGeodesic._uSquared = uSquared;
    }

+    //>>includeStart('debug', pragmas.debug);


I wonder about these scratch variables:
Why do these scratch variables live outside the only scope they're used?
Why use scratch variables like this?

While debugging (the web worker) I ran into a case where the "subdivide heights" scratch array had a zero-length even though the code that was being debugged had presumably just initialized it... I reproduced it once after a refresh (left the debugger open). I've noticed strange behavior while debugging web workers with Chrome, so it's possible it's an artifact of that and not an issue with the logic (shared scratch) and some kind of race condition (between worker/worker or worker/main -- ... apologies I'm not well versed here).

The scratch variables are at this scope so they can be reused each time the function is called instead of creating new instances each time. This is something we do commonly for performance. See https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#result-parameters-and-scratch-variables

So you'll need to remove the pragmas here

Same here with adding the pragma block: these scratch variables are only used for the debug check.

nmschulte · 2019-01-03T06:22:00Z

Digging a bit more this idea may be misguided, even though Cesium seems to render things fine for my case. I think the tolerance here is due to the nature of the approximation of the integral solution of Vincenty's formula, not necessarily the (maximum) locus length. Please correct me if so.

hpinkos · 2019-01-03T14:19:00Z

Source/Core/EllipsoidGeodesic.js

+        var includedAngle = Math.abs(Math.abs(Cartesian3.angleBetween(startCartesian, endCartesian)) - CesiumMath.PI);
+        // maxLambda is an approximation, assuming an oblate ellipsoid, and zero latitude (equatorial; phi = 0)
+        var maxLambda = (1 - (ellipsoid.maximumRadius - ellipsoid.minimumRadius) / ellipsoid.maximumRadius) * CesiumMath.PI;
+        Check.typeOf.number.lessThanOrEquals('value', includedAngle, maxLambda);


This Check call should be the only thing inside the debug pragmas here. Everything between //>>includeStart and //>>includeEnd will be removed from the code in the minified release version, so you don't want to have any logic included in that section. It's only for throwing DeveloperErrors

I moved that logic into the pragma block because it is only used for the check.

hpinkos · 2019-01-03T14:22:15Z

@shunter did you want to take a look at this?

cesium-concierge · 2019-02-03T03:00:11Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos · 2019-02-04T14:33:58Z

@shehzan10 could you give this a review?

shehzan10 · 2019-02-04T15:07:00Z

Digging a bit more this idea may be misguided, even though Cesium seems to render things fine for my case. I think the tolerance here is due to the nature of the approximation of the integral solution of Vincenty's formula, not necessarily the (maximum) locus length. Please correct me if so.

I think either @shunter or @fstoner have a better handle on this question than I do.

This code looks fine to me. On the default ellipsoid, maxLambda=3.131059488236545
thus the change is going from 0.0125 to 0.010533165353248108.

When I was doing the ellipsoid rhumb line code, I had a similar question about how 0.0125 was chosen, but this PR has a reasoning behind it. It was also brought up (tangentially) here: #7484 (comment).

So the only thing I would request is that this change should be applied to EllipsoidRhumbLine as well https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/EllipsoidRhumbLine.js#L141.

cesium-concierge · 2019-03-07T03:00:10Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos · 2019-03-07T14:25:29Z

@nmschulte can you merge in master? I think these changes look fine, I'll get them merged in this month after the branch is updated. Thanks!

hpinkos · 2019-03-07T14:25:46Z

Please also add a unit test

nmschulte · 2019-03-07T17:34:52Z

@nmschulte can you merge in master? I think these changes look fine, I'll get them merged in this month after the branch is updated. Thanks!

Is it a concern if I rebase these changes instead? I can include refactor/changes to update EllipsoidRhumbLine as well.

Please also add a unit test

I'll try my best ;).

I'm still concerned that my logic connecting locus length to the approximation algorithm is incorrect and including this change will cause the formula to diverge (spin-lock/Bad Things™).

idea may be misguided, even though Cesium seems to render things fine for my case. I think the tolerance here is due to the nature of the approximation of the integral solution of Vincenty's formula, not necessarily the (maximum) locus length.

hpinkos · 2019-03-11T15:08:11Z

@nmschulte yes, a rebase would be fine.

Thanks for pointing out your concern about locus length. I'm actually not familiar with that at all. @fstoner do you have any insight here?

cesium-concierge · 2019-04-11T02:00:16Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2019-05-11T02:00:13Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2019-06-10T02:00:22Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2019-07-10T02:00:15Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2019-08-09T02:00:20Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2019-09-08T02:00:19Z

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos · 2019-09-18T13:54:42Z

Thanks for the PR @nmschulte, but I'm going to close this due to inactivity. This check is probably fine, but it's been difficult to verify and get this merged.

cesium-concierge · 2019-09-18T13:54:45Z

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/uTaQKBSzxCU

If this issue affects any of these threads, please post a comment like the following:

The issue at #7457 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

improve sanity check for geodesic distance calc

447227c

guard Vincenty's inverse computation (surface distance) by the cut locus specific to the ellipsoid (assumed as oblate given min/max radii)

nmschulte commented Jan 3, 2019

View reviewed changes

hpinkos reviewed Jan 3, 2019

View reviewed changes

hpinkos closed this Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve sanity check for geodesic distance calc #7457

improve sanity check for geodesic distance calc #7457

nmschulte commented Jan 3, 2019 •

edited

Loading

cesium-concierge commented Jan 3, 2019

nmschulte Jan 3, 2019

nmschulte Jan 3, 2019

hpinkos Jan 3, 2019

nmschulte Jan 3, 2019

nmschulte commented Jan 3, 2019

hpinkos Jan 3, 2019

nmschulte Jan 3, 2019

hpinkos commented Jan 3, 2019

cesium-concierge commented Feb 3, 2019

hpinkos commented Feb 4, 2019

shehzan10 commented Feb 4, 2019

cesium-concierge commented Mar 7, 2019

hpinkos commented Mar 7, 2019

hpinkos commented Mar 7, 2019

nmschulte commented Mar 7, 2019

hpinkos commented Mar 11, 2019

cesium-concierge commented Apr 11, 2019

cesium-concierge commented May 11, 2019

cesium-concierge commented Jun 10, 2019

cesium-concierge commented Jul 10, 2019

cesium-concierge commented Aug 9, 2019

cesium-concierge commented Sep 8, 2019

hpinkos commented Sep 18, 2019

cesium-concierge commented Sep 18, 2019

improve sanity check for geodesic distance calc #7457

improve sanity check for geodesic distance calc #7457

Conversation

nmschulte commented Jan 3, 2019 • edited Loading

cesium-concierge commented Jan 3, 2019

nmschulte Jan 3, 2019

Choose a reason for hiding this comment

nmschulte Jan 3, 2019

Choose a reason for hiding this comment

hpinkos Jan 3, 2019

Choose a reason for hiding this comment

nmschulte Jan 3, 2019

Choose a reason for hiding this comment

nmschulte commented Jan 3, 2019

hpinkos Jan 3, 2019

Choose a reason for hiding this comment

nmschulte Jan 3, 2019

Choose a reason for hiding this comment

hpinkos commented Jan 3, 2019

cesium-concierge commented Feb 3, 2019

hpinkos commented Feb 4, 2019

shehzan10 commented Feb 4, 2019

cesium-concierge commented Mar 7, 2019

hpinkos commented Mar 7, 2019

hpinkos commented Mar 7, 2019

nmschulte commented Mar 7, 2019

hpinkos commented Mar 11, 2019

cesium-concierge commented Apr 11, 2019

cesium-concierge commented May 11, 2019

cesium-concierge commented Jun 10, 2019

cesium-concierge commented Jul 10, 2019

cesium-concierge commented Aug 9, 2019

cesium-concierge commented Sep 8, 2019

hpinkos commented Sep 18, 2019

cesium-concierge commented Sep 18, 2019

nmschulte commented Jan 3, 2019 •

edited

Loading