Consistency of parameters

[awulkiew]

I'm reading through the code and wondering if parameters handling in some cases is a bug or a feature.

I. no_rot in https://github.com/OSGeo/proj.4/blob/master/src/PJ_omerc.c#L125 Shouldn't it be treated as bool instead of checked for presence? Like here: https://github.com/OSGeo/proj.4/blob/master/src/PJ_labrd.c#L106 So:

Q->no_rot = pj_param(P->ctx, P->params, "bno_rot").i;

II. tilt and azi in https://github.com/OSGeo/proj.4/blob/master/src/PJ_nsper.c#L190 Shouldn't they be treated as angles/dms instead of real numbers in degrees? Like here: https://github.com/OSGeo/proj.4/blob/master/src/PJ_labrd.c#L107 or https://github.com/OSGeo/proj.4/blob/master/src/PJ_isea.c#L1085 So:

omega = pj_param(P->ctx, P->params, "rtilt").f;
gamma = pj_param(P->ctx, P->params, "razi").f;

[kbevers]

It basically does the same thing. From pj_params.c:

    case 'b':    /* boolean */
        switch (*opt) {
        case 'F': case 'f':
            value.i = 0;
            break;
        case '\0': case 'T': case 't':
            value.i = 1;
            break;

I agree that it is a mess, but I don't think that it causes any problems. I would like this part of the code changed to something less complicated at some point.

Regarding the nsper code, I think that you have presented a good argument for changing the parameter reading code. It is difficult to understand what the various prefixes mean. I think the nsper code is much easier to read than the labrd code since it explicitly states what is done. It probably is the way that it is because whoever implemented nsper did not know all the details of how to use pj_param().

[awulkiew]

Right, it doesn't cause any problems since boolean arguments are probably always passed by the users without any value.

If this is not considered to be a problem and since everyone are passing boolean params without values have you considered removing this boolean handling code from pj_params and instead simply checking for presence of boolean parameter in all places in the library?

Btw, I saw samples (https://en.wikibooks.org/wiki/PROJ.4) where value =1 is passed which AFAIU will result in error (it's the default condition below the code you pasted above).

[kbevers]

If this is not considered to be a problem and since everyone are passing boolean params without values have you considered removing this boolean handling code from pj_params and instead simply checking for presence of boolean parameter in all places in the library?

Short answer is no. @busstoptaktik and I have talk about completely replacing pj_param() with something more comprehensible though. You are welcome to suggest a better way of doing this - the current way might have been good ~25 years ago, today not so much. If a few potential bugs can be eliminated in the process that is all the better.

[awulkiew]

I'm not sure what do you have in mind by "better way of doing this". Do you think about the interface or about the internals of pj_param?

Personally I don't understand the rationale behind taking the param type as string and returning a union. Do you know why it was done this way? I'd rather make type definition part of function name like in OpenGL. Then instead of returning PROJVALUE value of desired type could be returned (int, double or const char *). Furthermore test for presence is already implemented as pj_param_exists and used in some places (e.g. in pj_init.c) so there is no need to rely on pj_param with 't' in opt.

So e.g. the gamma assignment in nspar would be:

gamma = pj_param_r(P->ctx, P->params, "azi");

Furthermore currently in some cases pj_param is called 2x for a parameter, to check if it exists and then to assign its value to some variable. So parameters list is traversed 2x. This could be done in 1 function call if the function returned BOOL and assigned value if parameter was present. E.g. this code in aitoff (https://github.com/OSGeo/proj.4/blob/master/src/PJ_aitoff.c#L187):

if (pj_param(P->ctx, P->params, "tlat_1").i) {
    if ((Q->cosphi1 = cos(pj_param(P->ctx, P->params, "rlat_1").f)) == 0.)

could be replaced with:

double phi1 = 0.0;
// ...
if (pj_param_r(P->ctx, P->params, "lat_1", phi1)) {
    if ((Q->cosphi1 = cos(phi1)) == 0.)

And last but not least, if boolean param code was not using ctx to for error handling, so e.g. the presence of boolean parameter was treated as setting it, ctx would be needed only for angle parameters (_r) but not for any other parameter. So the interface of pj_param_i, pj_param_f, pj_param_s and pj_param_b would be the same as the interface of pj_param_exists.

Btw, is there a reason why in pj_ell_set.c there are functions pj_get_param and pj_param_value which are similar to pj_param but are not implemented in the same file?

I'm not knowledgeable in proj4 enough to propose more severe changes.

[busstoptaktik]

@awulkiew: Basically asking "why" questions about the PROJ code is not very instructive. The foundation was laid out more than 30 years ago, to reflect and support a research hardware infrastructure based on ink pen plotters driven by PDP-11 generation CPUs. And while the system definition language on the surface of PROJ has stood the test of time, the same is not true for all of the internals.

This is why, 2 years ago, when @kbevers and I began our work of extending PROJ with better support for general geodetic transformations, we started by doing some major refactoring.

The first thing we did was to significantly reduce the use of a rather complex macro system, which occluded the code internals. Later, while working through the code, we have been redoing much of the internal plumbing - and we continue doing that.

We do, however, not change things just for the sake of change: We're here primarily to make PROJ support geodetic grade transformations, and as soon as the refactoring had reached a level where it was worth building on, this has been our main goal.

With that goal more or less fulfilled by now, the work towards 5.1.0 will include much refactoring and replumbing in order to improve the long term maintainability. My personal primary milestone for the 5.1.0 release will be to turn the internal plumbing into a purely 4D highway, which will greatly simplify (and somewhat speed up) things.

The case of param being obnoxious is less important here, but it will be handled: We're working on it, partially by doing the kind of experimental, localized implementations you have found in pj_ell_set.c.

We're getting there, and now, having a major improvement of functionality (almost) shipped, there's better time for infrastructural- and documentation improvements. Also, after release 6.0.0, with projects.h gone, we will be much more free to change things in backwards incompatible ways, hence opening up for a whole new class of radical improvements.

But renovating a major piece of geospatial infrastructure, while the world around us still run at full speed is no simple task. Reimplementing everything from scratch, with a clean architecture and design, would have been way easier than what we're doing, but that would not have resulted in geodetic grade transformations becomming generally available - it would just have resulted in yet another library sitting in a corner, unknown and underutilized.

A few of the wonderful things of working with PROJ are:

1. The user and developer community, in general.
2. And in particular the discussions with these extremely clever and helpful people, which much improves code and architectural quality - and
3. The ubiquity of PROJ, which means that code getting into PROJ is also getting into use.

To get a feeling for what that kind of stuff this can result in, I suggest you take a look at the code for version 4.9.0 to get a feeling for how much has changed the last few years.

And keep posting your findings of inconsistencies - just don't expect to find any obvious explanations of why they are there, other than the one @kbevers has already given.

[kbevers]

And keep posting your findings of inconsistencies - just don't expect to find any obvious explanations of why they are there, other than the one @kbevers has already given.

@awulkiew You are of course also very welcome to suggest fixes to those inconsistencies. I think you are on the right track with what you've already proposed. It would be interesting to see a pull request with these improvements.

One thing to note here is that the test coverage is generally quite bad with regards to parameter reading for the various projections in the library. This needs to be improved before a full replacement of pj_param is made, so that we can be confident that no new bugs are introduced. Unfortunately it is not a simple task to add the necessary test material.

[awulkiew]

@busstoptaktik Thanks for the explanation!

I'm asking because at Boost.Geometry we have older proj4 version (4.9.2? 4.9.1 with some additional fixes) converted to C++. It was in a place called "extensions", i.e. not released for the users officially, but recently I picked it up with a goal of releasing it. And the change of parameters handling is one of the changes I wanted to do. So I'm asking about it mainly to know if the way how this is implemented is important and maybe shouldn't be touched. But also I wanted to know if the changes I mentioned are welcome since I'm considering creating pull requests with the ones I make in Boost.Geometry that are suitable for C in order to keep both versions in sync as much as possible.

@kbevers Yes, I'm aware that such change ideally would be preceeded with adding tests. But I doubt I'll be able to do it in a timeframe I wanted to give to parameter handling. But anyway I'd like to create a pull request with this more major change too and allow you to pick it up possibly somewhere in the future when you feel test coverage is sufficient to apply it, if you're ok with that.

[kbevers]

@kbevers Yes, I'm aware that such change ideally would be preceeded with adding tests. But I doubt I'll be able to do it in a timeframe I wanted to give to parameter handling. But anyway I'd like to create a pull request with this more major change too and allow you to pick it up possibly somewhere in the future when you feel test coverage is sufficient to apply it, if you're ok with that.

I do not expect you to add the test material as part of a potential PR. At first a proof of concept adding new pj_param_whatever functions alongside the existing pj_param and application of those functions in a few PJ_*.c files (preferably ones with good test coverage).

The realistic way forward, I think, is a gradual migration from pj_param to a new paramter reading interface refactoring one PJ_ at a time. This is also a good opportunity to add tests as we go along with the refactoring instead of adding it all in one go.

[busstoptaktik]

But anyway I'd like to create a pull request.

@awulkiew: Just a few remarks, relating to the existing work, which may be of relevance for your proposed work:

1. Note the differences between pj_param_exists() and pj_get_param() wrt. handling the paralist->used element. We discovered the necessity of handling this - unfortunately, we discovered it the hard way :-)

2. pj_param_value() could be the foundation for type specific parameter handlers. But for flags (i.e. parameters with no value attached), it should probably be changed to return a pointer to a statically allocated buffer holding the letters "true", rather than (as now) returning the flag itself: If the flag name starts with "F" or "f" it might (by legacy code) be interpreted as "false", i.e. not specified.

[awulkiew]

For now I created a PR with fix for no_rot, azi and tilt parameters (https://github.com/OSGeo/proj.4/pull/832). Regarding other changes I'll propose something in the future.

@busstoptaktik Regarding the used member do I understand correctly that it's used only for generation of proj4 string parameters list from a projection definition? Or is the fact that a parameter was used important from some other reason? Because if that was the case then that might be reflected in a function name, e.g. pj_use_param().

[kbevers]

For now I created a PR with fix for no_rot, azi and tilt parameters (#832). Regarding other changes I'll propose something in the future.

Thanks. I guess this is mostly just scratching a personal itch, yes? I don't mind you changing this, I am just interested in why. It is a good opportunity to get familiar with the code if nothing else.

@busstoptaktik Regarding the used member do I understand correctly that it's used only for generation of proj4 string parameters list from a projection definition? Or is the fact that a parameter was used important from some other reason? Because if that was the case then that might be reflected in a function name, e.g. pj_use_param().

As far as I know, yes, that is how used is used. I don't think a pj_use_param() is needed at the moment. It can be added if that need should ever show up.

[awulkiew]

Thanks. I guess this is mostly just scratching a personal itch, yes? I don't mind you changing this, I am just interested in why. It is a good opportunity to get familiar with the code if nothing else.

Yes, it's only a matter of consistency. Initially I only wanted to ask a question about it but since you're ok with the change I decided to create a PR.

As far as I know, yes, that is how used is used. I don't think a pj_use_param() is needed at the moment. It can be added if that need should ever show up.

To be precise. With this function I was thinking about distinguishing between pj_get_param(), so a function returning a parameter and a function returning a parameter and marking it as used, hence pj_use_param(). My intention was not to propose a function only for marking parameter as used.

But anyway, thanks for the help. I think this issue can be closed.

[kbevers]

Ah okay, I understand now. That might be useful. I think there's a few potential cases where a function like that might prove useful.