-
Notifications
You must be signed in to change notification settings - Fork 782
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 a dedicated proj=webmerc operation #925
Conversation
Was also wondering if it would be worth to have a +proj=gmerc that would be an alias for +proj=merc +aux_sphere_type=0 ? |
This is nice and definitely a better solution to what we have now! This was on my list of things to do in the future so thanks for taking care of it.
That would have been my approach. I think it is a cleaner solution although it is a little more work to add a new projection. I think calling it |
src/PJ_merc.c
Outdated
@@ -48,11 +49,36 @@ static LP s_inverse (XY xy, PJ *P) { /* Spheroidal, inverse */ | |||
return lp; | |||
} | |||
|
|||
/* Cf http://desktop.arcgis.com/en/arcmap/10.3/guide-books/map-projections/mercator.htm */ | |||
#define AUX_SPHERE_TYPE_SEMI_MAJOR 0 |
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.
Why not use a enum here? You could use a switch below instead of the else if's. I think that is cleaner.
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.
Done
I really tried to implement ESRI stuff, which as far as I can see (from the doc ! not tested against Arc), is general mercator with all the the bells and whistles, and the ability to define how to compute the spherical radius |
Yeah, I figured that much. I am not opposed to it either, just trying to make sure we handle this as best as possible. Do you know of any other case, apart from the webmercator, where this is used? |
src/PJ_merc.c
Outdated
#define AUX_SPHERE_TYPE_SEMI_MAJOR 0 | ||
#define AUX_SPHERE_TYPE_SEMI_MINOR 1 | ||
#define AUX_SPHERE_TYPE_AUTHALIC_RADIUS 2 | ||
/* Unimplemented #define AUX_SPHERE_TYPE_AUTHALIC_LATITUDE 3 */ |
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.
healpix gives some hints as to how to implement this: https://github.com/OSGeo/proj.4/blob/master/src/PJ_healpix.c#L225-L246
51df307
to
6596bad
Compare
No... Perhaps @melitakennedy could comment on the use cases for auxiliary_sphere_type=1/2/3 for the Mercator projection ? |
src/PJ_merc.c
Outdated
@@ -76,3 +119,22 @@ PJ *PROJECTION(merc) { | |||
return P; | |||
} | |||
|
|||
PJ *PROJECTION(webmerc) { | |||
|
|||
if( pj_param(P->ctx, P->params, "tk_0").i ) { |
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.
This behaviour is different to the rest of PROJ. Usually you are allowed to set a parameter even if it is not used. If it is not used by the projection it will just get ignored. proj
will tell you if that is the case:
$ proj -v +proj=merc +lat_1=23
#Mercator
# Cyl, Sph&Ell
# lat_ts=
# +proj=merc +ellps=WGS84
#--- following specified but NOT used
# +lat_1=23
I think it would be better to keep this "tradition". Just make sure to override them in the setup with P->k0=1.0; P->lam0=0.0
.
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.
I kept the ones for k_0 and lat_0 since those are done in generic parts. I remove the one for lat_ts (which was wrongly spelled in my original commit)
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.
I understand why you would want to do that but I still don't think it is the correct way to handle it. I think it will cause more confusion than is necessary. This would be the only place in the code base where using one of the default projection parameters would cause an error. There are plenty examples of overriding default parameters in the setup without raising errors.
I think it would be awesome if we had a system that caused errors when using parameters that can't be used with a given operation but that is a problem for another day (and major version release :-) ).
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.
OK, checks reverted and parameters overriden
accept 487147.594520173 4934316.46263998 0 | ||
expect -10370728.80 5552839.74 0 | ||
------------------------------------------------------------------------------- | ||
|
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.
How about some tests for aux_sphere_type
== 1 and 2?
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.
Added, by the way while adding them I had originally issues with the z component being modified, which I think was wrong. I fixed that issue with commit ac78787 which I believe is a bugfix that should probably be applied to the 5.0 branch
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.
Added, by the way while adding them I had originally issues with the z component being modified, which I think was wrong.
Do you mind writing that up in an issue so it can be referred to in the release notes later on?
I fixed that issue with commit 1a575b6 which I believe is a bugfix that should probably be applied to the 5.0 branch
I don't plan on doing more bug fix release for the 5.0 branch. 5.1 will be out in two months. I think that will be fine.
Another thing, if the |
I'm not completely sure. GDAL should probably not parse itself the file, and QGIS probably not too. But the epsg file is generated by GDAL that for now will still generate the old hacky definition of web mercator. And we should expect that old hacky definition to be still passed for years ! |
By the way: this had apparently no impact on spherical Mercator but pj_calc_ellipsoid_params() behaviour is quite surprising: it doesn't recompute e, f and b is they are originally non zero ! I see that in ellps_spherification() there is a 'workaround' for that which I copied, but this feels messy |
… original ellipsoid parameters (before any projection mess with them)
c90bb74
to
7072ba4
Compare
Alright. Do you plan on updating GDAL so it generates the epsg file using
It does indeed. I wonder what the reason for that is. Can you answer that @busstoptaktik ? It doesn't affect the tests (apart from one minor detail in the gie |
Potentially, the exportToProj4() code could, once proj 5.1 is released and detected at runtime, export EPSG:3857 as webmerc.
The hack is only in the pj_transform() legacy interface where WGS84 has always (and will remain until we remove it) the pivot datum, so I don't think that's a big issue. |
That's true. I was thinking about the |
Although I believe "web Mercator" was the instigator, Esri added several "Auxiliary Sphere" versions of existing projections. At the time, we were trying to match results from NGA's GEOTRANS and ERDAS. They both had several projections that were implemented as sphere-only, but Esri had ellipsoidal versions. One or the other (I don't remember which) would internally calculate the authalic radius and use that. Esri's default is to use the semimajor axis. The developer, David Burrows, pointed out that for an equal-area sphere-only projection, calculating the authalic radius is not quite enough. The geodetic latitudes should be converted to authalic latitudes as well. As far as I know, no one using any of this except Mercator aux sphere. |
@melitakennedy Thanks for your insight. It sounds to me the "Auxiliary Sphere" doesn't bring anything new to PROJ. We already have both ellipsoidal and spherical versions of the Mercator projection and with the introduction of @rouault Based on the above I suggest removing the aux. sphere stuff. Do you agree with that sentiment? |
OK, I've just kept the new webmerc and dropped the aux_sphere_type option of merc |
Thanks! |
Directly based on the auxiliary sphere type parameter of
http://desktop.arcgis.com/en/arcmap/10.3/guide-books/map-projections/mercator.htm
Implemented:
Unimplemented: