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

Feature/#329 long based coordinates with max zoom 29 #723

Merged
merged 20 commits into from Oct 1, 2017

Conversation

Projects
None yet
2 participants
@monsieurtanuki
Collaborator

monsieurtanuki commented Sep 27, 2017

Zoom level is now up to 29.

monsieurtanuki added some commits Aug 25, 2017

feature/#329: store and restore the actual map center (instead of scr…
…oll) and the map orientation

This is the first (slightly unrelated) step.
Why should we use the map center instead of the scroll? Because when tilting the smartphone portrait-landscape-wise, the map center is kept only modulo the difference between `MapView`'s width and height.

Impactes classes:
* `OpenStreetMapConstants`: deprecated scroll preference tags, added latitude, longitude and orientation preference tags
* `MapView`: added the concept of `IGeoPoint initCenter`, took it into account in method `myOnLayout`, added a no-force-redraw version of method `setMapOrientation`
* `StarterMapFragment`: deprecated the use of scroll preferences tags, introduced the use of new preferences' tags (latitude, longitude, orientation), gently refactored in order to avoid Android Studio's nagging
bug/#683: mMapView.zoomToBoundingBox is not calculating the required …
…zoom level correctly

This commit:
* fixes the calculation of the zoom that matches a bounding box
* adds Unit Tests for `TileSystem`
* makes a more explicit demo

Impact on existing classes:
* `BoundingBox`: created method `getCenterWithDateLine` that takes into consideration the date line; deprecated `getCenter`
* `MapView`: created method `zoomToBoundingBox` with an additional `borderSizeInPixels` parameter; modified previous method `zoomToBoundingBox` in order to use the new `zoomToBoundingBox` with 0 as border size; fixed the bounding box zoom calculation that is now testable and moved to `TileSystem.getBoundingBoxZoom`
* `TileSystem`: created methods `getBoundingBoxZoom`, `getLongitudeZoom`, `getLatitudeZoom`, `getX01FromLongitude`and `getY01FromLatitude`; modified `LatLongToPixelXYMapSize` in order to take into account the new "XY01" methods; gently refactored javadoc version `6.0.0` to `5.6.6`
* `SampleZoomToBounding`: made a more explicit test (which is accessed in "More Samples / Events / Zoom to Bounding Box" demo)

New class:
* `TileSystemTest`: created in order to test methods `getY01FromLatitude`, `getX01FromLongitude`, `LatLongToPixelXYMapSize` and `getBoundingBoxZoom`
Fixed and enhanced a unit test
Impact on existing classes:
* OpenStreetMapViewTest: added random iterations; set zoom before center for decent results; added a +-1 tolerance because of my benevolence towards rounding; wrote a relevant test tag; removed commented and deprecated code
Fixed and enhanced unit tests
Impact on existing classes:
* Bug445Caching: made the test less resource dependent by limiting max zoom level to 16 (cf. https://operations.osmfoundation.org/policies/tiles/: "[tiles at zoom levels 17 and higher] are generally not available (cached) on the server in advance, and have to be rendered specifically for those requests, putting an unjustified burden on the available resources"); gently refactoring javadoc from `6.0` to `5.6.6`
* TileSystemTest: made the XY01 test stronger by checking [0,1]
#329 Zoom level is now up to 29
New files:
* 30 `.png` files corresponding to the tiles of the same place on the 30 zoom levels from 0 to 29. I had no way to compute real tiles for high zoom levels, therefore I created the "Abstract" source of tiles with simple colored .png where the zoom level is displayed.

New classes:
* `PointL`: like a `Point`, but with long instead of int
* `ProjectionTest`: a unit test class for `Projection`
* `RectL`: like a `Rect`, but with long instead of int
* `SampleVeryHighZoomLevel`: a demo dedicated to high zoom levels, available in "More Samples / Tileproviders / Offline abstract tiles for zoom levels 0 to 29"

Biggest impacts on existing classes:
* `MapView`: used `PointL` and `RectL` types for Projected data; removed `initCenter`; added `GeoPoint mCenter`, `long mMapScrollX` and `mMapScrollY` and their setters/getters; overridden `scrollBy`; changed `scrollTo`; added `getMapScale`
* `Projection`: created a new constructor without any reference to `MapView`; created `getOffspring` in order to compute a `Projection` from another (e.g. for `MinimapOverlay`); renamed methods for clarity; created _many_ methods
* org.osmdroid.util.TileSystem: renamed methods for clarity; used `PointL` and `RectL` types for Mercator data; created _many_ methods

Other impacted classes:
* `CacheManager`: removed useless code that would have required refactoring
* `GeometryMath`: refactored `DEG2RAD` / `RAD2DEG`; used `MyMath.floorToInt` instead of `(int)` for better handling of negative values; used simpler syntax on `Rect` for easier unit testing purposes
* `GeoPoint`: created method `distanceToAsDouble` to go beyond the meter; used a distance calculation algorithm more precise for small distances; increased precision from float to double
* `GeoPointTest`: create methods `test_distanceTo_itself`, `test_distanceTo_Equator`, `test_distanceTo_Equator_Smaller`, `test_distanceTo_Parallels`, `getCleanLongitudeDiff`, `getRandomLongitude`, `getRandomLatitude`; removed less relevant methods `test_distanceTo_zero`, `test_distanceTo_one`, test_distanceTo_one_degree`
* `MapController`: changed animation methods according to the new scroll behavior
* `MapTileProviderBase`: used `PointL` and `RectL` types for Mercator data
* `Marker`: refactored the use of methods from `Projection` to `MapView`
* `MathConstants`: increased precision from float to double
* `MinimapOverlay`: relied more on inherited methods than on specific code
* `MyLocationNewOverlay`: used `PointL` type for Projected data; using `double` instead of `int` zoom level; using new method `Projection.getPixelsFromProjected`
* `MyMath`: created new methods `flootToLong` and `flootToInt` in order to fix a counter intuitive Java behavior
* `OpenStreetMapViewTest`: unrelated light refactoring
* `OsmBitmapShader`: used `PointL` type for Mercator data
* `PathOverlay`: used `PointL` and `RectL` types for Projected data
* `PathProjection`: used `PointL` type for Mercator data
* `Polygon`: used `PointL` type for Projected data
* `Polyline`: used `PointL` type for Projected data
* `SampleAnimateTo`: changed slightly for testing reasons
* `SampleFactory`: added new class `SampleVeryHighZoomLevel`
* `SampleZoomToBounding`: unrelated light refactoring
* `ScaleBarOverlay`: used more precise new method `GeoPoint.distanceToAsDouble`; handled double distances in method `scaleBarLengthText`; added helper method `getScaleString`; refactored the use of methods from `Projection` to `MapView`
* `StarterMapFragment`: unrelated bug fix *** setInitCenter
* `TileLooper`: used `RectL` type for Mercator data; slightly refactored
* `TilesOverlay`: used `PointL` and `RectL` types for Mercator data; refactored `onTileReadyToDraw`; created `protected` methods `setCanvasRect`, `getCanvasRect`, `setProjection` and `getProjection`
* `microsoft.mappoint.TileSystem`: used 64 instead of 32 as a limit parameter in `setTileSize`; introduced the notions of `primaryKeyMaxZoomLevel` and `projectionZoomLevel`
* `TileSystemTest`: added `testGetLatitudeFromY01`, `testLatitude`, `testGetLongitudeFromX01`, `testLongitude`, `checkLatitude` and `checkLongitude`; removed `testLatLongToPixelXYMapSize`
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Sep 27, 2017

Collaborator

@spyhunter99 Any help regarding conflict resolution is welcomed, such as magical git instructions.

Collaborator

monsieurtanuki commented Sep 27, 2017

@spyhunter99 Any help regarding conflict resolution is welcomed, such as magical git instructions.

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

mmm you need to merge master into your branch.
assuming your local master is up to date
git checkout master
git pull origin master
git checkout feature/#329
git merge master
git push origin feature/#329

Collaborator

spyhunter99 commented Sep 29, 2017

mmm you need to merge master into your branch.
assuming your local master is up to date
git checkout master
git pull origin master
git checkout feature/#329
git merge master
git push origin feature/#329

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

super exciting by the way

Collaborator

spyhunter99 commented Sep 29, 2017

super exciting by the way

monsieurtanuki added some commits Sep 29, 2017

Impacts:
* `SampleFactory`: added a reference to new demo `SampleVeryHighZoomLevel`
* `TileSystem`: switched to `double` zoom level versions for methods `MapScale` and `GroundResolution`
* `TileSystemMathTest`: moved to `TileSystemTest` methods `test_MapSize`, `test_groundResolution` and `test_groundMapScale`
* `TileSystemTest`: moved from `TileSystemTest` methods `test_MapSize`, `test_groundResolution` and `test_groundMapScale`; improved their handling of high zoom levels
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

This is amazing

Collaborator

spyhunter99 commented Sep 29, 2017

This is amazing

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

Testing results:

  • With hardware acceleration turned on
    • holy crap this is fast
    • lines are ok to max zoom (expected)
    • polygons still have the same issue as before, disappears at zoom + 2 (expected)
    • polygons still have the world wrapping issue (expected)
  • With hardware acceleration turned off
    • map means slightly slower but its probably close to about usual or it's just my phone
    • polylines seem fine up to max zoom
    • polygons, at high zooms, the border drawn around the polygon disappears at z=25. Not sure why, maybe @MKergall would have some insight. Holes are fine. Still has the wrapping issue (expected)
    • latlon grid lines i saw one case where the label disappeared. It may be my crummy math though and I don't think it handles past zoom 20 anyhow, so this is somewhat expected. No worries, not asking you to fix this

fantastic work!

Collaborator

spyhunter99 commented Sep 29, 2017

Testing results:

  • With hardware acceleration turned on
    • holy crap this is fast
    • lines are ok to max zoom (expected)
    • polygons still have the same issue as before, disappears at zoom + 2 (expected)
    • polygons still have the world wrapping issue (expected)
  • With hardware acceleration turned off
    • map means slightly slower but its probably close to about usual or it's just my phone
    • polylines seem fine up to max zoom
    • polygons, at high zooms, the border drawn around the polygon disappears at z=25. Not sure why, maybe @MKergall would have some insight. Holes are fine. Still has the wrapping issue (expected)
    • latlon grid lines i saw one case where the label disappeared. It may be my crummy math though and I don't think it handles past zoom 20 anyhow, so this is somewhat expected. No worries, not asking you to fix this

fantastic work!

import org.osmdroid.views.overlay.ScaleBarOverlay;
/**
* A lousy example of very high zoom levels.

This comment has been minimized.

@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

This is brilliant

@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

This is brilliant

This comment has been minimized.

@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

Thank you! I like it when I just click on the zoom out button, from level 29 down to 0.
Btw if you have access to "less abstract" tiles up to zoom level 29, or if you feel like adding polygons / polylines as examples, don't hesitate to enhance this demo.

@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

Thank you! I like it when I just click on the zoom out button, from level 29 down to 0.
Btw if you have access to "less abstract" tiles up to zoom level 29, or if you feel like adding polygons / polylines as examples, don't hesitate to enhance this demo.

@@ -342,20 +339,35 @@ public Projection getProjection() {
return mProjection;
}
/**
* Use {@link #resetProjection()} instead

This comment has been minimized.

@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

i'm a little confused by the javadoc

@spyhunter99

spyhunter99 Sep 29, 2017

Collaborator

i'm a little confused by the javadoc

This comment has been minimized.

@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

The only use of setProjection was setProjection(null). The actual use was to reset the projection, so that it gets recomputed later in getProjection (after a change of zoom level or center).

@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

The only use of setProjection was setProjection(null). The actual use was to reset the projection, so that it gets recomputed later in getProjection (after a change of zoom level or center).

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

@spyhunter99 What is the "wrapping" issue with Polygons? Is that something like that:

  • in very low zoom levels
  • GeoPoints can be theoretically projected on the screen in several Points (modulo world size)
  • and as it's the top left Point that is chosen by default
  • depending on the map scroll, the Polygons can have different shapes, including unexpected ones?

Is there currently an open issue on that?

Collaborator

monsieurtanuki commented Sep 30, 2017

@spyhunter99 What is the "wrapping" issue with Polygons? Is that something like that:

  • in very low zoom levels
  • GeoPoints can be theoretically projected on the screen in several Points (modulo world size)
  • and as it's the top left Point that is chosen by default
  • depending on the map scroll, the Polygons can have different shapes, including unexpected ones?

Is there currently an open issue on that?

Fixed `Polygon` issue as in https://www.youtube.com/watch?v=S4Pf2u9WSyQ
Impacts in `Polygon` class:
* `mOriginalPoints` is now an array of `double`, no more `intE6` (but it's not related to the bug fixing)
* new method `setCloserPoint`: the assumption is that 2 consecutive points should be as close as possible, therefore we add or subtract "the size of the world" if necessary
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 30, 2017

Collaborator

for clarify, that youtube video was with the patch as applied in the open PR with hardware acceleration off

Collaborator

spyhunter99 commented Sep 30, 2017

for clarify, that youtube video was with the patch as applied in the open PR with hardware acceleration off

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

As far as I understood the expected results, I've just fixed https://www.youtube.com/watch?v=S4Pf2u9WSyQ
Might solve https://www.youtube.com/watch?v=EQsi38PIYm8 too.
Might even solve MKergall/osmbonuspack#168 as Polygon's coordinates were truncated into intE6, which is not the case anymore.
If needed I could have a look at performances issue #476 and #353. Is #476 fixed or no - there's a PR pending, right?

Collaborator

monsieurtanuki commented Sep 30, 2017

As far as I understood the expected results, I've just fixed https://www.youtube.com/watch?v=S4Pf2u9WSyQ
Might solve https://www.youtube.com/watch?v=EQsi38PIYm8 too.
Might even solve MKergall/osmbonuspack#168 as Polygon's coordinates were truncated into intE6, which is not the case anymore.
If needed I could have a look at performances issue #476 and #353. Is #476 fixed or no - there's a PR pending, right?

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 30, 2017

Collaborator

test results

  • hw acceleration off
    • polyline disappears at zoom 25
    • polygon border disappears at zoom 25
  • hw acceleration on
    • polyline disappears at zoom 26
    • polygon disappears when zooming in, it seems to be linked to large polygons that are primarily all offscreen
Collaborator

spyhunter99 commented Sep 30, 2017

test results

  • hw acceleration off
    • polyline disappears at zoom 25
    • polygon border disappears at zoom 25
  • hw acceleration on
    • polyline disappears at zoom 26
    • polygon disappears when zooming in, it seems to be linked to large polygons that are primarily all offscreen
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 30, 2017

Collaborator

regardless, i don't think any of these are major blockers for getting this pr merged. it's already a significant improvement. they didn't build rome in a day

Collaborator

spyhunter99 commented Sep 30, 2017

regardless, i don't think any of these are major blockers for getting this pr merged. it's already a significant improvement. they didn't build rome in a day

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

device-2017-09-30-183753
@spyhunter99 I'm a bit surprised: could you give an example about Polygon's border not displaying at zoom 25? Attached is an example of Polygon at zoom level 29.

In the meanwhile I'll fix Polyline as I fixed Polygon.

Collaborator

monsieurtanuki commented Sep 30, 2017

device-2017-09-30-183753
@spyhunter99 I'm a bit surprised: could you give an example about Polygon's border not displaying at zoom 25? Attached is an example of Polygon at zoom level 29.

In the meanwhile I'll fix Polyline as I fixed Polygon.

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 30, 2017

Collaborator

sure thing, i'll pull again, clean build, then take a video capture. I had draw the polygon at something like zoom =2 then zoomed in, keeping the border on screen at all times

Collaborator

spyhunter99 commented Sep 30, 2017

sure thing, i'll pull again, clean build, then take a video capture. I had draw the polygon at something like zoom =2 then zoomed in, keeping the border on screen at all times

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

I changed idea: I won't edit Polyline as I don't have a clear understanding of what is naturally expected on the screen in case of very low zoom level.

@spyhunter99 Understood: I just got the same bug by drawing on zoom level 0 and zooming to level 22.

Collaborator

monsieurtanuki commented Sep 30, 2017

I changed idea: I won't edit Polyline as I don't have a clear understanding of what is naturally expected on the screen in case of very low zoom level.

@spyhunter99 Understood: I just got the same bug by drawing on zoom level 0 and zooming to level 22.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Sep 30, 2017

Collaborator

I'll fix the Polygon border issue in another PR. From what I understood, the problem is that computed pixel coordinates get "too big" for a Path, especially with hardware acceleration flag on (but not that big either: it's still below Integer.MAX_VALUE). The solution will be to approximate the compute pixels to (sic) "not too big" values.

For the current PR, no further dev on my side.
@spyhunter99 Ready to merge?

Collaborator

monsieurtanuki commented Sep 30, 2017

I'll fix the Polygon border issue in another PR. From what I understood, the problem is that computed pixel coordinates get "too big" for a Path, especially with hardware acceleration flag on (but not that big either: it's still below Integer.MAX_VALUE). The solution will be to approximate the compute pixels to (sic) "not too big" values.

For the current PR, no further dev on my side.
@spyhunter99 Ready to merge?

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Sep 30, 2017

Collaborator

I'm fine with it. @MKergall any opinions?

Collaborator

spyhunter99 commented Sep 30, 2017

I'm fine with it. @MKergall any opinions?

@spyhunter99 spyhunter99 changed the title from Feature/#329 to Feature/#329 long based coordinates with max zoom 29 Oct 1, 2017

@spyhunter99 spyhunter99 merged commit 0990b70 into osmdroid:master Oct 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@spyhunter99 spyhunter99 added this to the v6.0.0 milestone Oct 1, 2017

@monsieurtanuki monsieurtanuki deleted the monsieurtanuki:feature/#329 branch Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment