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 GreatCircleLayer #2640

Merged
merged 8 commits into from
Feb 11, 2019
Merged

Add GreatCircleLayer #2640

merged 8 commits into from
Feb 11, 2019

Conversation

XiaokaiUber
Copy link
Contributor

@XiaokaiUber XiaokaiUber commented Jan 30, 2019

For #2499

Background

customize ArcLayer to draw a great circle between two points on the map

Change List

  • implement GreatCircleLayer by customizing ArcLayer
  • added vertex shader for GreatCircleLayer
  • add GreatCircleLayer example in layer browser

result

great-circle

arc

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

I really like this idea, fits well with our geospatial focus and the samples look really cool.

But shouldn't this be an example using a customized/subclassed ArcLayer?

To me this seem like an odd feature to add to a generic layer, especially a layer that currently works in any coordinate system, not just geospatial.

Specifically, I see a lot of extra lng/lat based shader code and a new prop that only apply to a very specialized case. I am assuming that 95% of ArcLayer users won't use this?

@XiaokaiUber
Copy link
Contributor Author

XiaokaiUber commented Jan 30, 2019

I really like this idea, fits well with our geospatial focus and the samples look really cool.

But shouldn't this be an example using a customized/subclassed ArcLayer?

To me this seem like an odd feature to add to a generic layer, especially a layer that currently works in any coordinate system, not just geospatial.

Specifically, I see a lot of extra lng/lat based shader code and a new prop that only apply to a very specialized case. I am assuming that 95% of ArcLayer users won't use this?

Changes made to ArcLayer are reverted. GreatCircleLayer is created as a subclass of ArcLayer in the experiment layer folder. A GreatCircleLayer example is added to layer browser(Experimental Core Layers) as can be seen below:
great-circle-layer

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 30, 2019

Changes made to ArcLayer are reverted. GreatCircleLayer is created as a subclass of ArcLayer in the experiment layer folder. A GreatCircleLayer example is added to layer browser(Experimental Core Layers) as can be seen below:

Thanks, that makes a lot more sense.

I could argue that this might be more of an "example specific layer" rather than an "experimental layer" (i.e. users would have to copy it from an example rather than import it from experimental layers), but I am fine with either approach.

Having it in experimental layers makes sense if we think there is a future layer catalog where this layer could fit in, I could for instance imagine a layer catalog with various geospatial "helper layers", other layers could perhaps a "compass layer", ...

Also, you could probably build some really cool geospatial grid layer/satellite orbit layers using this layer as a primitive.

@XiaokaiUber XiaokaiUber changed the title great circle Add great circle Jan 31, 2019
{source: [-20, 50], target: [-160, 50]},
{source: [-20, 60], target: [-160, 60]},
{source: [-20, 70], target: [-160, 70]}
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need more test cases. E.g. longitude 10 to longitude -170. Source and target not on the same latitude.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

more test cases are added.
great_circle_example

@Pessimistress
Copy link
Collaborator

Please update the PR title and summary to reflect the latest changes.

@XiaokaiUber XiaokaiUber changed the title Add great circle Add GreatCircleLayer Feb 5, 2019
vec3 nextPos = vec3(degrees(interpolate(source, target, angularDist, nextSegmentRatio)), 0.0);

vec4 curr = project_position_to_clipspace(currPos, instancePositions64Low.xy, vec3(0.0));
vec4 next = project_position_to_clipspace(nextPos, instancePositions64Low.zw, vec3(0.0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although instancePositions64Low is not significant in this use case, you need to make sure that the next position in one segment and the curr position in the following segment match.

vec3 currPos = vec3(degrees(interpolate(source, target, angularDist, segmentRatio)), 0.0);
vec3 nextPos = vec3(degrees(interpolate(source, target, angularDist, nextSegmentRatio)), 0.0);

vec2 currPos64Low = degrees(interpolate(instancePositions64Low.xy, instancePositions64Low.zw, angularDist, segmentRatio));
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • This is mathematically incorrect
  • 64bit accuracy of interpolated points is not a concern for this layer

So just use linear interpolation.

@XiaokaiUber XiaokaiUber merged commit 67fde63 into master Feb 11, 2019
@XiaokaiUber XiaokaiUber deleted the great-circle branch February 11, 2019 22:36
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