Add Rhumb Line Support to Polygon and Polyline Geometries #7492

shehzan10 · 2019-01-18T15:53:07Z

Added rhumb line support to:

PolygonGeometry and PolygonOutlineGeometry (and by extension GroundPrimitive).
PolylineGeometry, SimplePolylineGeometry, and GroundPolylineGeometry.

This PR also changes the default behavior of GeoJSON data source to be to use Rhumb lines. This makes GeoJSON spec compliant (See below).

Using LineType supercedes PolylineGeometry.followSurface. This option has been deprecated.

Also added sandcastle examples for all the rhumb line geometries.

Things to look at:

When should PolylineGeometry.followSurface be removed? Right now I put 1.55 as placeholder.
I used LineType enum as a placeholder and wanted to discuss in this PR what the most appropriate name for this would be. Potential options are CurveType, PathType or something better.

I'm limiting the scope of this PR to the above mentioned geometries. I will open an issue for the remaining geometry types that will benefit from rhumb line support.

Ref #4000.

GeoJSON Spec Snippet (from https://tools.ietf.org/html/rfc7946):

A line between two positions is a straight Cartesian line, the shortest line between those two points in the coordinate reference system (see Section 4).

In other words, every point on a line that does not cross the antimeridian between a point (lon0, lat0) and (lon1, lat1) can be calculated as
F(lon, lat) = (lon0 + (lon1 - lon0) * t, lat0 + (lat1 - lat0) * t)
with t being a real number greater than or equal to 0 and smaller than or equal to 1. Note that this line may markedly differ from the geodesic path along the curved surface of the reference ellipsoid.

The same applies to the optional height element with the proviso that the direction of the height is as specified in the coordinate reference system.

Note that, again, this does not mean that a surface with equal height follows, for example, the curvature of a body of water. Nor is a surface of equal height perpendicular to a plumb line.

Examples of positions and geometries are provided in Appendix A,"Geometry Examples".

…support

…pe options to PolylineGeometryUpdater

cesium-concierge · 2019-01-18T15:53:09Z

Thanks for the pull request @shehzan10!

✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

shehzan10 · 2019-01-18T16:01:38Z

@likangning93 Can you do the first review?

mramato · 2019-01-18T17:52:58Z

Fixes #4000 (Be sure to test with the geojson in that issue).

likangning93 · 2019-01-18T19:15:40Z

Is the line interpolation method described above different from rhumblines? F(lon, lat) = (lon0 + (lon1 - lon0) * t, lat0 + (lat1 - lat0) * t) seems to describe a straight line in plate caree as opposed to rhumblines which are straight in mercator but can be curved in plate caree.

shehzan10 · 2019-01-18T20:12:39Z

Fixes #4000 (Be sure to test with the geojson in that issue).

Yeah, that was my primary test. I didn't know where the data was from so I didn't add it.

That said, the US States geojson in sandcastle curves according to the rhumb line on the 45th parallel.

likangning93 · 2019-01-18T21:21:19Z

Is the line interpolation method described above different from rhumblines? F(lon, lat) = (lon0 + (lon1 - lon0) * t, lat0 + (lat1 - lat0) * t) seems to describe a straight line in plate caree as opposed to rhumblines which are straight in mercator but can be curved in plate caree.

Summary of offline discussion, it looks like geojson.io uses the equivalent of rhumblines because their lines between two points are straight on what appears to be a mercator projection. So it's probably fine for us to just use rhumblines for geojson even though (I think) that's technically wrong according to the spec. At least if someone's drawn a polygon around some points in geojson.io, it will bound those points in Cesium too instead of perhaps losing a few...

likangning93 · 2019-01-21T15:41:33Z

Source/Core/GroundPolylineGeometry.js

+                        !Cartesian3.equalsEpsilon(intersection, p1, CesiumMath.EPSILON7)) {
+                        splitPositions.push(Cartesian3.clone(intersection));
+                    }
+                }


I don't know if this will have an impact on javascript optimization, but this could be easier to read as two loops within if (groundPolylineGeometry.lineType === LineType.GEODESIC) {.

I think this one is better the way it is. If we do something like

if (GEODESIC) { } else if (RHUMB) { }

we'll need to duplicate the initial intersection test code.

Since the test will always pass or always fail inside the for-loop, branch prediction should work well and minimize any performance drops.

You're probably right that it won't make a difference for performance, but it still just feels kind of "off" to me reading that there's a boolean check with the same result repeated so many times.

I'd still prefer separate loops, but it's not a huge deal.

Source/Core/GroundPolylineGeometry.js

Source/Core/PolygonGeometry.js

Source/Core/PolygonGeometryLibrary.js

Specs/Core/PolylineGeometrySpec.js

Specs/DataSources/GeoJsonDataSourceSpec.js

Specs/DataSources/PolylineGeometryUpdaterSpec.js

shehzan10

@likangning93 Made the recommended changes, and in a couple of places have questions.

shehzan10 · 2019-01-22T16:49:15Z

Source/Core/GroundPolylineGeometry.js

+                        !Cartesian3.equalsEpsilon(intersection, p1, CesiumMath.EPSILON7)) {
+                        splitPositions.push(Cartesian3.clone(intersection));
+                    }
+                }


I think this one is better the way it is. If we do something like

if (GEODESIC) { } else if (RHUMB) { }

we'll need to duplicate the initial intersection test code.

Since the test will always pass or always fail inside the for-loop, branch prediction should work well and minimize any performance drops.

Source/Core/PolygonGeometryLibrary.js

shehzan10 · 2019-01-22T17:20:01Z

Source/Core/PolygonOutlineGeometry.js

@@ -485,7 +525,6 @@ define([

        var geometryInstance;
        var geometries = [];
-        var minDistance = CesiumMath.chordLength(granularity, ellipsoid.maximumRadius);


I think I changed while coding when it made sense to pass granularity, but I'm going to rever this since that is not being done anymore.

shehzan10 · 2019-01-22T17:27:23Z

Source/Core/PolylineGeometry.js

+                granularity: granularity,
+                ellipsoid: ellipsoid,
+                height: heights
+            });


I'll take the suggestion, but just for the record point out that because LineType.STRAIGHT is also supported, the entire block will have to be put into if (lineType === LineType.GEODESIC || lineType === LineType.RHUMB) with the if-conditions you mentions as nested.

shehzan10 · 2019-01-22T18:16:11Z

Source/DataSources/PolylineGeometryUpdater.js

@@ -626,6 +650,8 @@ define([
        geometryUpdater._clampToGround = Property.getValueOrDefault(polyline._clampToGround, time, false);
        geometryUpdater._groundGeometryOptions.positions = positions;
        geometryUpdater._groundGeometryOptions.width = Property.getValueOrDefault(polyline._width, time, 1);
+        geometryUpdater._groundGeometryOptions.lineType = Property.getValueOrDefault(polyline._lineType, time, LineType.GEODESIC);
+        geometryUpdater._groundGeometryOptions.granularity = Property.getValueOrDefault(polyline._granulatiry, time, 9999);


For the default, yes it is a bit weird, but the reason it's working is that polyline._granularity is always defined and will thus use that.

To fix it, we could base the default on the lineType and set it accordingly. What do you think?

shehzan10 · 2019-01-22T18:17:30Z

Source/DataSources/PolylineGraphics.js

     * @param {Property} [options.clampToGround=false] A boolean Property specifying whether the Polyline should be clamped to the ground.
     * @param {Property} [options.width=1.0] A numeric Property specifying the width in pixels.
     * @param {Property} [options.show=true] A boolean Property specifying the visibility of the polyline.
     * @param {MaterialProperty} [options.material=Color.WHITE] A Property specifying the material used to draw the polyline.
     * @param {MaterialProperty} [options.depthFailMaterial] A property specifying the material used to draw the polyline when it is below the terrain.
-     * @param {Property} [options.granularity=Cesium.Math.RADIANS_PER_DEGREE] A numeric Property specifying the angular distance between each latitude and longitude if followSurface is true.
+     * @param {Property} [options.granularity=Cesium.Math.RADIANS_PER_DEGREE] A numeric Property specifying the angular distance between each latitude and longitude if lineType is now LineType.STRAIGHT.


There is no subdivision (thus no granularity) when using STRAIGHT.

Specs/DataSources/PolylineGeometryUpdaterSpec.js

Source/Core/SimplePolylineGeometry.js

shehzan10 · 2019-01-23T15:25:18Z

@likangning93 Updated.

mramato · 2019-01-23T18:09:32Z

Source/Core/PolygonGeometry.js

@@ -578,6 +581,9 @@ define([
        if (defined(options.perPositionHeight) && options.perPositionHeight && defined(options.height)) {
            throw new DeveloperError('Cannot use both options.perPositionHeight and options.height');
        }
+        if (defined(options.lineType) && options.lineType === LineType.STRAIGHT) {
+            throw new DeveloperError('Cannot use {@link LineType.STRAIGHT} as option.lineType');


Can't use jsdoc inside of an error string.

mramato · 2019-01-23T18:13:05Z

I did a secondary high-level pass, but didn't have much. @hpinkos do you think you can take a look at least at the low-level geometry/pipeline side of things? You know that code better than most. Thanks!

hpinkos · 2019-01-23T18:25:35Z

Apps/Sandcastle/gallery/Polyline.html

@@ -73,7 +84,7 @@
        positions : Cesium.Cartesian3.fromDegreesArrayHeights([-75, 43, 500000,
                                                               -125, 43, 500000]),
        width : 10,
-        followSurface : false,
+        lineType : Cesium.LineType.STRAIGHT,


At demo day, didn't we talk about coming up with a better name than STRAIGHT?

I can think of DIRECT. Do you have any suggestions?

I do think STRAIGHT is a fair option since the enum is LineType, so we get Geodesic line, Rhumb line and Straight line.

Anyone else have any suggestions?

I prefer STRAIGHT, why do we think it would be confusing?

I think the enum name itself, LineType is open to change. As I wrote in the PR description, options I can think of are CurveType PathType.

My only idea is something super pedantic like NOT_CURVED_TO_ELLIPSOID_SURFACE lol

I don't feel super strongly, but I thought it was something Patrick pointed out during demo day

What about ArcType GEODESIC RHUMB or NONE?

Rhumb lines are straight only in a Mercator projection.

^ ^ ^
Proof that "STRAIGHT" is confusing. @GatorScott in this case we were using 'STRAIGHT' for a line with neither rhumb or geodesic subdivision that connects two points in space. ie a line from the ground to a satellite.

I'm in favor of switching to my suggestion above: ArcType GEODESIC RHUMB or NONE

I'm not against the proposed ArcType enum and values, I tried to google around and didn't see anything anyone else was already doing that we could reference.

CHANGES.md

Source/Core/GroundPolylineGeometry.js

Source/Core/PolygonGeometryLibrary.js

hpinkos · 2019-01-23T20:00:31Z

If you haven't already, make an issue for removing the deprecated feature. This change is probably going out in 1.54, so I think removing followSurface in 1.55 is too quick. I would give it until 1.57

hpinkos · 2019-01-23T20:00:58Z

That's all my comments though. The geometry changes look solid. Great work @shehzan10!

shehzan10 · 2019-01-23T21:02:04Z

Created issue to remove deprecated option followSurface #7504.

mramato · 2019-01-23T21:10:34Z

I used LineType enum as a placeholder and wanted to discuss in this PR what the most appropriate name for this would be. Potential options are CurveType, PathType or something better.

I think LineType is the best of the options you propose. Path would get confusing with other Path terminology in Cesium and I would expect CurveType to be more along the Bezier type functionality.

shehzan10 · 2019-01-24T04:21:29Z

@hpinkos changed LineType to ArcType. This is ready now.

shehzan10 · 2019-01-24T13:19:24Z

I think I missed reordering the includes since the alphabet changed for ArcType. I'll reorder it and bump.

shehzan10 · 2019-01-24T14:46:46Z

@hpinkos This is good to go.

Source/Core/GroundPolylineGeometry.js

Source/Core/PolygonGeometryLibrary.js

Source/Core/PolylineGeometry.js

Source/Core/SimplePolylineGeometry.js

…t check

hpinkos · 2019-01-24T15:30:37Z

Great work @shehzan10! I'll merge this as soon as CI passes

shehzan10 added 13 commits January 18, 2019 10:28

Initial implementation of using rhumb lines limited to PolygonGeometry

f473014

Rhumb lines working on PolygonGeometry with geojson load

ba3a6dc

Add enum for LineType.STRAIGHT

0248d9e

Add rhumb line support to polygon and polyline geometry with extrude …

f90b6d4

…support

Add lineType support to GeoJsonDataSource

5be47b2

Add rhumb line support to GroundPolylineGeometry

7c60e88

Rename EllipsoidRhumb -> EllipsoidRhumbLine

d9db99a

Fix GroundPolylineGeometry across PM and IDL, Add granularity, lineTy…

6362be5

…pe options to PolylineGeometryUpdater

Add tests for all the classes where LineType is used

4c2141d

Remove usage of EllipsoidRhumbLine.fromStartAndEnd

9cbc248

Fix propagation of lineType from shadow volume

851a21e

Add rhumb line polygons and polylines to sandcastle examples

39e6d95

Update CHANGES.md

739eb52

Fix tests failing because of ground primitives

62b28a8

shehzan10 mentioned this pull request Jan 18, 2019

Add Rhumb Line Support to Additional Geometries #7493

Open

13 tasks

Revert inadvertent change to sandcastle

2745a2d

shehzan10 added 2 commits January 21, 2019 09:49

Merge remote-tracking branch 'upstream' into rhumb-lines

0f4e088

Add PR numbers to CHANGES.md

9ee96b7

likangning93 reviewed Jan 21, 2019

View reviewed changes

Fixes from PR comments

a47ebd1

shehzan10 commented Jan 22, 2019

View reviewed changes

likangning93 reviewed Jan 23, 2019

View reviewed changes

Source/Core/SimplePolylineGeometry.js Show resolved Hide resolved

Some more fixes from PR review

1dcdbe2

mramato reviewed Jan 23, 2019

View reviewed changes

shehzan10 added 2 commits January 23, 2019 13:23

Typo

6584a8f

Remove lineType option from GeoJsonDataSource. Always uses RHUMB lines

b2e70ed

hpinkos reviewed Jan 23, 2019

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

hpinkos reviewed Jan 23, 2019

View reviewed changes

Source/Core/GroundPolylineGeometry.js Outdated Show resolved Hide resolved

hpinkos reviewed Jan 23, 2019

View reviewed changes

Source/Core/PolygonGeometryLibrary.js Outdated Show resolved Hide resolved

shehzan10 added 2 commits January 23, 2019 15:57

Add checks for invalid lineType option, cleaner checks in constructors

81b2ecb

Better desciption of changing followSurface to lineType in CHANGES.md

ce9f96b

shehzan10 mentioned this pull request Jan 23, 2019

Remove deprecated option followSurface in 1.57 #7504

Closed

Change LineType to ArcType, LineType.STRAIGHT to ArcType.NONE

96d25d5

Fix ordering of defines for ArcType

763aa70

hpinkos reviewed Jan 24, 2019

View reviewed changes

Source/Core/GroundPolylineGeometry.js Outdated Show resolved Hide resolved

hpinkos reviewed Jan 24, 2019

View reviewed changes

Source/Core/PolygonGeometryLibrary.js Outdated Show resolved Hide resolved

hpinkos reviewed Jan 24, 2019

View reviewed changes

Source/Core/PolylineGeometry.js Show resolved Hide resolved

hpinkos reviewed Jan 24, 2019

View reviewed changes

Source/Core/SimplePolylineGeometry.js Show resolved Hide resolved

Fixes for PR comments: Remove followSurface from doc, remove redundan…

5f7a7ac

…t check

hpinkos merged commit 9892e1d into CesiumGS:master Jan 24, 2019

hpinkos mentioned this pull request Jan 24, 2019

Add option to use rhumb lines for geometry #4000

Closed

shehzan10 deleted the rhumb-lines branch January 24, 2019 16:08

OmarShehata mentioned this pull request Aug 8, 2019

GeoJSON renders incorrectly with default Rhumb arc type #8042

Open

Add Rhumb Line Support to Polygon and Polyline Geometries #7492

Add Rhumb Line Support to Polygon and Polyline Geometries #7492

Conversation

shehzan10 commented Jan 18, 2019 • edited

cesium-concierge commented Jan 18, 2019 • edited by hpinkos

shehzan10 commented Jan 18, 2019

mramato commented Jan 18, 2019

likangning93 commented Jan 18, 2019

shehzan10 commented Jan 18, 2019

likangning93 commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzan10 left a 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 23, 2019

Choose a reason for hiding this comment

mramato commented Jan 23, 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

hpinkos commented Jan 23, 2019

hpinkos commented Jan 23, 2019

shehzan10 commented Jan 23, 2019

mramato commented Jan 23, 2019

shehzan10 commented Jan 24, 2019

shehzan10 commented Jan 24, 2019

shehzan10 commented Jan 24, 2019

hpinkos commented Jan 24, 2019

shehzan10 commented Jan 18, 2019 •

edited

cesium-concierge commented Jan 18, 2019 •

edited by hpinkos