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

Zoom Button Crash in 6.1.0 #1299

Closed
InI4 opened this issue Mar 25, 2019 · 39 comments

Comments

@InI4
Copy link

commented Mar 25, 2019

Issue Type

[X] Bug

Description and/or steps/code to reproduce the problem

Plugging 6.1.0 into gradle to replace 6.0.3 in my App, all things so far seem fine.
All but the (standard) zoom buttons: These bottons relyably crash 100% of the time after app start. With plenty of stack trace which all seem repeations of this (one click creates hundreds and so far I only checked manually):

2019-03-25 09:26:07.704 ? E/AndroidRuntime:     at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
2019-03-25 09:26:07.704 ? E/AndroidRuntime:     at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1133)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
        at android.animation.ValueAnimator.animateBasedOnTime(ValueAnimator.java:1339)
        at android.animation.ValueAnimator.doAnimationFrame(ValueAnimator.java:1471)
        at android.animation.AnimationHandler.doAnimationFrame(AnimationHandler.java:146)
        at android.animation.AnimationHandler.access$100(AnimationHandler.java:37)
        at android.animation.AnimationHandler$1.doFrame(AnimationHandler.java:54)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1170)
        at android.view.Choreographer.doCallbacks(Choreographer.java:984)
        at android.view.Choreographer.doFrame(Choreographer.java:806)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1158)
        at android.os.Handler.handleCallback(Handler.java:873)
2019-03-25 09:26:07.704 ? E/AndroidRuntime:     at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:193)
        at android.app.ActivityThread.main(ActivityThread.java:6863)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:537)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

Once the user has done some manual (gesture or in my App location bookmarks) zooming or location change, this does not happen any more. But when the user hits zoom directly after start it crashes relyably.

The stack traces seem to depend on Android version, on Android 4.2.2 I get the following:

03-25 09:42:27.976 de.spieleck.app.badgers.debug E/AndroidRuntime: FATAL EXCEPTION: main
java.lang.StackOverflowError

(which matches to the amount of output on Android 9 where log is just to fast to keep the start of problems in Android Studio memory).

Environment

My Badge(r)s App, Android 4.2.2, 9 checked here

If it's a bug, version(s) of android this affects:

A9, 4.2.2

Version of osmdroid the issue relates to:

6.1.0

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2019

@InI4 What do you mean by "standard" zoom buttons? Android standard zoom buttons or the new CustomZoomButtonsController zoom buttons?

Your issue looks like an init issue.

Currently I have some problems with Android Studio; I'll have a look at your issue a bit later.

@InI4

This comment has been minimized.

Copy link
Author

commented Mar 25, 2019

CustomZoomButtonsController. Yes, looks like something in the initialization is missing.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

@InI4 I couldn't reproduce the bug doing that:

  • uninstalling the demo app
  • in StarterMapFragment, changing the code to mMapView.getZoomController().setVisibility(CustomZoomButtonsController.Visibility.ALWAYS); (instead of SHOW_AND_FADEOUT)
  • compiling/starting the demo app, selecting "OSMDroid Sample map (Start Here)"
  • the zoom buttons are immediately visible - I click on one of them, and the zoom animation is correct

I think that's the closest I can get in order to reproduce the bug.

I believe that the old standard zoom buttons didn't appear unless you move the map.
I therefore assume that in your code you never handled the case when zoom buttons were ALWAYS visible, and that the "init bug" is on your side. Does that make sense?

@atomic7777

This comment has been minimized.

Copy link

commented Mar 29, 2019

I have exactly the same problem (both my working app and a simple test app with MapView and LocationOverlay).
Tested on Android 5 (AVD) and Android 9 (Samsung S9)
It only happens when zoom in / out just after app start. I use the buttons that appear after tapping
The beginning of my error log is:

    --------- beginning of crash
2019-03-29 10:05:25.480 28752-28752/pl.at7.testapp E/AndroidRuntime: FATAL EXCEPTION: main
    Process: pl.at7.testapp, PID: 28752
    java.lang.StackOverflowError: stack size 8MB
        at android.util.LongSparseLongArray.clear(LongSparseLongArray.java:217)
        at android.view.View.requestLayout(View.java:21977)
        at org.osmdroid.views.MapView.setMapScroll(MapView.java:1781)
        at org.osmdroid.views.MapView.setExpectedCenter(MapView.java:1807)
        at org.osmdroid.views.MapView.setExpectedCenter(MapView.java:1823)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:480)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1522)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1110)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)
        at org.osmdroid.views.MapController$MapAnimatorListener.onAnimationUpdate(MapController.java:520)
        at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1522)
        at android.animation.ValueAnimator.end(ValueAnimator.java:1110)
        at org.osmdroid.views.MapController.animateTo(MapController.java:167)
        at org.osmdroid.views.MapController.animateTo(MapController.java:137)
        at org.osmdroid.views.MapController.animateTo(MapController.java:183)
        at org.osmdroid.views.MapController.animateTo(MapController.java:129)
        at org.osmdroid.views.MapView.setZoomLevel(MapView.java:492)

...and repeated several times

My code in 'empty' MainActivity:

    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
 /* permissions here*/

        setContentView(R.layout.activity_main);
        Context ctx = getApplicationContext();
        Configuration.getInstance().load(ctx, PreferenceManager.getDefaultSharedPreferences(ctx));
        map = findViewById(R.id.map);
        mapController = map.getController();
        mapController.setZoom(16.0);
        map.setTileSource(TileSourceFactory.MAPNIK);

        GpsMyLocationProvider provider = new GpsMyLocationProvider(ctx);
        provider.addLocationSource(LocationManager.GPS_PROVIDER);
        MyLocationNewOverlay mLocationOverlay = new MyLocationNewOverlay(provider, map);
        mLocationOverlay.enableMyLocation();
        mLocationOverlay.enableFollowLocation();
  }
    public void onResume(){
        super.onResume();
        map.onResume(); 
    }

    public void onPause(){
        super.onPause();
        map.onPause();
    }
@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Could it be because you don't have an explicit map geo center?
For test's sake, could you add something like

mMapView.post(new Runnable() {
	@Override
	public void run() {
		mMapView.setExpectedCenter(new GeoPoint(0., 0));
	}
});

and see if it's still crashing?

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

@atomic7777 @shorti1996 Could it be a side effect of #1237?

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Looks related but how to reproduce it?
I cannot make it crash on OSM sample app.

@atomic7777

This comment has been minimized.

Copy link

commented Mar 29, 2019

@monsieurtanuki It could be. In one of my apps the problem disappears, when I'm not using enableFollowLocation.
setExpectedCenter didn't help.
Sometimes it works without any crash
Now I'm trying to reproduce it. Here is my simple app https://github.com/atomic7777/osm-testapp
The error in my first post

2019-03-29 10:05:25.480 28752-28752/pl.at7.testapp E/AndroidRuntime: FATAL EXCEPTION: main
    Process: pl.at7.testapp, PID: 28752
    java.lang.StackOverflowError: stack size 8MB
        at android.util.LongSparseLongArray.clear(LongSparseLongArray.java:217)
        at android.view.View.requestLayout(View.java:21977)
        at org.osmdroid.views.MapView.setMapScroll(MapView.java:1781)

is only on physical device, but on virtual one I get:

03-29 12:41:58.499 21065-21065/pl.at7.testapp E/AndroidRuntime: FATAL EXCEPTION: main
    Process: pl.at7.testapp, PID: 21065
    java.lang.StackOverflowError: stack size 8MB
        at org.osmdroid.util.TileSystemWebMercator.getX01FromLongitude(TileSystemWebMercator.java:16)
        at org.osmdroid.util.TileSystem.getX01FromLongitude(TileSystem.java:221)
        at org.osmdroid.util.TileSystem.getMercatorXFromLongitude(TileSystem.java:489)
        at org.osmdroid.views.Projection.<init>(Projection.java:94)
        at org.osmdroid.views.Projection.<init>(Projection.java:65)
@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

The project you specified works just fine on Android 9.0
I will check on 4.4 as it's the oldest Android I have right now.

To reproduce the crash I have to click the zoom buttons as soon as the app loads?

Edit:
The same on Android 4.4. I can't make it crash with zoom buttons.

@InI4

This comment has been minimized.

Copy link
Author

commented Mar 29, 2019

My app has a setting to switch zoom buttons between off, dynamic and always. All tests have been done in the always setting, with both 6.0.3 and 6.1.0. Something is not working in 6.1.0. Also "followLocation" is enabled on start of my app.

It is an StackOverFlow, so one could expect, something gets into infinite recursion...

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Ok, I got it.
You should PERFECTLY center the map to your location (so for example using enableFollowLocation and wait for it to center). Then the app crashes because there is an infinite recursion.

It's caused by the fact that each time the zoom updates, osm tries to "snap" to the user location using org.osmdroid.views.overlay.mylocation.MyLocationNewOverlay#onSnapToItem .

I can revert #1237 for now and try to fix both issues later.

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

It can be fixed by replacing the line at /osmdroid/views/MapController.java:167 in org.osmdroid.views.MapController#animateTo(org.osmdroid.api.IGeoPoint, java.lang.Double, java.lang.Long, java.lang.Float, java.lang.Boolean)
which is mCurrentAnimator.end(); with mapAnimatorListener.onAnimationCancel(mCurrentAnimator);.

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Can you try to reproduce the issue with this aar?
osmdroid-android-6.1.1-SNAPSHOT-debug.zip

shorti1996 added a commit to shorti1996/osmdroid that referenced this issue Mar 30, 2019

bug osmdroid#1299: Zoom Button Crash in 6.1.0
fix StackOverflow, because when zooming in with zoom buttons,
osm tries to keep the map centered to the current user location.
See: org.osmdroid.views.overlay.mylocation.MyLocationNewOverlay.onSnapToItem

shorti1996 added a commit to shorti1996/osmdroid that referenced this issue Mar 30, 2019

bug osmdroid#1299: Zoom Button Crash in 6.1.0
fix StackOverflow, because when zooming in with zoom buttons,
osm tries to keep the map centered to the current user location.
See: org.osmdroid.views.overlay.mylocation.MyLocationNewOverlay.onSnapToItem
mCurrentAnimator's listener animateTo
@atomic7777

This comment has been minimized.

Copy link

commented Mar 30, 2019

It's working correctly with this aar, no more errors.

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2019

if we are all in concur on this, i can merge and get a new version rolled out today or tomorrow

@InI4

This comment has been minimized.

Copy link
Author

commented Mar 30, 2019

Working nicely here. Good job!

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

It should be merged.
However, the "snap to" animation cannot be cancelled with touch event (as it was before #1237). But at least other map centering animations are still cancellable.
It destroys user experience and grinds my gears. We should look into it in future.
Now the crash fix is top priority.

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

@monsieurtanuki thoughts on this? I'm a bit on the edge to merge as this. It fixes a problem but creates another. I'm not sure how to resolve the secondary issue

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

@spyhunter99 I have very limited time for dev until June.
The best I can do today is to re-implement the standard Android zoom buttons as the default zoom implementation in MapView - assuming that the new custom zoom buttons are the only reason of the crash.
Allegedly, things will work well - except that the zoom buttons are "old-style", which is acceptable. You guys that experience the crash will have to test it: if it's OK, it's a GO! for a new version of osmdroid.

Afterwards, it's about working on a fix for this current issue, that would work with the (then) non-default custom zoom buttons.

monsieurtanuki added a commit that referenced this issue Apr 9, 2019

bug/#1299 - partial rollback of the custom zoom buttons
We temporarily roll back to the standard Android zoom buttons, as the default zoom controller.
If a developer wants to switch to the new custom zoom buttons, `mMapView.getZoomController()` must explicitly be called, in something like `mMapView.getZoomController().setVisibility(CustomZoomButtonsController.Visibility.SHOW_AND_FADEOUT)`.

Impacted classes:
* `MapView`: partial roll back to the standard Android zoom controller - the custom zoom controller is still there, but is silent (default behavior)
* `SampleWithTilesOverlayAndCustomTileSource`: removed a useless zoom control code (which is already the default mode)
* `StarterMapFragment`: removed a useless zoom control code (which is already the default mode)

monsieurtanuki added a commit that referenced this issue Apr 9, 2019

bug/#1299 - partial rollback of the custom zoom buttons
We temporarily roll back to the standard Android zoom buttons, as the default zoom controller.
If a developer wants to switch to the new custom zoom buttons, `mMapView.getZoomController()` must explicitly be called, in something like `mMapView.getZoomController().setVisibility(CustomZoomButtonsController.Visibility.SHOW_AND_FADEOUT)`.

Impacted classes:
* `MapView`: partial roll back to the standard Android zoom controller - the custom zoom controller is still there, but is silent (default behavior)
* `SampleWithTilesOverlayAndCustomTileSource`: removed a useless zoom control code (which is already the default mode)
* `StarterMapFragment`: removed a useless zoom control code (which is already the default mode)

[actually, a desperate attempt to PR a code that github just ignored]
@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

Just PR'ed a partial rollback of #1190 (custom zoom buttons) in #1315.

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Is that necessary? The problem was related to handling animation controllers and stuff.
The problem was the animation was cancelled -> on zoom animation end map is centered (snapped) to the user position -> which cancelled animation -> on animation cancel snap center etc. resulting in StackOverflow.
See: org.osmdroid.views.overlay.mylocation.MyLocationNewOverlay.onSnapToItem

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

@shorti1996 I don't know if it is necessary: I was not able to reproduce the issue and didn't participate to the fix.

My point was only to say that if for some reason the new custom zoom provoked side-effect regressions (and the concept of custom zoom seems easy but we already had bad surprises with a former version) we can bypass the new custom zoom controller and switch back to the standard one.

From one of your latest posts, I understood that you found regressions in the behavior of the snap action. But if there's no regression or crash with the 6.1.0 custom zoom controller and your latest fix, then everything's fine and we can create a new allegedly bug-free version of osmdroid.

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

#1307 fixes that and is enough to get rid of the issue.

spyhunter99 added a commit that referenced this issue Apr 21, 2019

Bug/#1299 2 - partial rollback of the custom zoom buttons (#1315)
* bug/#1299 - partial rollback of the custom zoom buttons

We temporarily roll back to the standard Android zoom buttons, as the default zoom controller.
If a developer wants to switch to the new custom zoom buttons, `mMapView.getZoomController()` must explicitly be called, in something like `mMapView.getZoomController().setVisibility(CustomZoomButtonsController.Visibility.SHOW_AND_FADEOUT)`.

Impacted classes:
* `MapView`: partial roll back to the standard Android zoom controller - the custom zoom controller is still there, but is silent (default behavior)
* `SampleWithTilesOverlayAndCustomTileSource`: removed a useless zoom control code (which is already the default mode)
* `StarterMapFragment`: removed a useless zoom control code (which is already the default mode)

* bug/#1299 - partial rollback of the custom zoom buttons

We temporarily roll back to the standard Android zoom buttons, as the default zoom controller.
If a developer wants to switch to the new custom zoom buttons, `mMapView.getZoomController()` must explicitly be called, in something like `mMapView.getZoomController().setVisibility(CustomZoomButtonsController.Visibility.SHOW_AND_FADEOUT)`.

Impacted classes:
* `MapView`: partial roll back to the standard Android zoom controller - the custom zoom controller is still there, but is silent (default behavior)
* `SampleWithTilesOverlayAndCustomTileSource`: removed a useless zoom control code (which is already the default mode)
* `StarterMapFragment`: removed a useless zoom control code (which is already the default mode)

[actually, a desperate attempt to PR a code that github just ignored]
@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented Apr 21, 2019

i just merged @monsieurtanuki pr as it should fix the problem and not cause adverse effects. If we can find a way to make @shorti1996 's PR work and resolve the snap to cancel issue, then that can be merged and the 'revert back to android zoom buttons' can be reversed

@pasniak

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I tried b6ea341 and 0a8a64a to no avail. Finally reverted to 6.0.3, to make zooming work.

I have controller.zoomIn() wired to keyboard's +-, so on emulator I can zoom without touching; this crashes 100% of time.

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

ahh, that helps narrow it down. i'll look into this shortly

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2019

@pasniak what kind of emulator are you using? android86? genymotion? the google provided avd?

unfortunately, google doesn't support AMD processors so i can't run avd. Genymotion doesn't seem to bind in the keyboard, however android86 does work with the keyboard. Crashing is not present on the current master commit.

@pasniak

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I use avd (AS 3.4 on Ubuntu/Intel). I find android keyboard hooks v useful. Maybe you just need to send few zoomIn events:

    override fun onKeyUp(keyCode: Int, event: KeyEvent?): Boolean {
        mapViewWrap.run {
            return when (keyCode) {
                KeyEvent.KEYCODE_8 -> simulateGpsLocationFromKeyboard()
                KeyEvent.KEYCODE_W -> simulateMovingFwdFromKeyboard()
                KeyEvent.KEYCODE_A -> simulateTurningLeftFromKeyboard()
                KeyEvent.KEYCODE_D -> simulateTurningRightFromKeyboard()
                KeyEvent.KEYCODE_EQUALS -> zoomIn()
                KeyEvent.KEYCODE_MINUS -> zoomOut()
                KeyEvent.KEYCODE_DPAD_DOWN -> down()
                KeyEvent.KEYCODE_DPAD_UP -> up()
                KeyEvent.KEYCODE_DPAD_LEFT -> left()
                KeyEvent.KEYCODE_DPAD_RIGHT -> right()
                KeyEvent.KEYCODE_DPAD_CENTER -> zoomInToLastKnownLocation()
                KeyEvent.KEYCODE_N -> { showNearestFragment(); true }
                KeyEvent.KEYCODE_P -> OpenAddWptToRouteEvent().sendRx()
                else -> super.onKeyUp(keyCode, event)
            }
        }
    }

in class inheriting from MapView:
    fun zoomIn(): Boolean {
        return mapView?.controller?.zoomIn() ?: false
    }
@pasniak

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

I created this patch to get keyboard working in Android Emulator:

Index: OpenStreetMapViewer/src/main/java/org/osmdroid/ExtraSamplesActivity.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- OpenStreetMapViewer/src/main/java/org/osmdroid/ExtraSamplesActivity.java	(revision b6ea341a9bc2658011f827f57edeaf3baea51fc8)
+++ OpenStreetMapViewer/src/main/java/org/osmdroid/ExtraSamplesActivity.java	(date 1558657192000)
@@ -63,12 +63,15 @@
         if (mMapView==null)
             return super.onKeyUp(keyCode,event);
         switch (keyCode) {
-            case KeyEvent.KEYCODE_PAGE_DOWN:
+            case KeyEvent.KEYCODE_EQUALS:
                 mMapView.getController().zoomIn();
                 return true;
-            case KeyEvent.KEYCODE_PAGE_UP:
+            case KeyEvent.KEYCODE_MINUS:
                 mMapView.getController().zoomOut();
                 return true;
+            case KeyEvent.KEYCODE_BACKSLASH:
+                mMapView.getController().zoomTo(12);
+                return true;
         }
         return super.onKeyUp(keyCode,event);
     }

With the change above, I tried to zoom to 12 (pressing BACKSLASH, which is what I start at in my app) in Osmdroid tests (ExtraSamplesActivity), and then out (MINUS) which I understand zooms out to 11. The zoomAnimatorListener are the same in ExtraSamplesActivity and my app; in ExtraSamplesActivity it seems to work fine... In my app I hit zoom 11 and keep hitting 11 in an endless loop leading to stack overflow (valueAnimator.getAnimatedValue() keeps on returning the same value of 1.0f):
image

On both sides I am at b6ea341

@pasniak

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Success today!

The issue is related to any app having a Snappable overlay (an OwnLocationOverlay):

This code in setZoomLevel

if (this.getOverlayManager().onSnapToItem((int) mMultiTouchScaleInitPoint.x,

gets true from onSnapItem

and continuously calls animateTo leading to SO:

			if (this.getOverlayManager().onSnapToItem((int) mMultiTouchScaleInitPoint.x,
					(int) mMultiTouchScaleInitPoint.y, snapPoint, this)) {
				IGeoPoint geoPoint = pj.fromPixels(snapPoint.x, snapPoint.y, null, false);
				getController().animateTo(geoPoint);
			}

and onSnapItem:

    public boolean onSnapToItem(final int x, final int y, final Point snapPoint, final IMapView pMapView) {
        for (final Overlay overlay : this.overlaysReversed()) {
            if (overlay instanceof Snappable) {
                if (((Snappable) overlay).onSnapToItem(x, y, snapPoint, pMapView)) {
                    return true;

I could repro stack overflow by hardcoding return true; in onSnapToItem; patch this below.and zoom out to stack overflow at will in Osmdroid-android test app!

Index: osmdroid-android/src/main/java/org/osmdroid/views/overlay/DefaultOverlayManager.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- osmdroid-android/src/main/java/org/osmdroid/views/overlay/DefaultOverlayManager.java	(revision b6ea341a9bc2658011f827f57edeaf3baea51fc8)
+++ osmdroid-android/src/main/java/org/osmdroid/views/overlay/DefaultOverlayManager.java	(date 1558697029000)
@@ -265,11 +265,12 @@
     @Override
     public boolean onSnapToItem(final int x, final int y, final Point snapPoint, final IMapView pMapView) {
         for (final Overlay overlay : this.overlaysReversed()) {
-            if (overlay instanceof Snappable) {
-                if (((Snappable) overlay).onSnapToItem(x, y, snapPoint, pMapView)) {
-                    return true;
-                }
-            }
+//            if (overlay instanceof Snappable) {
+//                if (((Snappable) overlay).onSnapToItem(x, y, snapPoint, pMapView)) {
+//                    return true;
+//                }
+//            }
+            return true;
         }
 
         return false;
@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

awesome, sorry i must have missed this. i'll get this merged as soon as possible and roll back in the newer zoom buttons. assuming it works i can cut a release this weekend

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

on second thought, i don't know enough about how snappable is used. if i comment that bit out we are effectively disabling it

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

I believe snappable is used to keep the exact center of the map when zooming in with buttons.
If the user zooms in or out due to accuracy loss, the center of the map can be slightly off comparing to the previous state of zoom.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

Overlay.Snappable.onSnapToItem is only called in MapView.setZoomLevel: when it returns true, MapController.animateTo is called.
Does it call then several times MapView.setZoomLevel during the animation? If so, that would explain the stack overflow. But I couldn't find where.

Maybe there's a safer way to handle this behavior in MapView.setZoomLevel:

  • Overlay.Snappable.onSnapToItem is called
  • if it returns true, we check MapView.isAnimating, and call only MapController.setCenter if it's true, and MapController.animateTo if it's false

Anyway I need to set up a demo for OverlaySnappable, in order to reproduce the stack overfow. And hopefully fix it.

The point being to keep the Snappable effect and avoid stack overflows.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

Good news: I managed to reproduce the bug!
I also managed to fix it more or less, with a MapView.isAnimating() check as planned: no more stack overflow, and the snap feature is working.

That being said, I'm not sure I understand the full scope of the snap feature.
So far, it's implemented this way:

  • if the zoom level changes
  • and if at least one Overlay sees that the pinch starting point is close enough to an interesting point (fishy assumption: the zoom level changed because of a pinch gesture - what about the zoom buttons?)
  • then the map should animate to the interesting point, assuming that we're still in the pinch animation phase

The pinch gesture has priority:

  • while I'm pinching, everything works according to my pinch gesture
  • when I stop pinching, the map will snap to the snap interesting point only if I'm still in the pinch gesture animation phase
  • therefore if I stop pinching but keep my fingers still on the screen for a couple of seconds, the animation will be ended and the map won't go to the snap point

Anyone there has an idea of the way the snap feature should work for the end-users? I tried to find something about it in Google Maps but there only a "snap to road" feature, that works basically without zoom level change.

@shorti1996

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

The snap is here because when zooming in or out with zoom buttons, the map center is slightly off (probably due to double/float precision) and that snap tries to keep the map perfectly centered.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@shorti1996 What you describe is what a snap would mean in the context of zoom in/out buttons.
That makes sense.

But there's also an Overlay.Snappable.onSnapToItem method, and I'm not sure of its purpose / typical use case.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

I've just merged #1371, which adds a demo dedicated to the "snap" feature ("More Samples / Events / Snappable").
With @shorti1996's #1307 (and without the fix I worked on yesterday), the behavior seems correct.

spyhunter99 added a commit that referenced this issue Jul 20, 2019

bug #1299: Zoom Button Crash in 6.1.0 (#1307)
fix StackOverflow, because when zooming in with zoom buttons,
osm tries to keep the map centered to the current user location.
See: org.osmdroid.views.overlay.mylocation.MyLocationNewOverlay.onSnapToItem

@spyhunter99 spyhunter99 added this to the v6.1.1 milestone Jul 20, 2019

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

commented Jul 20, 2019

awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.