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

What's the purpose of proj_obj_create_projected_crs_* functions? #1184

Closed
kbevers opened this issue Nov 28, 2018 · 8 comments
Closed

What's the purpose of proj_obj_create_projected_crs_* functions? #1184

kbevers opened this issue Nov 28, 2018 · 8 comments
Labels

Comments

@kbevers
Copy link
Member

kbevers commented Nov 28, 2018

I am slowly finding my way through the massive amounts of new code that was introduced in #1175 and find myself wondering what the purpose of having projection specific creation functions in the C API? Example of said functions here:

https://github.com/OSGeo/proj.4/blob/4794d755a8dea4f4501c61e896e1829bb720e69a/src/proj.h#L573-L591

To me it seems to go in the complete opposite direction of the established practice in PROJ by setting up the transformation object by means of a proj string. It also seems that there are proj_obj_create_* functions that follow that practice. So, how does a call to proj_obj_create_projected_crs_TransverseMercator() differ from a call to proj_obj_create_from_proj_string() with a +proj=etmerc ... proj-string as input?

Ping @rouault :-)

@rouault
Copy link
Member

rouault commented Nov 28, 2018

Those functions are for the purpose of being able to replace the GDAL code that instanciates projected CRS and directly creates WKT nodes. For example https://github.com/OSGeo/gdal/blob/master/gdal/ogr/ogrspatialreference.cpp#L5834

It is better to use setters that are close to the object modelling.
And those setters help to correctly instanciate projections by mentioning expected parameter names.
One downside of using proj strings is that you must specify linear parameters in meters and angular parameters in degrees, whereas the ISO modelling allows to specify them in any unit (generally the unit of the underlying coordinate system)

Note: in my current tree, I've reworked them to only instantiate a Conversion object, but that doesn't fundamentally change things.

@kbevers
Copy link
Member Author

kbevers commented Nov 28, 2018

We haven't really had the discussion, so let me take the role of the devils advocate for a minute:

Those functions are for the purpose of being able to replace the GDAL code that instanciates projected CRS and directly creates WKT nodes

Wouldn't GDAL be able to use the PROJ C++ API and be able to do the same thing? Or maybe the question is, why is GDAL internals PROJ's problem to solve?

A quick count of the proj_obj_create_projected_crs_* functions tells me that only about half of the projections in PROJ has been mapped with functions like this. Why not all of them?

One downside of using proj strings is that you must specify linear parameters in meters and angular parameters in degrees, whereas the ISO modelling allows to specify them in any unit (generally the unit of the underlying coordinate system)

Surely we could fix that if we wanted to (not necessarily a simple job, I know!).

I acknowledge that it may be necessary to include these functions in the C API but I don't appreciate the extra complexity that comes with it. The whole point of proj.h when first introduced was to create a simple and no-nonsense API that was easy to maintain. ~600 lines of boilerplate header file ruins that somewhat...

@kbevers
Copy link
Member Author

kbevers commented Nov 28, 2018

Another thing, why the mixed snake_case and camelCase?

proj_obj_create_projected_crs_transverse_mercator (as compared to proj_obj_create_projected_crs_TransverseMercator) would be more in line with the rest of the API.

@rouault
Copy link
Member

rouault commented Nov 28, 2018

Wouldn't GDAL be able to use the PROJ C++ API and be able to do the same thing?

Sure it could, but transitionally we have used the C API for a lot of historical reasons (ABI issues mostly). And other users such as QGIS also do.

A quick count of the proj_obj_create_projected_crs_* functions tells me that only about half of the projections in PROJ has been mapped with functions like this. Why not all of them?

Those are the ones currently used by GDAL. And a good subset of EPSG conversion methods. This is already an enormous task... The rest of PROJ conversion methods are more esoteric and do not have an official EPSG WKT mapping, so I've let them aside. For those users have to fallback to PROJ strings or using a adhoc WKT mapping like

$ src/projinfo "+proj=bertin1953 +x_0=1000"
PROJ string: 
+proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad +step +proj=bertin1953 +x_0=1000 +ellps=WGS84

WKT2_2015 string: 
PROJCRS["unknown",
    BASEGEODCRS["unknown",
        DATUM["World Geodetic System 1984",
            ELLIPSOID["WGS 84",6378137,298.257223563,
                LENGTHUNIT["metre",1]],
            ID["EPSG",6326]],
        PRIMEM["Greenwich",0,
            ANGLEUNIT["degree",0.0174532925199433],
            ID["EPSG",8901]]],
    CONVERSION["unknown",
        METHOD["PROJ bertin1953"],
        PARAMETER["x_0",1000,
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]]],
    CS[Cartesian,2],
        AXIS["(E)",east,
            ORDER[1],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]],
        AXIS["(N)",north,
            ORDER[2],
            LENGTHUNIT["metre",1,
                ID["EPSG",9001]]]]

I acknowledge that it may be necessary to include these functions in the C API but I don't appreciate the extra complexity that comes with it

We could perhaps move all the creators / setters, that are mostly of interest for GDAL or other packages that need to do advanced manipulations of objects, in a dedicated section of proj.h

Another thing, why the mixed snake_case and camelCase?

This is just because this is autogenerated from the C++ methods. But I guess I could auto-fix that

@kbevers
Copy link
Member Author

kbevers commented Nov 28, 2018

Sure it could, but transitionally we have used the C API for a lot of historical reasons (ABI issues mostly). And other users such as QGIS also do.

The prime reason being that there's only been a C API up until now, yes? :-) I would have thought that using the C++ API was the preferred approach when using PROJ in C++ based applications.

Those are the ones currently used by GDAL. And a good subset of EPSG conversion methods. This is already an enormous task...

I think we should aim for completeness if we're going to have functions like this. I can help with the rest if needed. Would be educational for me.

The rest of PROJ conversion methods are most esoteric and do not have an official EPSG WKT mapping, so I've let them aside. For those users have to fallback to PROJ strings or using a adhoc WKT mapping like

It seems reasonable to me to use an ad hoc WKT mapping in this case.

We could perhaps move all the creators / setters, that are mostly of interest for GDAL or other packages that need to do advanced manipulations of objects, in a dedicated section of proj.h

It more or less already is in it's own section. Could be moved to the absolute bottom of the file to make it a bit easier to find the functions defined in the last ~300 lines of proj.h.

This is just because this is autogenerated from the C++ methods. But I guess I could auto-fix that

Yes, please! Consistent naming is easier to remember and is easier on the eyes.

@rouault
Copy link
Member

rouault commented Nov 28, 2018

I would have thought that using the C++ API was the preferred approach when using PROJ in C++ based applications.

On the other hand, having GDAL use the PROJ C API is a good way to show it is useful for real-world usage... And I'm not sure to which extent GRASS will use the new API, but they are C only.

I think we should aim for completeness if we're going to have functions like this.

Hum, I'd think on the contrary it is urgent to defer on this ! Doing our own ad-hoc mapping to WKT will not help interoperability. If you really want to pursue on this, the best option would be to provide an official mapping to WKT that would be accepted by the EPSG dataset. Once that is done, we can consider having dedicate setters in the API.

Could be moved to the absolute bottom of the file

I'll probably make a section with the high-level functions (getters, import/export from/to WKT/PROJString), and another one with the other setters

This is just because this is autogenerated from the C++ methods. But I guess I could auto-fix that
Yes, please!

will do. But I've already a lot of new changes in my branch, so that will be on-top of them...

@kbevers
Copy link
Member Author

kbevers commented Nov 28, 2018

On the other hand, having GDAL use the PROJ C API is a good way to show it is useful for real-world usage...

Of course. I am just a bit sad that the simplicity of proj_create() has been adjoined with ~100 new functions that appear to do almost the same thing. I'll get over it soon enough though :-)

And I'm not sure to which extent GRASS will use the new API, but they are C only.

Well, they have already claimed that they were the first project to adopt the PROJ 5 API so I expect them to jump on the new stuff as soon as it's available :-)

If you really want to pursue on this, the best option would be to provide an official mapping to WKT that would be accepted by the EPSG dataset. Once that is done, we can consider having dedicate setters in the API.

What's the governing body for this? ISO TC211? EPSG?

I'll probably make a section with the high-level functions (getters, import/export from/to WKT/PROJString), and another one with the other setters

Cool.

will do. But I've already a lot of new changes in my branch, so that will be on-top of them...

Sure, very reasonable :-)

@rouault
Copy link
Member

rouault commented Nov 28, 2018

What's the governing body for this? ISO TC211? EPSG?

That's a good question... Probably that Roger Lott would know

@kbevers kbevers closed this as completed Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants