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

Initial version of WKT2 support and ISO-19111:2018 / OGC Topic 2 #1040

Open
wants to merge 184 commits into
base: iso19111_dev
from

Conversation

Projects
None yet
3 participants
@rouault
Member

rouault commented Jun 8, 2018

It is known to be incomplete, but this is already enormous to review...
Lots of TODO and FIXME. See TODO.TXT for a non exhaustive list.

Short summary of the main changes file-wise

  • include/proj directory added with the new c++ headers. Public ones are installed in $prefix/include/proj, so as to avoid pollutiong $prefix/include
  • src has the new .cpp files
  • test/unit has new tests
    Everything indented with scripts/reformat_cpp.sh (llvm based with 4 space for indentation)

As discussed with @kbevers , in a preliminary step, that work and following will land in the OSGeo/proj.4/iso19111_dev staging branch (just forked from master), (this PR is against that branch) and when the whole work is ready, it will be merged back to master.
I'm not sure how to deal with git history wise. Probably that I will stack new commits on top of that one for the iso19111_dev branch, and when we will decide to merge it back to master, we'll decide if we squash all the intermediate commits or not depending on how messy this history will be.

@mloskot

This comment has been minimized.

Show comment
Hide comment
@mloskot

mloskot Jun 8, 2018

Member

I see you aim to distinguish C from C++ headers. Good.
Why you prefer .hh instead of .hpp which nicely matches the three-letter .cpp?

Member

mloskot commented Jun 8, 2018

I see you aim to distinguish C from C++ headers. Good.
Why you prefer .hh instead of .hpp which nicely matches the three-letter .cpp?

Show outdated Hide outdated include/proj/common.hh Outdated
Show outdated Hide outdated src/internal.cpp Outdated
//! @cond Doxygen_Suppress
struct Citation::Private {
optional<std::string> title{};

This comment has been minimized.

@mloskot

mloskot Jun 8, 2018

Member

title{}; is redundant I think, just title;

@mloskot

mloskot Jun 8, 2018

Member

title{}; is redundant I think, just title;

This comment has been minimized.

@rouault

rouault Jun 8, 2018

Member

This is needed to make -Weffc++ happy.

@rouault

rouault Jun 8, 2018

Member

This is needed to make -Weffc++ happy.

Show outdated Hide outdated src/metadata.cpp Outdated
Show outdated Hide outdated include/proj/util.hh Outdated
@kbevers

This is a lot to grasp so I have only looked it over briefly and it looks good to me. It seems like the overall structure is good and that is probably the most important thing at this point. Very impressed with how much you've manage to do already!

Show outdated Hide outdated include/proj/coordinateoperation.hpp Outdated
@mloskot

This comment has been minimized.

Show comment
Hide comment
@mloskot

mloskot Jun 9, 2018

Member

Even, this is a huge and impressive overhaul indeed. It's clear the stage is very early and please don't consider my question as high priority, ignore them if answering would be too much effort.
What is relation of this implementation to the ISO 19111; what is different, what is improved, what does your implementation do not cover, etc. Basically some bit of documenting background and rationale would be helpful during reviews. Especially for those who do not analyse the ISO document thoroughly (I'd assume it's majority of reviewers).

Member

mloskot commented Jun 9, 2018

Even, this is a huge and impressive overhaul indeed. It's clear the stage is very early and please don't consider my question as high priority, ignore them if answering would be too much effort.
What is relation of this implementation to the ISO 19111; what is different, what is improved, what does your implementation do not cover, etc. Basically some bit of documenting background and rationale would be helpful during reviews. Especially for those who do not analyse the ISO document thoroughly (I'd assume it's majority of reviewers).

@rouault

This comment has been minimized.

Show comment
Hide comment
@rouault

rouault Jun 9, 2018

Member

@mloskot Yes I can imagine that it is hard to make sense of this without looking at the ISO-19111/OGC Topic 2 doc. At some point I'll add some documentation notes to each class to relate the C++ implementation with the standard (or document the departure). But I won't repeat the standard document, so I think readers will have to look at the UML schemas of the standard (or the hiearchy diagram generated by Doxygen) to get the whole picture.

But at that point of the implementation, there isn't much departure; this is really close. There will probably be some to model WKT2 BoundCRS concept, generalization of TOWGS84, that has no direct equivalent in the 19111 modelization. There are likely simplifications in the metadata/common packages regarding to other OGC/ISO standards since I didn't study them closely, but just put the strict minimum needed for the CRS / CoordinateSystem / CoordinateOperation packages.

Anyway I don' think most users will have to directly deal with this level of modelling. They will be likely deal with utility classes that offer higher level services: like import/export from/to WKT1,WKT2,PROJ strings, EPSG database. And the actual C++ objects will mostly be opaque to users.

Member

rouault commented Jun 9, 2018

@mloskot Yes I can imagine that it is hard to make sense of this without looking at the ISO-19111/OGC Topic 2 doc. At some point I'll add some documentation notes to each class to relate the C++ implementation with the standard (or document the departure). But I won't repeat the standard document, so I think readers will have to look at the UML schemas of the standard (or the hiearchy diagram generated by Doxygen) to get the whole picture.

But at that point of the implementation, there isn't much departure; this is really close. There will probably be some to model WKT2 BoundCRS concept, generalization of TOWGS84, that has no direct equivalent in the 19111 modelization. There are likely simplifications in the metadata/common packages regarding to other OGC/ISO standards since I didn't study them closely, but just put the strict minimum needed for the CRS / CoordinateSystem / CoordinateOperation packages.

Anyway I don' think most users will have to directly deal with this level of modelling. They will be likely deal with utility classes that offer higher level services: like import/export from/to WKT1,WKT2,PROJ strings, EPSG database. And the actual C++ objects will mostly be opaque to users.

@mloskot

This comment has been minimized.

Show comment
Hide comment
@mloskot

mloskot Jun 9, 2018

Member

@rouault

(...) so I think readers will have to look at the UML schemas of the standard

This is fair play.

But at that point of the implementation, there isn't much departure; this is really close

This is an important note and it should be motivating for reviewers to actually gain deeper familiarity with the standard.

Those who don't, because they can't or don't want to, may still act as extra pair of debugging eyes and review the low level C++ aspects of the implementation.

Member

mloskot commented Jun 9, 2018

@rouault

(...) so I think readers will have to look at the UML schemas of the standard

This is fair play.

But at that point of the implementation, there isn't much departure; this is really close

This is an important note and it should be motivating for reviewers to actually gain deeper familiarity with the standard.

Those who don't, because they can't or don't want to, may still act as extra pair of debugging eyes and review the low level C++ aspects of the implementation.

@@ -346,6 +358,9 @@ class BoundCRS : public CRS {
const operation::TransformationNNPtr &transformationIn);
INLINED_MAKE_SHARED
bool isTOWGS84Compatible() const;
std::string getVDatumPROJ4GRIDS() const;

This comment has been minimized.

@kbevers

kbevers Jun 24, 2018

Member

Is there a particular reason for using including the "PROJ4" name here? Why not getVDatumPROJGRIDS or simply getVDatumGrids?

@kbevers

kbevers Jun 24, 2018

Member

Is there a particular reason for using including the "PROJ4" name here? Why not getVDatumPROJGRIDS or simply getVDatumGrids?

This comment has been minimized.

@rouault

rouault Jun 24, 2018

Member

Yes, that's the name used by the GDAL WKT1 EXTENSION. The purpose here is to be compatible with the past.

VERT_CS["EGM2008 geoid height",
    VERT_DATUM["EGM2008 geoid",2005,
        EXTENSION["PROJ4_GRIDS","egm08_25.gtx"],
        AUTHORITY["EPSG","1027"]],
    UNIT["metre",1,
        AUTHORITY["EPSG","9001"]],
    AXIS["Up",UP],
    AUTHORITY["EPSG","3855"]]

The equivalent WKT2 will be

BOUNDCRS[
    SOURCECRS[
        VERTCRS["EGM2008 geoid height",
            VDATUM["EGM2008 geoid"],
            CS[vertical,1],
                AXIS["up",up,
                    LENGTHUNIT["metre",1]],
            ID["EPSG",3855]]],
    TARGETCRS[
        GEODCRS["WGS 84",
            DATUM["WGS_1984",
                ELLIPSOID["WGS 84",6378137,298.257223563,
                    LENGTHUNIT["metre",1]]],
            PRIMEM["Greenwich",0,
                ANGLEUNIT["degree",0.0174532925199433]],
            CS[ellipsoidal,3],
                AXIS["latitude",north,
                    ORDER[1],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["longitude",east,
                    ORDER[2],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["ellipsoidal height",up,
                    ORDER[3],
                    LENGTHUNIT["metre",1]],
            ID["EPSG",4979]]],
    ABRIDGEDTRANSFORMATION["EGM2008 geoid height height to WGS84 ellipsoidal height",
        METHOD["GravityRelatedHeight to Geographic3D"],
        PARAMETERFILE["Geoid (height correction) model file","egm08_25.gtx",
            ID["EPSG",8666]]]]
@rouault

rouault Jun 24, 2018

Member

Yes, that's the name used by the GDAL WKT1 EXTENSION. The purpose here is to be compatible with the past.

VERT_CS["EGM2008 geoid height",
    VERT_DATUM["EGM2008 geoid",2005,
        EXTENSION["PROJ4_GRIDS","egm08_25.gtx"],
        AUTHORITY["EPSG","1027"]],
    UNIT["metre",1,
        AUTHORITY["EPSG","9001"]],
    AXIS["Up",UP],
    AUTHORITY["EPSG","3855"]]

The equivalent WKT2 will be

BOUNDCRS[
    SOURCECRS[
        VERTCRS["EGM2008 geoid height",
            VDATUM["EGM2008 geoid"],
            CS[vertical,1],
                AXIS["up",up,
                    LENGTHUNIT["metre",1]],
            ID["EPSG",3855]]],
    TARGETCRS[
        GEODCRS["WGS 84",
            DATUM["WGS_1984",
                ELLIPSOID["WGS 84",6378137,298.257223563,
                    LENGTHUNIT["metre",1]]],
            PRIMEM["Greenwich",0,
                ANGLEUNIT["degree",0.0174532925199433]],
            CS[ellipsoidal,3],
                AXIS["latitude",north,
                    ORDER[1],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["longitude",east,
                    ORDER[2],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["ellipsoidal height",up,
                    ORDER[3],
                    LENGTHUNIT["metre",1]],
            ID["EPSG",4979]]],
    ABRIDGEDTRANSFORMATION["EGM2008 geoid height height to WGS84 ellipsoidal height",
        METHOD["GravityRelatedHeight to Geographic3D"],
        PARAMETERFILE["Geoid (height correction) model file","egm08_25.gtx",
            ID["EPSG",8666]]]]

This comment has been minimized.

@kbevers

kbevers Jun 24, 2018

Member

Ah, okay. Is the function going to be used to create both WKT1 and WKT2 strings? Just wondering if we'll be stuck with a hardcoded version number in a (API?) function that will stay around for a long time.

@kbevers

kbevers Jun 24, 2018

Member

Ah, okay. Is the function going to be used to create both WKT1 and WKT2 strings? Just wondering if we'll be stuck with a hardcoded version number in a (API?) function that will stay around for a long time.

This comment has been minimized.

@rouault

rouault Jun 24, 2018

Member

The function is mostly to re-create GDAL WKT1 string from the internal model (using BoundCRS approach internally). And I've also used it for now to regenerate PROJ strings using the +geoidgrids params, but that might change later to use pipeline and +proj=vgridshift

@rouault

rouault Jun 24, 2018

Member

The function is mostly to re-create GDAL WKT1 string from the internal model (using BoundCRS approach internally). And I've also used it for now to regenerate PROJ strings using the +geoidgrids params, but that might change later to use pipeline and +proj=vgridshift

@rouault

@kbevers Might be of interest for you. With that changeset

$ PROJ_LIB=data  src/projinfo  -s EPSG:8267 -t EPSG:4909
PROJ string: 
+proj=vgridshift +grids=gvr2016.gtx

WKT2_2015 string: 
COORDINATEOPERATION["Inverse of GR96 to GVR2016 height (1)",
    SOURCECRS[
        VERTCRS["GVR2016 height",
            VDATUM["Greenland Vertical Reference 2016"],
            CS[vertical,1],
                AXIS["gravity-related height (H)",up,
                    LENGTHUNIT["metre",1]]]],
    TARGETCRS[
        GEODCRS["GR96",
            DATUM["Greenland 1996",
                ELLIPSOID["GRS 1980",6378137,298.257222101,
                    LENGTHUNIT["metre",1]]],
            PRIMEM["Greenwich",0,
                ANGLEUNIT["degree",0.0174532925199433]],
            CS[ellipsoidal,3],
                AXIS["geodetic latitude (Lat)",north,
                    ORDER[1],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["geodetic longitude (Lon)",east,
                    ORDER[2],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["ellipsoidal height (h)",up,
                    ORDER[3],
                    LENGTHUNIT["metre",1]]]],
    METHOD["Geographic3D to GravityRelatedHeight (Gravsoft)",
        ID["EPSG",1047]],
    PARAMETERFILE["Geoid (height correction) model file","gvr2016.gtx"],
    OPERATIONACCURACY[0.1],
    AREA["Greenland - 58°N to 85°N"],
    BBOX[58,-75,85.01,-6.99],
    ID["INVERSE(DERIVED_FROM(EPSG))",8269]]
$ PROJ_LIB=data  src/projinfo  -s EPSG:5733 -t EPSG:4937
PROJ string: 
+proj=vgridshift +grids=dnn.gtx

WKT2_2015 string: 
COORDINATEOPERATION["DNN height to ETRS89",
    SOURCECRS[
        VERTCRS["DNN height",
            VDATUM["Dansk Normal Nul"],
            CS[vertical,1],
                AXIS["gravity-related height (H)",up,
                    LENGTHUNIT["metre",1]]]],
    TARGETCRS[
        GEODCRS["ETRS89",
            DATUM["European Terrestrial Reference System 1989",
                ELLIPSOID["GRS 1980",6378137,298.257222101,
                    LENGTHUNIT["metre",1]]],
            PRIMEM["Greenwich",0,
                ANGLEUNIT["degree",0.0174532925199433]],
            CS[ellipsoidal,3],
                AXIS["geodetic latitude (Lat)",north,
                    ORDER[1],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["geodetic longitude (Lon)",east,
                    ORDER[2],
                    ANGLEUNIT["degree",0.0174532925199433]],
                AXIS["ellipsoidal height (h)",up,
                    ORDER[3],
                    LENGTHUNIT["metre",1]]]],
    METHOD["GravityRelatedHeight to Geographic3D",
        ID["PROJ","HEIGHT_TO_GEOGRAPHIC3D"]],
    PARAMETERFILE["Geoid (height correction) model file","dnn.gtx"],
    AREA["Denmark - onshore"],
    BBOX[54.51,8,57.8,15.24],
    ID["PROJ","EPSG_5733_TO_EPSG_4937"]]
@rouault

@busstoptaktik I've hopefully fixed the whitespaces issues in proj.h

rouault added some commits May 25, 2018

Initial version of WKT2 support and ISO-19111:2018 / OGC Topic 2
It is known to be incomplete. Lots of TODO and FIXME.
See TODO.TXT for a non exhaustive list.

rouault added some commits Oct 12, 2018

Add a text_definition column to geodetic_crs and projected_crs to all…
…ow definition by PROJ string (or WKT); add a createFromUserInput(); add a celestial_body table
Allow navigation from CRS to its 'canonical' BoundCRS; allow +towgs84…
… to be put in text_definition column of database
Implement DerivedParametricCRS and DerivedTemporalCRS, to finally ach…
…ieve the full ISO-19111:2018 CRS modelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment