Pipeline plus api - in continuation of the Genereic Coordinates pull request #445

busstoptaktik · 2016-10-30T13:16:01Z

This PR continues the work on a rationalized (i.e. better namespaced) API, and also, as another step down the road to implementing transformation pipelines, reintrodices the PJ_cart.c cartesian/ellipsoidal converter, previously discussed in a former PR.

Regarding the API improvements, the full story is in the comments in proj.h, which I, for completeness, include below:

       Purpose:  Revised, experimental API for PROJ.4, intended as the foundation
       for added geodetic functionality.

       The original proj API (defined in projects.h) has grown organically
       over the years, but it has also grown somewhat messy.

       The same has happened with the newer high level API (defined in
       proj_api.h): To support various historical objectives, proj_api.h
       contains a rather complex combination of conditional defines and
       typedefs. Probably for good (historical) reasons, which are not
       always evident from today's perspective.

       This is an evolving attempt at creating a re-rationalized API
       with primary design goals focused on sanitizing the namespaces.
       Hence, all symbols exposed are being moved to the pj_ namespace,
       while all data types are being moved to the PJ_ namespace.

       Please note that this API is *orthogonal* to  the previous APIs:
       Apart from some inclusion guards, projects.h and proj_api.h are not
       touched - if you do not include proj.h, the projects and proj_api
       APIs should work as they always have.

       A few implementation details:

       Apart from the namespacing efforts, I'm trying to eliminate three
       proj_api elements, which I have found especially confusing.

       FIRST and foremost, I try to avoid typedef'ing away pointer
       semantics. I agree that it can be occasionally useful, but I
       prefer having the pointer nature of function arguments being
       explicitly visible.

       Hence, projCtx has been replaced by PJ_CONTEXT *.

       SECOND, I try to eliminate cases of information hiding implemented
       by redefining data types to void pointers.

       I prefer using a combination of forward declarations and typedefs.
       Hence:
           typedef void *projCtx;
       Has been replaced by:
           struct projCtx_t;
           typedef struct projCtx_t PJ_CONTEXT;
       This makes it possible for the calling program to know that the
       PJ_CONTEXT data type exists, and handle pointers to that data type
       without having any idea about its internals.

       (obviously, in this example, struct projCtx_t should also be moved
       to struct pj_ctx some day...)

       THIRD, I try to eliminate implicit type punning. Hence this API
       introduces the OBSERVATION data type, for generic coordinate and
       ancillary data handling.

       It includes the PJ_SPATIOTEMPORAL and PJ_TRIPLET unions
       making it possible to make explicit the previously used
       "implicit type punning", where a XY is turned into a LP by
       re#defining both as UV, behind the back of the user.

       The bare essentials API presented here follows the PROJ.4
       convention of sailing the coordinate to be reprojected, up on
       the stack ("call by value"), and symmetrically returning the
       result on the stack. Although the OBSERVATION object is 4 times
       as large as the traditional XY and LP objects, timing results
       have shown the overhead to be very reasonable.

       See pj_proj_test.c for an example of how to use the API.

       Author:   Thomas Knudsen, <thokn@sdfe.dk>

This commit reflects continued work on the new rationalized API with geodetic extensions (see rationale in proj.h). It also reflects the parallel work on the transformation pipeline reimplementation, by introducing the PJ_cart cartesian/ellipsoidal converter.

rouault · 2016-10-30T13:28:31Z

src/proj.h

 int pj_show_triplet (FILE *stream, const char *banner, PJ_TRIPLET point);

-/* Constructor for the OBSERVATION object */
+PJ *pj_pj (PJ_CONTEXT *ctx, char *definition);


const char* would be better

pj_pj() isn't really explicit. pj_create_from_def() maybe ?

This is a more general question, but how should we document functions and structures. Informally here as comments, or with Doxygen formalism in the .c file (or in .h) ? Update: I see we have https://github.com/OSGeo/proj.4/tree/master/docs/source/development now, so that would be the place for that

I agree with 1 and 2. Even though the most obvious name is occupied it must be possible to come out with something more explicit thapj_pj. pj_setup maybe?
3. Yes, I added that recently but I am not happy with it yet. For now a proj_h.rst or something like that would be good enough. I think eventually we should use a system like Doxygen.

I agree with 1 & 2 and changed the char * arg to const char *, and the name to pj_create which I believe strikes a balance between brevity and expressivity :-)

rouault · 2016-10-30T13:29:50Z

src/proj.h

+
+/* This is mostly a direct copy of (parts of) the proj_api header, but with cleaned up name space */
+
+#define PJ_VERSION 493


This is a bit error prone to have PJ_VERSION defines in 2 header files (but we probably don't want to have a proj_version.h with just that #define ?). HOWTO-RELEASE will have to be updated.

I have changed it now, so it lives in proj_api.h only - but making it work requires some ugly defines.

rouault · 2016-10-30T13:39:15Z

src/Makefile.am

@@ -74,7 +74,7 @@ libproj_la_SOURCES = \
 	jniproj.c pj_mutex.c pj_initcache.c pj_apply_vgridshift.c geodesic.c \
 	pj_strtod.c \
 	\
-	pj_observation.c
+	pj_observation.c pj_cart.c


Not sure if makefiles are filename case sensitive. But if so, should be PJ_cart.c.
Or should PJ_cart.c be in lowercase itself? (I tend to prever lower case for filenames but proj is a mix of both...)

Keeping with the tradition the file should be PJ_cart.c since all files with "projections" are named that way, and the rest uses the lower-cases nomenclature. I don't like it either (I'd prefer to organise the files in directories instead).

Yes, that was a bug. Actually, I think that 8 years ago, Gerald had planned to move all projection files into a proj_NAME.c namespace (cf. proj_etmerc.c and proj_rouss.c)

Doing that would also make it way easier to find the right file in a lexicographically ordered file open dialog.

But it would probably make it slightly harder to follow the development history of the code in git.

rouault · 2016-10-30T13:40:22Z

src/pj_proj_test.c

+    printf ("%15.9f %15.9f\n", a.coo.lpz.lam, a.coo.lpz.phi);
+    err = pj_ctx_get_errno (pj_ctx (p));
+    printf ("pj_ctx_errno: %d\n", err);
+    return 0;


Would be a good practice to free p

Done - thanks for spotting that!

rouault · 2016-10-30T13:45:05Z

src/proj_api.h

@@ -49,20 +49,21 @@ extern "C" {
 *   are padded with leading zeros if necessary); e.g., PJ_VERSION=401000
 *   for version 4.10.0.
 */
+#ifndef PROJ_H


It seems that there's a provision to do #include "proj.h" and then #include "proj_api.h", but the reverse wouldn't work well, right ?

I have now added inclusion guards - they are implemented using the "error" preprocessor directive, which is not universally supported, but have the strange property that it is supported even where not - since if it is not supported it magically turns into a conditional syntax error

rouault · 2016-10-30T13:52:24Z

src/proj.h

+PJ_FILEAPI *pj_ctx_get_fileapi (PJ_CONTEXT *);
+PJ_FILEAPI *pj_get_default_fileapi();
+
+PJ_FILE  pj_ctx_fopen  (PJ_CONTEXT *ctx, const char *filename, const char *access);


Do we need those pj_ctx_fopen() and the like in the public API ? Looks like they should be used only by the proj.4 implementation

If I remember Frank's explanation when they were introduced, they were actually intended as user accessible, for making the proj.4 library play nice with other environments.

Personally I wouldn't mind moving them to a lower level API, but I do not think thay are intended as proj.4 internals (on the other hand, I have never seen them in use...)

This is Franks introduction to the context file API. He mentions that it is used in the init and gridshift file accessors. I am not sure if that is actually true since I am having a hard time finding said code. I am not very familiar with that part of the code base though, so it is likely that I have missed something.

I wouldn't be surprised if no one has ever used that functionality.

rouault · 2016-10-30T13:53:14Z

src/proj.h

+
+
+/* Logging API */
+void pj_log (PJ_CONTEXT *ctx, int level, const char *fmt, ... );


pj_log() probably doesn't need to be public

It has been up until now, but I agree that keeping things compact from start would be a good idea

rouault · 2016-10-30T13:56:24Z

src/pj_observation.c

+    return pj_init_ctx (ctx, argc, argv);
+}
+
+PJ_CONTEXT *pj_ctx (PJ *P) {


Why would one need to get the context of a PJ* object ?

You can do the same with the API in proj_api.h as documentend here: http://proj4.org/development/threads.html
This, and the one below, is just to get consistent naming across all pj_ctx*functions. I am not really able to comment on why you would need that functionality though...

Probably in order to pass it on to pj_init_ctx in order to initialize another PJ running in the same thread?

rouault · 2016-10-30T13:56:31Z

src/pj_observation.c

+    return pj_get_ctx (P);
+}
+
+void pj_ctx_set (PJ *P, PJ_CONTEXT *ctx) {


Why would one need to set the context of a PJ* object ?

Probably reason closely related to the above mentioned. Basically I have never seen it in use.

This might be another of the "drop them now, and reintroduce if necessary later" crowd

rouault · 2016-10-30T14:02:26Z

src/proj.h

+void     pj_ctx_fclose (PJ_CONTEXT *ctx, PJ_FILE *file);
+char    *pj_ctx_fgets  (PJ_CONTEXT *ctx, char *line, int size, PJ_FILE *file);
+
+PJ_FILE  pj_open_lib(PJ_CONTEXT *, const char *, const char *);


I know this isn't a new function that you added, but the naming is quite bad. It doesn't open the lib, but a file.
OK so if we keep that one public (could potentially be useful if people wants to parse the 'epsg' file at hand?), we need to keep the fread(), fseek() etc public as well

The name is horrible... but I think this API should live its life inside the library and evolve a bit on the trip to the next release, and for a new API we do not need to keep the old names.

I will probably remove some of these, and reintroduce them when need (and better ideas) arise

kbevers

My initial comments are inlined. I'll probably have more after my second read-through of the code. In general it looks good.
The build fails that obviously has to change before merging...

kbevers · 2016-10-30T15:45:31Z

src/pj_observation.c

+};
+
+PJ_OBSERVATION pj_fwdobs (PJ_OBSERVATION obs, PJ *P) {
+#if 0


I guess this should have been removed?

kbevers · 2016-10-30T15:45:49Z

src/pj_observation.c

+}
+
+PJ_OBSERVATION pj_invobs (PJ_OBSERVATION obs, PJ *P) {
+#if 0


Same as the above

kbevers · 2016-10-30T15:51:44Z

src/proj.h

@@ -116,20 +205,17 @@ typedef struct { double lam, phi,  z; }  LPZ;
 typedef struct { double d, m,  s; }  PJ_DMS;

 /* Geoid undulation (N) and deflections of the vertical (z, e) */
-typedef struct { double   N,   z,  e; }  PJ_NZE;
+typedef struct { double   z,  e, N; }  PJ_ZEN;


Did you change this just to have a cool name? :)

No, not really - and I have changed it again now to EZN, since eta is the east-west component, zeta the north-south, and N is related to heights, so EZN makes most sense

kbevers · 2016-10-30T16:00:16Z

src/proj.h

 int pj_show_triplet (FILE *stream, const char *banner, PJ_TRIPLET point);

-/* Constructor for the OBSERVATION object */
+PJ *pj_pj (PJ_CONTEXT *ctx, char *definition);


I agree with 1 and 2. Even though the most obvious name is occupied it must be possible to come out with something more explicit thapj_pj. pj_setup maybe?
3. Yes, I added that recently but I am not happy with it yet. For now a proj_h.rst or something like that would be good enough. I think eventually we should use a system like Doxygen.

kbevers · 2016-10-30T16:03:01Z

src/pj_proj_test.c

@@ -0,0 +1,39 @@
+/* Tiny test of an evolving new API. Thomas Knudsen, 2016-10-30 */


Since this file is not really a part of the main library I think it would be better to move it to either the test folder or a new examples. This way it is more clear what is what.

Introduced the examples folder now

kbevers · 2016-10-30T16:06:44Z

src/Makefile.am

@@ -74,7 +74,7 @@ libproj_la_SOURCES = \
 	jniproj.c pj_mutex.c pj_initcache.c pj_apply_vgridshift.c geodesic.c \
 	pj_strtod.c \
 	\
-	pj_observation.c
+	pj_observation.c pj_cart.c


Keeping with the tradition the file should be PJ_cart.c since all files with "projections" are named that way, and the rest uses the lower-cases nomenclature. I don't like it either (I'd prefer to organise the files in directories instead).

kbevers · 2016-10-30T16:22:19Z

src/pj_observation.c

+    return pj_init_ctx (ctx, argc, argv);
+}
+
+PJ_CONTEXT *pj_ctx (PJ *P) {


You can do the same with the API in proj_api.h as documentend here: http://proj4.org/development/threads.html
This, and the one below, is just to get consistent naming across all pj_ctx*functions. I am not really able to comment on why you would need that functionality though...

kbevers · 2016-10-30T22:14:51Z

src/PJ_cart.c

+
+
+static void freeup (PJ *P) {
+    pj_freeup_plain (P);


This function does not appear to be defined anywhere. pj_dealloc would usually be used here. Is this meant to be an alias?

It is defined in pj_malloc.c and has a slightly different prototype than pj_dealloc: It takes a PJ as arg and frees its opaque object. It can be used to simplify/eliminate the freeup code for a large number of projections.

In response to reviews by Even Rouault and Kristian Evers

kbevers · 2016-10-31T11:46:33Z

src/proj.def

+    pj_roundtrip                    @95
+    pj_pj                           @96
+    pj_pj_argv                      @97
+    pj_roundtrip                    @98


defined twice

Almost all references to the ctx subsystem eliminated

pj_context_inherit plus distance operators for two PJ_OBSERVATIONS

kbevers · 2016-11-01T09:26:16Z

@busstoptaktik Your last commit, Minor extensions to new API, adds 1800+ lines of code. That doesn't seem very minor to me :) Was it your intention to add PJ_helmert.c, PJ_pipeline.c and horner.c at this stage? Or were they included by accident?

horner.c PJ_helmert.c PJ_pipeline.c. They will probably be added again in a short time - either to this PR or to an upcomming follow-up.

busstoptaktik · 2016-11-02T16:11:29Z

Definitely a "git add" blunder. Removed now.

Everything accessed through the PJ object

busstoptaktik · 2016-11-03T21:44:51Z

@kbevers - I think I have taken care of all your comments.

@rouault - following your review, and a study by @kbevers revealing the sparsity of use of the context interface, I have now cleaned thing up dramatically, cf. e.g. examples/pj_proj_test.c for some descriptive comments

(Trying to enforce a rebuild with const initializers 1/0 hoping this works for MSVC, which has a non-const HUGE_VAL)

static initialization with HUGE_VAL in MSVC is annoying.

pj_observation.c -> pj_obs_api.c PJ_OBSERVATION -> PJ_OBS PJ_SPATIOTEMPORAL -> PJ_COORD

busstoptaktik · 2016-11-12T15:46:34Z

OK - this PR has been announced on the mailing list (see thread "A re-rationalized API for PROJ.4"), without generating any negative response. It passes all checks, and has "no conflicts with the base branch". I find it ready for merging, and will do so in a moment.

Thanks for reviewing, @kbevers and @rouault - your comments and change requests took the PR in a different, but much better, direction

Thomas Knudsen added 3 commits October 27, 2016 17:20

Merge remote-tracking branch 'refs/remotes/OSGeo/master'

0a2bb13

Merge remote-tracking branch 'refs/remotes/OSGeo/master'

d8ec154

rouault reviewed Oct 30, 2016

View reviewed changes

kbevers requested changes Oct 30, 2016

View reviewed changes

kbevers reviewed Oct 30, 2016

View reviewed changes

A score of, mostly minor, corrections...

0a48962

In response to reviews by Even Rouault and Kristian Evers

kbevers reviewed Oct 31, 2016

View reviewed changes

src/proj.def

pj_roundtrip @95

pj_pj @96

pj_pj_argv @97

pj_roundtrip @98

Copy link

Member

kbevers Oct 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defined twice

Thomas Knudsen added 3 commits October 31, 2016 17:22

Additional simplifications

e147754

Almost all references to the ctx subsystem eliminated

pj_roundtrip support for multiple roundtrips

bb9a7ca

Minor extensions to new API

185973e

pj_context_inherit plus distance operators for two PJ_OBSERVATIONS

remove accidentally added files

665a513

horner.c PJ_helmert.c PJ_pipeline.c. They will probably be added again in a short time - either to this PR or to an upcomming follow-up.

Thomas Knudsen added 3 commits November 2, 2016 20:58

Minimum support for contexts and fileapis

4c66b6c

Everything accessed through the PJ object

Minor API cleanups, major example code improvements

62d5663

Make pj_apply(0, obs) the identity operation

4ab3333

Thomas Knudsen added 3 commits November 7, 2016 15:24

Added comments + test MSVC workaround

3b2dfc8

(Trying to enforce a rebuild with const initializers 1/0 hoping this works for MSVC, which has a non-const HUGE_VAL)

Revert DBL_MAX -> 1/0.

7e88b90

static initialization with HUGE_VAL in MSVC is annoying.

Better type names in new API

283f889

pj_observation.c -> pj_obs_api.c PJ_OBSERVATION -> PJ_OBS PJ_SPATIOTEMPORAL -> PJ_COORD

kbevers approved these changes Nov 11, 2016

View reviewed changes

Thomas Knudsen added 2 commits November 11, 2016 23:11

Clean up inclusion guards in headers

b1b136f

Add selfttest to PJ_cart.c

dc6ae18

busstoptaktik added the C: Core label Nov 12, 2016

busstoptaktik added this to the 4.9.4 milestone Nov 12, 2016

busstoptaktik self-assigned this Nov 12, 2016

Thomas Knudsen added 2 commits November 12, 2016 16:06

Wrong direction bug in selftest corrected

19115e5

A few more inclusion guard simplifications

b8e5727

busstoptaktik merged commit 26e6589 into OSGeo:master Nov 12, 2016

busstoptaktik deleted the pipeline_plus_api branch November 12, 2016 16:22

kbevers modified the milestones: 5.0.0-b, 5.0.0 Feb 1, 2018


		/* This is mostly a direct copy of (parts of) the proj_api header, but with cleaned up name space */

		#define PJ_VERSION 493



		/* Logging API */
		void pj_log (PJ_CONTEXT ctx, int level, const char fmt, ... );

		@@ -0,0 +1,39 @@
		/* Tiny test of an evolving new API. Thomas Knudsen, 2016-10-30 */

Pipeline plus api - in continuation of the Genereic Coordinates pull request #445

Pipeline plus api - in continuation of the Genereic Coordinates pull request #445

Conversation

busstoptaktik commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers commented Nov 1, 2016

busstoptaktik commented Nov 2, 2016

busstoptaktik commented Nov 3, 2016

busstoptaktik commented Nov 12, 2016

busstoptaktik Oct 30, 2016 •

edited

Loading