Add EllipsoidRhumb class similar to EllipsoidGeodesic #7484

shehzan10 · 2019-01-14T19:47:59Z

As the title suggest, this PR adds an EllipsoidRhumb class similar to EllipsoidGeodesic. This is the first step in adding rhumb line support for geometry (#4000).

cesium-concierge · 2019-01-14T19:48:01Z

Thanks for the pull request @shehzan10!

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

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.

🌍 🌎 🌏

shehzan10 · 2019-01-14T20:03:20Z

@likangning93 Can you do the initial review?

pjcozzi · 2019-01-15T13:12:57Z

Are you sure about the name? Why isn't this EllipsoidRhumbLine?

likangning93 · 2019-01-15T14:19:53Z

Are you sure about the name? Why isn't this EllipsoidRhumbLine?

I think most dictionaries consider the term rhumb to imply the arc/line. It's the same as saying geodesic instead of geodesic line.

On the other hand the Wikipedia entry argues for loxodrome, but it also admits that modern usage is synonymous, so I think shorter is better in this case ;)

pjcozzi · 2019-01-15T14:27:57Z

Please double check how the term is used in our field; in the past 15 years, I have only heard people say "rhumb line", not "rhumb" - it's OK if it really is commonly called "rhumb" - just be sure please.

shehzan10 · 2019-01-15T14:46:23Z

I think most dictionaries consider the term rhumb to imply the arc/line. It's the same as saying geodesic instead of geodesic line.

@pjcozzi This was my exact reasoning behind the name, that's geodesic is EllipsoidGeodesic and not EllipsoidGeodesicLine, so I made it EllipsoidRhumb accordingly.

I'm seeing uses of both in API references (I'm guessing others chose the shorter name for the same reason). One notable use is STK Components which uses EllipsoidRhumbLine http://help.agi.com/AGIComponents/html/T_AGI_Foundation_Geometry_Shapes_EllipsoidRhumbLine.htm

Should I change it to EllipsoidRhumbLine?

pjcozzi · 2019-01-15T16:30:58Z

Should I change it to EllipsoidRhumbLine?

Seems that way even though it does not have perfect symmetry with EllipsoidGeodesic; these are the household names AFAIK.

likangning93

Can't say I was able to parse much of the math, but some comments in other places:

Source/Core/EllipsoidRhumbLine.js

Specs/Core/EllipsoidRhumbLineSpec.js

shehzan10 · 2019-01-15T18:50:45Z

@likangning93 Updated.

…to EllipsoidRhumbLine

shehzan10 · 2019-01-15T19:44:16Z

@likangning93 I added 2 new functions to compute intersections.

likangning93 · 2019-01-15T20:42:06Z

I threw together a crude toy to play with rhumbs vs. geodesics in Sandcastle, and it looks like the lines work as advertised:

var viewer = new Cesium.Viewer('cesiumContainer', {
    mapProjection : new Cesium.WebMercatorProjection()
});

var entities = viewer.entities;

var drawing = false;
var startPoint;

var handler = new Cesium.ScreenSpaceEventHandler(viewer.canvas);
handler.setInputAction(function(event) {
    if (drawing) {
        drawing = false;
        return;
    }

    var earthPosition = viewer.camera.pickEllipsoid(event.position);

    if (Cesium.defined(earthPosition)) {
        drawing = true;
        startPoint = Cesium.Cartographic.fromCartesian(earthPosition);
    }
}, Cesium.ScreenSpaceEventType.RIGHT_CLICK);

handler.setInputAction(function(event) {
    if (!drawing) {
        return;
    }

    var newPosition = viewer.camera.pickEllipsoid(event.endPosition);
    if (Cesium.defined(newPosition)) {
        var endPoint = Cesium.Cartographic.fromCartesian(newPosition);
        if (!Cesium.Cartographic.equalsEpsilon(endPoint, startPoint, Cesium.Math.EPSILON3)) {
            makeLines(startPoint, endPoint);
        }
    }
}, Cesium.ScreenSpaceEventType.MOUSE_MOVE);

function makeLines(start, end) {
    entities.removeAll();

    // create regular line
    entities.add({
        name : 'Geodesic',
        polyline : {
            positions : Cesium.Cartesian3.fromRadiansArray([start.longitude, start.latitude,
                                                            end.longitude, end.latitude]),
            width : 5,
            material : Cesium.Color.RED
        }
    });

    var rhumb = new Cesium.EllipsoidRhumbLine(start, end);

    var radians = [];
    var positionsCount = 100;
    for (var i = 0; i <= positionsCount; i++) {
        var interpolated = rhumb.interpolateUsingFraction(i / positionsCount);
        radians.push(interpolated.longitude);
        radians.push(interpolated.latitude);
    }

    entities.add({
        name : 'Rhumbline',
        polyline : {
            positions : Cesium.Cartesian3.fromRadiansArray(radians),
            width : 5,
            material : Cesium.Color.BLUE
        }
    });
}

Source/Core/EllipsoidRhumbLine.js

likangning93 · 2019-01-15T21:33:17Z

Source/Core/EllipsoidRhumbLine.js

+            result.height = 0;
+            return result;
+        } else if (CesiumMath.equalsEpsilon(Math.abs(CesiumMath.PI_OVER_TWO - absHeading), CesiumMath.PI_OVER_TWO, CesiumMath.EPSILON8)) {
+            if (CesiumMath.equalsEpsilon(intersectionLongitude, start.longitude, CesiumMath.EPSILON12)) {


Should this be !CesiumMath.equalsEpsilon(...?

No, this is correct. It's else if is checking if the line is N-S, and the nested if is checking if the longitude is the same, in which case there are infinite intersections.

Hmmm ok. So this case is "infinite intersections" and returns undefined, and the path below returns the intersection at the pole when considering the rhumbline to be an infinite spiral defined by a start and a heading? If so, that might be worth noting in the doc.

likangning93 · 2019-01-15T21:35:52Z

Source/Core/EllipsoidRhumbLine.js

+            var eSinPhi = ellipticity * Math.sin(phi);
+            var numerator = (1 + eSinPhi) / (1 - eSinPhi);
+            newPhi = 2 * Math.atan(leftComponent * Math.pow(numerator / denominator, ellipticity / 2)) - CesiumMath.PI_OVER_TWO;
+        } while (!CesiumMath.equalsEpsilon(newPhi, phi, CesiumMath.EPSILON12));


Do you know if there's a situation where this wouldn't converge, and we would want this to break after some number of iterations?

It will converge and it's converging pretty quickly in all the tests I did. In the test cases I ran, it took less than 20 iterations.

Specs/Core/EllipsoidRhumbLineSpec.js

likangning93 · 2019-01-15T21:51:36Z

Specs/Core/EllipsoidRhumbLineSpec.js

+        expect(pointUsingInterpolation.longitude).toEqualEpsilon(pointUsingIntersection.longitude, CesiumMath.EPSILON12);
+        expect(pointUsingInterpolation.latitude).toEqualEpsilon(pointUsingIntersection.latitude, CesiumMath.EPSILON12);
+
+        pointUsingInterpolation = rhumb.interpolateUsingFraction(1.1);


What's the expected behavior here? I guess does it clamp to the endpoint?

It will interpolate beyond the endpoint.

Specs/Core/EllipsoidRhumbLineSpec.js

shehzan10 · 2019-01-15T22:57:07Z

@likangning93 Updated.

likangning93 · 2019-01-16T16:33:27Z

Maybe document the "infinite spiral from start+heading" behavior, since that might change expectations for N-S rhumb/longitude intersection and interpolation beyond length. Otherwise that should be it for me, this looks good @shehzan10!

shehzan10 · 2019-01-16T16:45:56Z

Thanks @likangning93. I added the comment.

@mramato Do you want to take a look?

mramato · 2019-01-16T18:16:42Z

Will do later today or first thing tomorrow.

Source/Core/EllipsoidRhumbLine.js

mramato · 2019-01-17T15:05:32Z

Source/Core/EllipsoidRhumbLine.js

+        heading : {
+            get : function() {
+                //>>includeStart('debug', pragmas.debug);
+                Check.defined('distance', this._distance);


I know this is what EllipsoidGeodesic has, but this is a really odd check to me, especially since there is no public property called distance. Would it be better for us to update this and EllipsoidGeodesic to just be undefined? On the other hand, it may never happen in practice so I could be bikeshedding. I'm okay with just punting, but since EllipsoidGeodesic is so old, it's good to revisit things once in a while.

I think it was just a way to assure that heading and distance are both computed since the constructor allows using undefined as start and end.

We could simply change this to check whether heading is defined, which will make more sense here.

Since EllipsoidGeodescic has been like this forever, I'm probably bike shedding, let's just leave it as-is and we can worry about it if it ever comes up.

mramato · 2019-01-17T15:08:09Z

Source/Core/EllipsoidRhumbLine.js

+     * @param {EllipsoidRhumbLine} [result] The object in which to store the result.
+     * @returns {EllipsoidRhumbLine} The EllipsoidRhumbLine object.
+     */
+    EllipsoidRhumbLine.fromStartAndEnd = function(start, end, ellipsoid, result) {


Why is this function needed, it just matches the constructor, doesn't it??

I added it because it allows using a scratch result object.

Seems like overkill, can't you just call setEndPoints on an existing object? I doubt someone would need to change the ellipsoid on an existing instance. If we have a valid use case, then maybe it's worth it, but we try not to add code "just because it might be useful" since that introduces more code to maintain.

mramato · 2019-01-17T15:12:05Z

Source/Core/EllipsoidRhumbLine.js

+        result._heading = CesiumMath.negativePiToPi(heading);
+        result._distance = distance;
+
+        result._end = result.interpolateUsingSurfaceDistance(distance, new Cartographic());


This whole function is kind of an odd implementation to me. Why not break out interpolateUsingSurfaceDistance into a standalone local private function that can be called with start, heading, distance, ellipsoid, us that to compute the end point then just create the fully contructed result. Using a half-initialized instance like this seems like something that could bite us.

Obviously we still expose the prototype version of interpolateUsingSurfaceDistance that just called the local one.

Yeah, this could work better. I'll make the change.

mramato · 2019-01-17T15:14:05Z

Source/Core/EllipsoidRhumbLine.js

+            result.latitude = start.latitude;
+            result.height = 0;
+            return result;
+        } else if (CesiumMath.equalsEpsilon(Math.abs(CesiumMath.PI_OVER_TWO - absHeading), CesiumMath.PI_OVER_TWO, CesiumMath.EPSILON8)) {


I assume these epsilons were from the ported code? (if not, how did we choose them?) Same for the rest of the PR.

Yes, it was chosen as the epsilon at which Math.cos() returns 1.0 similar to http://help.agi.com/AGIComponents/html/F_AGI_Foundation_Trig_AngleEpsilon.htm.

mramato · 2019-01-17T15:16:32Z

Plese merge in master when you get a chance.

mramato · 2019-01-17T15:25:53Z

Thanks @shehzan10, tests look comprehensive (but I can't run coverage right now since I'm on Linux).

Those few comments are all I have, bump when ready and I'll merge.

Looking forward to the follow-up PR for geometries!

GatorScott · 2019-01-17T15:46:47Z

Source/Core/EllipsoidRhumbLine.js

+
+    /**
+     * Initializes a rhumb line on the ellipsoid connecting the two provided planetodetic points.
+     *


Should document behavior when start and end are on opposite meridians (returns E-W line). Should there be a mechanism to get the opposite line?

@GatorScott This check does not allow points to be on opposite ends meridians (same limitation as EllipsoidGeodesic).

https://github.com/AnalyticalGraphicsInc/cesium/blob/c323623a52b7b7b318421ffbb09738a4d9727d5e/Source/Core/EllipsoidRhumbLine.js#L159

Shouldn't this limitation/restriction be documented so the developer can protect themselves from this DeveloperError: "Expected value to be greater than or equal to 0.0125, actual value was 0.0"? Secondarily, is 0.716197 degrees the right epsilon?

I added the documentation for the developer error. I'm not entirely certain about how 0.0125 came to be, which is from the STK Components code. @fstoner anything you can add?

shehzan10 · 2019-01-17T16:25:22Z

@mramato Updated.

mramato · 2019-01-18T14:59:39Z

Looks good, just document the exception that @GatorScott mentioned and this should be good to go.

…llipsoidRhumbLine

mramato · 2019-01-18T15:19:03Z

Thanks again, @shehzan10.

Add EllipsoidRhumb class similar to EllipsoidGeodesic

efe2a13

Add EllipsoidRhumb to CHANGES.md

1722c44

EllipsoidRhumb -> EllipsoidRhumbLine

94e1eca

likangning93 reviewed Jan 15, 2019

View reviewed changes

shehzan10 added 2 commits January 15, 2019 13:22

Fixes from PR comments

e64e988

Check correctly for distance=0 in EllipsoidRhumbLine, other PR fixes

86c23bc

shehzan10 force-pushed the ellipsoid-rhumb branch from ebbde06 to 86c23bc Compare January 15, 2019 18:48

Add intersectionWithLatitude and intersectionWithLongitude functions …

0e12d1c

…to EllipsoidRhumbLine

likangning93 reviewed Jan 15, 2019

View reviewed changes

Fixes from PR comments

882b01a

Add comment about rhumb lines being spirals for rhumb line intersection

c323623

mramato reviewed Jan 17, 2019

View reviewed changes

Source/Core/EllipsoidRhumbLine.js Show resolved Hide resolved

mramato reviewed Jan 17, 2019

View reviewed changes

GatorScott reviewed Jan 17, 2019

View reviewed changes

shehzan10 added 3 commits January 17, 2019 11:01

Remove EllipsoidRhumbLine.fromStartAndEnd function

12bc94d

Restructure EllipsoidRhumbLine.fromStartHeadingDistance

3bf2528

Merge remote-tracking branch 'upstream/master' into ellipsoid-rhumb

813282f

Document developer error for angle between start and end when using E…

f687d2a

…llipsoidRhumbLine

mramato merged commit 38efff5 into CesiumGS:master Jan 18, 2019

shehzan10 deleted the ellipsoid-rhumb branch January 18, 2019 15:27

shehzan10 mentioned this pull request Feb 4, 2019

improve sanity check for geodesic distance calc #7457

Closed

Add EllipsoidRhumb class similar to EllipsoidGeodesic #7484

Add EllipsoidRhumb class similar to EllipsoidGeodesic #7484

Conversation

shehzan10 commented Jan 14, 2019

cesium-concierge commented Jan 14, 2019

shehzan10 commented Jan 14, 2019

pjcozzi commented Jan 15, 2019

likangning93 commented Jan 15, 2019

pjcozzi commented Jan 15, 2019

shehzan10 commented Jan 15, 2019

pjcozzi commented Jan 15, 2019

likangning93 left a comment

Choose a reason for hiding this comment

shehzan10 commented Jan 15, 2019

shehzan10 commented Jan 15, 2019

likangning93 commented Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzan10 commented Jan 15, 2019

likangning93 commented Jan 16, 2019

shehzan10 commented Jan 16, 2019

mramato commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Jan 17, 2019

mramato commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzan10 commented Jan 17, 2019

mramato commented Jan 18, 2019

mramato commented Jan 18, 2019

likangning93 commented Jan 15, 2019 •

edited

Loading