Introducing GeoSceneAbstractTileProjection for tile x/y <-> lonlat
ClosedPublic

Authored by kossebau on Sep 14 2016, 5:12 PM.

Details

Summary

In the current code in some places hard assumptions are made about
the projection used by the tile material. Also are calculations
duplicated.
The new abstract class GeoSceneAbstractTileProjection and its currently
two concrete subclasses GeoSceneEquirectTileProjection & GeoSceneMercatorTileProjection
should allow to make most code agnostic of the tile projection details
and also remove the code duplication, also makes unit testing of the projection easy.

GeoSceneAbstractTileProjection follows the concepts of AbstractProjection
and has conversion methods in the interface, with the output vars as
non-const references at the end of the argument list (for consistency, I personally
prefer yielded stuff as return parameter, or have output vars at least being
first in the argument list and as pointers for improved markup in the calling code).
As the current two implementations need to know about the variables
levelZeroColumns and levelZeroRows, I made these properties of the classes
themselves, to avoid having the classes depend on GeoSceneTileDataset.
Comes at the cost of data duplication and thus more complicated setup,
but also avoids an indirection in the class methods using these values.
For simplicity I put these properties onto GeoSceneAbstractTileProjection
itself, to avoid another intermediate abstract subclass for the concept of
levelZeroColumns and levelZeroRows.

The projection methods do not do any out-of-bounds handling, but expect
the calling code to pass proper value. Which should be valid with the current
callees.

Any methods tileProjection() have been renamed to tileProjectionType(),
to make clear those just return the type, not a projection object.

The patch also makes TileId more dump again and just a property container,
especially no longer knowing about GeoSceneTileDataset.

FUTURE WORK:

  • Look into all remaining places which have "if tileProjectionType() == x" to see how they can be made tile projection type agnostic as well

Diff Detail

Repository
R34 Marble
Branch
GeoSceneAbstractTileProjection
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau updated this revision to Diff 6747.Sep 14 2016, 5:12 PM
kossebau retitled this revision from to Introducing GeoSceneAbstractTileProjection for tile x/y <-> lonlat.
kossebau updated this object.
kossebau added a reviewer: Marble.
rahn added a subscriber: rahn.Sep 15 2016, 10:55 AM

Q3: Always use Gudermannian function?

Yes! See below

The old TileId::toLatLonBox() used not the Gudermannian function, but
atan( sinh( ( radius - y() - 1 ) / radius * M_PI ) );

atan(sinh()) _is_ one possible representation of the gudermannian function, in fact it's the most important one and the one we are using inside
our MarbleMath::gd( qreal x ) implementation (read the source Luke ... ;-)

The Gudermannian function and its inverse, the inverse Gudermannian are the basically by definition the transformation and inverse transformation of Mercator. They are not bound to a particular representation, so you'll find many "different" representations (e.g. at https://en.wikipedia.org/wiki/Mercator_projection#Derivation_of_the_Mercator_projection) but still all of them are just a different way of phrasing the Gudermannian function.

There are also other possible alternate implementations, see e.g. the "Alternate definitions" section at

https://en.wikipedia.org/wiki/Gudermannian_function
https://en.wikipedia.org/wiki/Mercator_projection#Derivation_of_the_Mercator_projection

Inside MarbleMath we are standardizing our preferred pick for the implementation in order to guarantee best performance on a variety of devices.
For the inverse Gudermannian we developed the method into a power series using the horner method representation for best performance.
The the regular Gudermannian we are using the bare atan(sinh()) representation.

Bottomline: we really should _always_ use gd() and gdInv() whereever possible. Therefore we should also use it inside GeoSceneMercatorTileProjection::geoCoordinates().

rahn requested changes to this revision.Sep 15 2016, 11:10 AM
rahn added a reviewer: rahn.
rahn added inline comments.
src/lib/marble/geodata/scene/GeoSceneAbstractTileProjection.h
81 ↗(On Diff #6747)

see below

82 ↗(On Diff #6747)

see below

86 ↗(On Diff #6747)

The naming here is wrong.

Longitude always changes with x and latitude changes with y. Additionally we always stick to the order Lon, Lat. So it should be

westLon, northLat

src/lib/marble/geodata/scene/GeoSceneEquirectTileProjection.cpp
72 ↗(On Diff #6747)

You are mixing up longitude and latitude here:

Longitude always changes along the X axis

Latitude always changes along the Y axis

So the naming should rather be westLon and northLat ---
Or since that still sounded a bit odd maybe something like
northernTileEdgeLat, westernTileEdgeLon ?

src/lib/marble/geodata/scene/GeoSceneMercatorTileProjection.cpp
99 ↗(On Diff #6747)

Same naming issue as above:

longitude changes with x, so it should be westLon
latitude changes with y, so it should be northLat

121 ↗(On Diff #6747)

This should use gd() instead.

122 ↗(On Diff #6747)

This should use gd() instead.

This revision now requires changes to proceed.Sep 15 2016, 11:10 AM
rahn added a comment.Sep 15 2016, 11:17 AM

Q1: Error/out-of-bounds handling?
Where to handle bad input data, like out-of-bound values?

For Mercator latitude we always return the actual value for +/-85° for all values that exceed +/-85°. This allows us to avoid the odd looking "empty polar cap" on the globe.
For the latitude in all other projections we can use GeoDataCoordinates::normalizeLat() to handle the out-of-bound case
For longitudes we usually use GeoDataCoordinates::normalizeLon() to handle out-of-bound cases (if it could realistically become an issue).

kossebau updated this revision to Diff 6787.Sep 17 2016, 12:03 AM
kossebau edited edge metadata.

Changed:

  • Fixed mixed up lon/lat parameter names
  • use gd() instead of explict own calculation
  • added unittest TileProjectionTest
  • fixed some issues found with unit tests
kossebau marked 7 inline comments as done.Sep 17 2016, 12:09 AM
kossebau updated this object.Sep 17 2016, 12:15 AM
kossebau edited edge metadata.
kossebau updated this revision to Diff 6821.Sep 19 2016, 8:38 PM

update to latest master

rahn accepted this revision.Oct 6 2016, 2:13 PM
rahn edited edge metadata.
This revision is now accepted and ready to land.Oct 6 2016, 2:13 PM
This revision was automatically updated to reflect the committed changes.