feat: add projections #39
Conversation
src/s2geography/projections.cc
Outdated
| } | ||
|
|
||
| S2::Projection* mercator() { | ||
| static S2::MercatorProjection projection(20037508.3427892); |
There was a problem hiding this comment.
Is there a need to make this configurable? (maybe since it is in R s2?) Currently just took the value from the s2 code, which I think matches the reported projected bounds of EPSG:3857 (https://epsg.io/3857)
There was a problem hiding this comment.
I am not sure the degree to which that's ever been used in R/s2, so you're free give this a better name or make this configurable (or not include it!). Probably safer to refer to this as pseudo_mercator() and make it align with EPSG:3857 (I think Google has an internal version of this that uses -2pi...2pi instead of a range in meters, but probably nobody else needs that).
There was a problem hiding this comment.
I think including (Web)Mercator could still be useful to enable plotting (e.g. for combining with tiles where you want to use WebMercator and not something like Orthographic), without requiring some PROJ bindings (of course, for something like spherely, we could also easily have an optional dependency on pyproj for those use cases)
There was a problem hiding this comment.
You probably want this here regardless of PROJ, since PROJ won't handle the wraparound situation here (and S2's projections will!)
src/s2geography/projections.cc
Outdated
| S2::Projection* orthographic(const S2LatLng& centre) { | ||
| static OrthographicProjection projection(centre); | ||
| return &projection; | ||
| } |
There was a problem hiding this comment.
I don't think you want this global state here, which won't work, for example, if two threads are trying to plot a spinning globe (plotting a spinning globe is the reason I added this 🙂 ). You probably need this to return a std::unique_ptr<S2::Projection>.
(That begs the question of whether the options class should be parameterized as S2::Projection* or std::unique_ptr<S2::Projection>. Probably the latter is more flexible).
There was a problem hiding this comment.
Ah, yes of course, I was just blindly copying the code of the others.
Returning a unique_ptr for all of them will be the easiest then I suppose
There was a problem hiding this comment.
I tried using a unique_ptr, but ran into the issue that it is then not copyable, and because the Options class also holds the projection, it then makes the Options class not copyable (AFAIU?), which is then a bit annoying to use that.
So used a shared_ptr instead. Does that look good, or should I be able to get it working with a unique_ptr as well?
Moving the
lnglat()projection definition (which is currently already used in the geoarrow import/export) to its own file, so we can easily add more (without making geoarrow.cc unwieldy large).Will add additional ones in a next commit.