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

ob_tran: restore traditional handling of +to_meter with pj_transform() and proj utility (fixes #1782) #1783

Conversation

@rouault
Copy link
Member

rouault commented Dec 10, 2019

Fixes side-effect of #1525 that went in 6.1.1

The correction is horribly hacky. Sorry...

@rouault rouault added this to the 6.3.0 milestone Dec 10, 2019
…) and proj utility (fixes #1782)

Fixes side-effect of #1525 that went in 6.1.1

The correction is horribly hacky. Sorry...
@rouault rouault force-pushed the rouault:restore_ob_tran_to_meter_compat_with_pj_transform branch from cb3244b to 2c3a7c8 Dec 10, 2019
Copy link
Member

kbevers left a comment

The correction is horribly hacky. Sorry...

Too hacky in my opinion. I don't think there's a way around a hacky solution but at least it should be localized to ob_tran. I think that should be possible by doing a checking what type of projection is used in o_forward() and then scaling the values so they come out as degrees.

@rouault

This comment has been minimized.

Copy link
Member Author

rouault commented Dec 10, 2019

I think that should be possible by doing a checking what type of projection is used in o_forward() and then scaling the values so they come out as degrees.

That's pretty much what we already do in ob_tran.cpp with

    /* Support some rather speculative test cases, where the rotated projection */
    /* is actually latlong. We do not want scaling in that case... */
    if (Q->link->right==PJ_IO_UNITS_RADIANS)
        P->right = PJ_IO_UNITS_WHATEVER;

The reallity is that ob_tran has always been hacky. With the new interface of proj_create_crs_to_crs(), it is somewhat less hacky since we don't require any more (and no longer honour) the to_meter=0.0174532925199433 . Hence adding back the hack only when invoked from the ancient API.

I'm happy to let you find a better solution if you can think of one that doesn't make the fix of #1525 regress...

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 10, 2019

That's pretty much what we already do in ob_tran.cpp with

Except that it is done in the setup and not on the individual coordinate. I would put in a check similar to

    if( Proj->right == PJ_IO_UNITS_WHATEVER && Proj->descr &&
        strncmp(Proj->descr, "General Oblique Transformation",
                strlen("General Oblique Transformation")) == 0 )
    {
        Proj->right = PJ_IO_UNITS_PROJECTED;
    }

in o_forward().

I'm happy to let you find a better solution if you can think of one that doesn't make the fix of #1525 regress...

I'd like to give it a go. I'm a bit overloaded at the moment but I think I can manage to squeeze in some time in the weekend.

@rouault

This comment has been minimized.

Copy link
Member Author

rouault commented Dec 10, 2019

One thing I considered to make it slightly less awkward was to add a "int is_ob_trans" member in struct PJconsts, to avoid the string comparison.

Except that it is done in the setup and not on the individual coordinate

I don't think this will have measurable runtime impact as it is done outside of the loop that transforms several coordinates at once and the Proj->right == PJ_IO_UNITS_WHATEVER comparison is going to be false for pretty much all projection methods, so the string comparison should be done only in a few cases

I would put in a check similar to in o_forward()

We do want a different behaviour depending if we use it through proj_create() or pj_transform(), so I'm not sure the solution can come from a change in ob_tran.cpp itself. Happy to be be proven wrong ! Or perhaps through all the magic that occurs in fwd.cpp / inv.cpp with the pre and post processing stages.

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 10, 2019

We do want a different behaviour depending if we use it through proj_create() or pj_transform(), so I'm not sure the solution can come from a change in ob_tran.cpp itself. Happy to be be proven wrong ! Or perhaps through all the magic that occurs in fwd.cpp / inv.cpp with the pre and post processing stages.

Good point about the differing behaviour in pj_transform() and proj_create(). It could be that the pre- and post-processing stages are the correct place to fix this.

Copy link
Member

kbevers left a comment

I didn't find the time to look at this. The hacky fix it is!

@kbevers kbevers merged commit e5526bc into OSGeo:master Dec 15, 2019
4 checks passed
4 checks passed
FreeBSD Task Summary
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.003%) to 85.997%
Details
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Dec 27, 2019
It has been fixed upstream in OSGeo/PROJ#1783
and will be in Proj 6.3.0.
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Dec 27, 2019
It has been fixed upstream in OSGeo/PROJ#1783
and will be in Proj 6.3.0.
QuLogic added a commit to QuLogic/cartopy that referenced this pull request Dec 27, 2019
It has been fixed upstream in OSGeo/PROJ#1783
and will be in Proj 6.3.0.
greglucas added a commit to greglucas/cartopy that referenced this pull request Dec 31, 2019
It has been fixed upstream in OSGeo/PROJ#1783
and will be in Proj 6.3.0.
greglucas added a commit to greglucas/cartopy that referenced this pull request Jan 7, 2020
It has been fixed upstream in OSGeo/PROJ#1783
and will be in Proj 6.3.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.