-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 satellite and various projection types to geo subplots #5801
Conversation
LGTM! ✨ |
src/plots/geo/geo.js
Outdated
constants.lonaxisSpan[projType] / 2 : | ||
null; | ||
var clipAngle = | ||
projType === 'satellite' ? Math.acos(1 / projection.distance()) * 180 / Math.PI : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does projection.distance()
come from? Is that linked to the projection.scale
attribute? I imagine users of the satellite would want to be able to fix the satellite's location (height and center lat/lon) and still zoom and pan - to match up with a specific image taken by a satellite. Is that possible as it stands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distance
and tilt
options are now implemented for satellite
by d102a47.
|
||
'airy': 'airy', | ||
// 'albers': 'albers', | ||
'armadillo': 'armadillo', | ||
// 'armadillo': 'armadillo', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a pattern to the ones we're still omitting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these are now enabled by eb30073.
Let me see if I could easily fix the clipping issue for some other remaining ones...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the remaining ones are interrupted-projections and polyhedral-projections as well as two-point projections , require extra work and attributes and we could say it is outside the scope of this PR.
@plotly/plotly_js and @disberd this PR is now reviewable and if you could play with it : ) |
"fitbounds": "locations", | ||
"projection": { | ||
"rotation": {"lat": 45}, | ||
"type": "mercator" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange to think of this as a mercator projection when it has a rotation... can we omit projection.rotation
from this mock, and let the projection do its own thing? Hopefully that will automatically rotate those projections that use rotation to fit their contents, but will use center
for mercator and others like it. That would allow this mocks to function as more than just "these projections all work zoomed in", but also "here's the kind of map you can expect from each projection"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping rotation for those three projections can simply help illustrate that the projection could actually be used with Canada.
I'd argue that this shows these projections may not be optimizing the correct variables when doing fitbounds
- but if there's no obvious fix for that we can leave it. Thanks for dropping all the other rotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 that's a whole lot of projections 😲
Resolves #4781 by adding new projection types to
geo
subplots.cc: #424
Here is the full list of types:
@plotly/plotly_js
cc: @disberd