mapper icon indicating copy to clipboard operation
mapper copied to clipboard

Add CRS-to-CRS transformations to ProjTransform

Open dg0yt opened this issue 5 years ago • 2 comments

These are the current first two commits from #1598 ("direct pipeline"), adding a new, tested encapsulation of PROJ features via ProjTransform. Unlike (the final change from) #1598, this can be merged without significant risk of regressions IMO, and it would be useful for improving #1774 ("fixing track positioning inconsistency"; WIP changes not yet pushed).

This PR includes two minor unrelated changes to ProjTransform:

  • ~Consistenly using UTF-8 with modern PROJ. This was was done for consistency and for clean code. This should not cause changes in visible behaviour.~ Already merged with #1814.
  • Adding spec substitutions for legacy PROJ. This was required to make the new unit tests succeed. This should not cause changes in visible behaviour. However, it does somewhat reduce efficiency because the search for substitutions may be done twice. I don't consider this harmful.

(I think we should require modern PROJ with Mapper 1.0.)

Note that I went away from the "pipeline" term initally used in #1598 because this term identifies a more powerful interface in PROJ than CRS-to-CRS transformations.

dg0yt avatar Nov 12 '20 08:11 dg0yt

I went one step further, reimplementing the existing single-CRS constructors in terms of the two-CRS constructors, for more uniformity in construction and transformation. While this potentially increases the risk for regressions, I feel like this field is sufficiently covered by unit tests. With legacy PROJ, there is another added inefficiency (by creating more geographic CRS objects), but I think it is reasonable to pay this price for supporting legacy PROJ at all.

dg0yt avatar Nov 14 '20 09:11 dg0yt

The ProjTransform class is a little awkward.

Well, it encapsulates the awkwardness of supporting PROJ 4.9 to PROJ latest version.

forward has its inverse, but transform has no inverse.

transform is new (#1777, these two commit are listed here due to a merge). I would probably construct a second transform with parameters swapped if I would need the inverse transform.

Also, the forward and inverse methods are tailored to the case where the ProjTransform is constructed using ProjTransform (const QString&). They do not make sense for all ProjTransform objects.

I don't think this statement is accurate. The constructors are uniform (now, #1777). However, forward and inverse are tailored to be used with LatLon.

dg0yt avatar Nov 15 '20 06:11 dg0yt