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

Get rid of `MapView` scale by using the floating point zoom #790

Closed
monsieurtanuki opened this Issue Nov 28, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@monsieurtanuki
Collaborator

monsieurtanuki commented Nov 28, 2017

Issue Type

[v] Improvement

Description and/or steps/code to reproduce the problem

Now that we have floating point zooms (instead of integer zooms), we could get rid of the notion of MapView scale.
For the moment, the scale always equals 1.0f, except during pinch gesture where it reflects the delta zoom in/out. The idea would be to compute directly the next zoom and forget the notion of scale:

  • The code would be simpler and easier to maintain.
  • The behavior (especially the tiles display) would be the same during pinch gesture and when the gesture stops
  • The best possible tiles would always be displayed, instead of a scaled version of the tiles that where on screen at pinch gesture start.

Please feel free to comment!

Version of osmdroid the issue relates to:

6.0.0

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Nov 28, 2017

Collaborator

Additional thoughts if we get rid of the notion of scale:

  • we won't use it on Canvas and as a result we may avoid related problems about hardware acceleration
  • I think there's a pending issue about "some layers being scaled when some others are not", which displays smaller/bigger marker pop-ups. We would solve that too
Collaborator

monsieurtanuki commented Nov 28, 2017

Additional thoughts if we get rid of the notion of scale:

  • we won't use it on Canvas and as a result we may avoid related problems about hardware acceleration
  • I think there's a pending issue about "some layers being scaled when some others are not", which displays smaller/bigger marker pop-ups. We would solve that too
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Nov 29, 2017

Collaborator

Could solve #595 too.

Collaborator

monsieurtanuki commented Nov 29, 2017

Could solve #595 too.

@secaw

This comment has been minimized.

Show comment
Hide comment
@secaw

secaw Nov 30, 2017

Contributor

Sounds great! Would probably reduce some CPU overhead too, right?

Contributor

secaw commented Nov 30, 2017

Sounds great! Would probably reduce some CPU overhead too, right?

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Dec 1, 2017

Collaborator

I guess so. And in the supposedly common case of a 0 degree map orientation, we could get rid of the whole canvas manipulation (save / rotate / scale / restore).

Collaborator

monsieurtanuki commented Dec 1, 2017

I guess so. And in the supposedly common case of a 0 degree map orientation, we could get rid of the whole canvas manipulation (save / rotate / scale / restore).

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Dec 5, 2017

Collaborator

I can tell you, so far it looks great! Still working on it.

Collaborator

monsieurtanuki commented Dec 5, 2017

I can tell you, so far it looks great! Still working on it.

@secaw

This comment has been minimized.

Show comment
Hide comment
@secaw

secaw Dec 5, 2017

Contributor

Sounds promising - really looking forward to this!

Contributor

secaw commented Dec 5, 2017

Sounds promising - really looking forward to this!

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Dec 8, 2017

feature/osmdroid#790: map scale merged into floating point zoom
This is just a teaser, there are things to fix. For instance, the pinch gesture center is not yet taking into consideration (zooming in or out scrolls the map to the bottom right).
All comments are welcomed.

Impacted classes:
* `CompassOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`
* `CopyrightOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`
* `LinearRing`: integrated the fact that there's no map scale anymore / should be considered as equal to 1
* `MapController`
  * feature-related: edited `zoomToFixing` and `onAnimationUpdate`; simplified `onAnimationEnd`
  * refactoring: removed members `mZoomInAnimation` and `mZoomOutAnimation`; simplified methods `zoomIn`, `zoomInFixing`, `zoomOut`, `zoomOutFixing`
* `MapView`
  * feature-related: removed members `ZOOM_SENSITIVITY`, `ZOOM_LOG_BASE_INV`, `mTargetZoomLevel` and `mMultiTouchScale`; deprecated methods `getZoomLevel(final boolean aPending)`, `zoomIn`, `zoomOut` and `getMapScale`; simplified `canZoomIn` and `canZoomOut`; optimized `dispatchDraw` using the new `Canvas` related `Matrix` methods of `Projection`; created new methods `startAnimation`, `setMultiTouchScale` and new member `mStartAnimationZoom`; impacted pinch gesture methods `getPositionAndScale`, `selectObject` and `setPositionAndScale`
  * refactoring: removed members `sMotionEventTransformMethod` and `mMercatorPoint`; deprecated methods `getLatitudeSpan`, `getLongitudeSpan` and `getBoundingBoxE6`; simplified method `rotateTouchEvent`
* `MinimapOverlay`: removed the "display only when not animating" restriction
* `Projection`
  * feature-related: removed member `float mMultiTouchScale`; removed parameter `pScale` from constructor; removed scale operations from matrices; optimized the computation of `mCurrentCenter`; added methods `save` and `restore` that apply the matrices to canvas only if necessary.
  * refactoring: renamed member as `mWrapEnabled`; simplified the initialization of `mScreenRectProjection`; added generic method `apply` that apply a `Matrix` to a `Point`
* `ScaleBarOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`
* `ZoomButtonsOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`; restricted the zoom action to a `ACTION_UP` event

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Dec 9, 2017

@spyhunter99 spyhunter99 added this to the v6.0.0 milestone Dec 15, 2017

spyhunter99 added a commit that referenced this issue Dec 16, 2017

feature/#790: map scale merged into floating point zoom (#800)
* feature/#728 - related: more verbose assertions
Not very often the unit test fails. With more verbose assertions, we will be able to find out why.

Impacted class:
* `OpenStreetMapViewTest`: more verbose assertions

* feature/#790: map scale merged into floating point zoom

This is just a teaser, there are things to fix. For instance, the pinch gesture center is not yet taking into consideration (zooming in or out scrolls the map to the bottom right).
All comments are welcomed.

Impacted classes:
* `CompassOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`
* `CopyrightOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`
* `LinearRing`: integrated the fact that there's no map scale anymore / should be considered as equal to 1
* `MapController`
  * feature-related: edited `zoomToFixing` and `onAnimationUpdate`; simplified `onAnimationEnd`
  * refactoring: removed members `mZoomInAnimation` and `mZoomOutAnimation`; simplified methods `zoomIn`, `zoomInFixing`, `zoomOut`, `zoomOutFixing`
* `MapView`
  * feature-related: removed members `ZOOM_SENSITIVITY`, `ZOOM_LOG_BASE_INV`, `mTargetZoomLevel` and `mMultiTouchScale`; deprecated methods `getZoomLevel(final boolean aPending)`, `zoomIn`, `zoomOut` and `getMapScale`; simplified `canZoomIn` and `canZoomOut`; optimized `dispatchDraw` using the new `Canvas` related `Matrix` methods of `Projection`; created new methods `startAnimation`, `setMultiTouchScale` and new member `mStartAnimationZoom`; impacted pinch gesture methods `getPositionAndScale`, `selectObject` and `setPositionAndScale`
  * refactoring: removed members `sMotionEventTransformMethod` and `mMercatorPoint`; deprecated methods `getLatitudeSpan`, `getLongitudeSpan` and `getBoundingBoxE6`; simplified method `rotateTouchEvent`
* `MinimapOverlay`: removed the "display only when not animating" restriction
* `Projection`
  * feature-related: removed member `float mMultiTouchScale`; removed parameter `pScale` from constructor; removed scale operations from matrices; optimized the computation of `mCurrentCenter`; added methods `save` and `restore` that apply the matrices to canvas only if necessary.
  * refactoring: renamed member as `mWrapEnabled`; simplified the initialization of `mScreenRectProjection`; added generic method `apply` that apply a `Matrix` to a `Point`
* `ScaleBarOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`
* `ZoomButtonsOverlay`: used the new `Canvas` related `Matrix` methods of `Projection`; restricted the zoom action to a `ACTION_UP` event

* Desperate try to resolve conflict

* Desperate try to resolve conflict

* Desperate try to resolve conflict

* Desperate try to resolve conflict

* Merging the pinch gesture scale into floating point zoom (last fix?)

Impacted classes:
* `Projection`: rewrote the code in order to comply with `ProjectionTest`'s important notice (cf. javadoc)
* `ProjectionTest`: fixed the call to `Projection` constructor

* Lousy attempt to decrease the time taken by travis.

Impacted classes:
* `ExtraSamplesTest`: limited to 60 the number of items to run.
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Dec 18, 2017

Collaborator

merged

Collaborator

spyhunter99 commented Dec 18, 2017

merged

@MKergall

This comment has been minimized.

Show comment
Hide comment
@MKergall

MKergall Dec 22, 2017

Collaborator

After all these changes related to "floating point map scale", the overall result is wonderful.
Seeing for instance the marker clustering operating during the pinch zoom gesture (without any code change needed on my side), that's really great.

@spyhunter99 & @monsieurtanuki Do you see something else absolutely required before releasing a new version? (could V6.0 land under the christmas tree?)

Collaborator

MKergall commented Dec 22, 2017

After all these changes related to "floating point map scale", the overall result is wonderful.
Seeing for instance the marker clustering operating during the pinch zoom gesture (without any code change needed on my side), that's really great.

@spyhunter99 & @monsieurtanuki Do you see something else absolutely required before releasing a new version? (could V6.0 land under the christmas tree?)

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Dec 22, 2017

Collaborator

@MKergall that can be arranged! depends on what you want to do for the zoon button overlay

Collaborator

spyhunter99 commented Dec 22, 2017

@MKergall that can be arranged! depends on what you want to do for the zoon button overlay

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Dec 23, 2017

Collaborator

@MKergall I'm glad you enjoyed the new pinch gesture!
Regarding pending issues I have an interest in:

  • #817 is minor but could be quickly fixed.
  • I'm currently working on #809, which is very interesting but not altogether a core feature, and which is already working - I'm just expanding the feature
    I don't see anything absolutely required before the new release.
Collaborator

monsieurtanuki commented Dec 23, 2017

@MKergall I'm glad you enjoyed the new pinch gesture!
Regarding pending issues I have an interest in:

  • #817 is minor but could be quickly fixed.
  • I'm currently working on #809, which is very interesting but not altogether a core feature, and which is already working - I'm just expanding the feature
    I don't see anything absolutely required before the new release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment