Closed gaelforget closed 4 months ago
This should work with other projections such as
fig = Figure()
ax = GeoAxis(fig[1,1]; dest = "+proj=wintri +lon_0=-160", coastlines=true)
fig
Note : issues remain with the axes as highlighted in this plot but that's separate from the coastline part I think.
Could I get a review on this PR?
@asinghvi17 maybe based on https://github.com/MakieOrg/GeoMakie.jl/issues/118#issuecomment-1161280836
It seems GUI zooming doesn't quite work as expected, though I'm not entirely sure if that is actually related to this PR.
edit: after reviewing the PR more closely, it is clear that this is a separate issue.
If I set lon0=0
, things work as expected:
however, setting lon0=160
:
Note that I'm also on GLMakie#master, Makie#master, etc... but I can test this again on latest release.
Will review this in a couple days.
Oops - looks like I didn't post my review when I last looked at this. I'll look at it again in the next couple days.
Couple questions:
- Why is this in a Module?
- does this only work on LineStrings for now, or is there a more generic way?
Simon and I just spoke last night and we're trying to get this kind of splitting directly in the transform, so that we can override the behaviour for individual transforms as well (e.g., some of the weird transforms which aren't "convex").
Could we merge this (freshly updated) PR and punt further improvements to later revisions (when new issue arises)?
The current implementation is not that intrusive (see module), PR provides test & docstrings, and having this feature inside GeoMakie would make the package more potentially useful for ocean viz.
If a general solution were eventually found and implemented (for now this PR only treats the lon_0 & LineStrings case) then that's great, but this might take a few more years, who knows ...
Currently I am just carrying what I need in an extension to MeshArrays ( https://github.com/JuliaClimate/MeshArrays.jl/blob/master/ext/MeshArraysMakieExt.jl ), because I couldn't get this merged here in timely manner. That's less than ideal, so I am giving GeoMakie one more try now.
Sure, we can merge for now but I will refactor a bit to make it easier to merge #207. That PR also adds a dependency on GeometryOps.jl that should make it a lot easier to extend this for arbitrary geometry.
Sure, we can merge for now but I will refactor a bit to make it easier to merge #207. That PR also adds a dependency on GeometryOps.jl that should make it a lot easier to extend this for arbitrary geometry.
Great! Thank you very much @asinghvi17 & @haakon-e
This adresses
https://github.com/MakieOrg/GeoMakie.jl/issues/118#issuecomment-1161280836 https://github.com/MakieOrg/GeoMakie.jl/issues/126 https://github.com/MakieOrg/GeoMakie.jl/issues/7
The new
split
method let's us go fromto
The new feature can be tested with: