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

Add Rhumb Line Support to Polygon and Polyline Geometries #7492

Merged
merged 27 commits into from
Jan 24, 2019

Conversation

shehzan10
Copy link
Member

@shehzan10 shehzan10 commented Jan 18, 2019

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".

@cesium-concierge
Copy link

cesium-concierge commented Jan 18, 2019

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
Copy link
Member Author

@likangning93 Can you do the first review?

@mramato
Copy link
Contributor

mramato commented Jan 18, 2019

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

@likangning93
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Source/Core/PolygonGeometry.js Outdated Show resolved Hide resolved
Source/Core/PolygonGeometryLibrary.js Outdated Show resolved Hide resolved
Source/Core/PolygonGeometryLibrary.js Show resolved Hide resolved
Specs/Core/PolylineGeometrySpec.js Outdated Show resolved Hide resolved
Specs/DataSources/GeoJsonDataSourceSpec.js Outdated Show resolved Hide resolved
Specs/DataSources/PolylineGeometryUpdaterSpec.js Outdated Show resolved Hide resolved
Specs/DataSources/PolylineGeometryUpdaterSpec.js Outdated Show resolved Hide resolved
Specs/DataSources/PolylineGeometryUpdaterSpec.js Outdated Show resolved Hide resolved
Copy link
Member Author

@shehzan10 shehzan10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

!Cartesian3.equalsEpsilon(intersection, p1, CesiumMath.EPSILON7)) {
splitPositions.push(Cartesian3.clone(intersection));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@@ -485,7 +525,6 @@ define([

var geometryInstance;
var geometries = [];
var minDistance = CesiumMath.chordLength(granularity, ellipsoid.maximumRadius);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

granularity: granularity,
ellipsoid: ellipsoid,
height: heights
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* @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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Specs/DataSources/PolylineGeometryUpdaterSpec.js Outdated Show resolved Hide resolved
@shehzan10
Copy link
Member Author

@likangning93 Updated.

@@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use jsdoc inside of an error string.

@mramato
Copy link
Contributor

mramato commented Jan 23, 2019

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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only idea is something super pedantic like NOT_CURVED_TO_ELLIPSOID_SURFACE lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about ArcType GEODESIC RHUMB or NONE?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rhumb lines are straight only in a Mercator projection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ ^ ^
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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@hpinkos
Copy link
Contributor

hpinkos commented Jan 23, 2019

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
Copy link
Contributor

hpinkos commented Jan 23, 2019

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

@shehzan10
Copy link
Member Author

Created issue to remove deprecated option followSurface #7504.

@mramato
Copy link
Contributor

mramato commented Jan 23, 2019

  • 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
Copy link
Member Author

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

@shehzan10
Copy link
Member Author

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

@shehzan10
Copy link
Member Author

@hpinkos This is good to go.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 24, 2019

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

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.

6 participants