I think the issue is mainly champlain_view_ensure_visible being slow for some reason.

> * cancellable: if I drag the map during the movement, that should immediately
> stop the movement and do what I want.

Yeah, that makes sense. This should also be handled by champlain. The most we should do here is to set some boolean prop on the view.

> My initial thought was to try these two simultaneously:
> 1. pan to the center of the search results with ease-in-out, taking time 0 ->
> 2*t

This is what  champlain_view_ensure_visible is supposed to do. If needed, we could also add  champlain_view_ensure_visible_with_delay.

> 2. a) zoom out (if needed) to the zoom level where both the "from" location and
> the search results (or the route) could be visible, during time 0 -> t

That just means we should pass the currently location to MapView.ensureVisible() along with search results.

>    b) zoom in (if needed) so that all results are visible (or to an appropriate
> zoom level for the single results), during time t -> 2*t

From what I can tell from a quick look at champlain_view_ensure_visible, it simply choose an appropriate zoom-level where the bounding box is completely visible. I think that is quite sufficient, at least for now.

We discussed this in real life a while ago but I'll just document here too. The problem is two-fold:
* ensureVisible does two separate movements in a way that does not look totally smooth
* MapLocation does ensureVisible, and then two other distinct movements (view.go_to() and view.set_zoom_level())
So three to four separate zooms/pans and many of those have animations that accelerate and decelarate slowly in the beginning and end. A good example is first searching for Tampere, then Helsinki. Looks really ugly.

I'll see if I can add another version of ensureVisible() to ChamplainView that would do what we want.

Any progress on this?
Or any idea on how anoter version of ensureVisibility would behave?

The champlain_view_go_to[_with_duration] function has a CLUTTER_EASE_IN_OUT_CIRC animation mode. So that makes the two go_to functions that's performed have a jerky effect.

If I change the animation mode to CLUTTER_LINEAR for testing the effect is a somewhat smoother path. But kind of slow.

There is no way to control the animation mode tho. So should there be a new libchamplain go_to function that travels to multiple places? and does the ease in and ease-out on the whole trip. Or something else?

No progress from me, this was quite a bit more complex than I expected it to be. IIRC one thing that really makes major improvements difficult is the way zooming is implemented (with a separate zoom-actor): The way it's currently done just doesn't work with concurrent panning -- this is easy to test by using slower zoom-animation. Fixing the zoom is probably a major champlain modification and I guess it basically means implementing smooth (non-stepped) zooming, at least internally.

I can't remember the details now, but a low(er) hanging fruit is probably avoiding two separate go_to's either by adding something we can use to champlain or by working around in gnome-maps. Adding some control of animation modes to champlain might work as well, but I'm not sure that's easy without butchering the api. Either of these options would at least remove the weird accelerate-decelerate-accelerate-decelerate panning...

yes, I have spun two test patches, to see what direction one wants to go in.

It bothers the API i champlain a bit, but it does not change any public functions, but it adds two functions.

champlain_view_go_to_full and
champlaun_view_ensure_visibility_full

where you can specify duration and animation mode using:

typedef enum
{
  CHAMPLAIN_ANIMATION_EASE_IN = 0,
  CHAMPLAIN_ANIMATION_EASE_OUT,
  CHAMPLAIN_ANIMATION_EASE_IN_OUT
} ChamplainGoToAnimationMode;

I'm new to maps and champlain and the patches may reflect that :)

But maybe it can be a start. You can maybe apply them and see if the behavior is better? An added bonus is the control of the duration of the animations.

I have no idea if this is an acceptable approach for libchamplain, but if the maps people like it maybe we can push for it.

Posting patches below.

/Jonas

Created attachment 248140 [details] [review]
The libchamplain part

Created attachment 248141 [details] [review]
The maps part

Created attachment 248148 [details] [review]
version 2

The last one was buggy, use this instead.

Created attachment 248200 [details] [review]
Version 3

I expanded the approach a bit.


now adds:
champlain_view_go_to_with_duration
champlain_view_go_to_with_animation_mode
champlain_view_go_to_full

and:
champlain_view_ensure_visibility_with_duration
champlain_view_ensure_visibility_with_animation_mode
champlain_view_ensure_visibility_full

And a new enum for LINEAR animation mode.

The maps part now uses _with_animation_mode. And Linear mode.

Created attachment 248201 [details] [review]
version 2

So here is the new maps patch, for experimentation. I feel LINEAR feels most smooth. Later on you could to specify duration as well. Maybe add a function to calculate it from distance or something.

(In reply to comment #5)
> 
> But maybe it can be a start. You can maybe apply them and see if the behavior
> is better? An added bonus is the control of the duration of the animations.
> 

Busy right now but I might have time later this week.

The reason I'm generally a bit cautious about this approach is that it feels like we're adding API to workaround a bug...

Now, there may be good reasons to be able to fine tune the animation properties of the View, but we should make sure that really is the case: at least I'm not convinced yet (I'm not a champlain developer though). If we can't think of a real use case, then we should probably try to either workaround the issue in maps or actually fix whatever bugs are preventing us from having a useful function that "just works" and does the whole zoomout/pan/zoomin in champlain.

Basically, I think there should be a function like this in champlain:
  champlain_view_go_to_with_zoom (latitude, longitude, zoom_level);
The zoom level is needed because only the application knows what the user wants. The implementation should respect champlain_view_stop_go_to (this is a bit trickier because of the zooming involved). It should do the animated movement that includes (if needed) 
 - zooming out from old location to level where both locations
   are visible and then zooming in to new location
 - at the same time panning so it looks like a jump from one
   lcoation to another
In reality, knowing the limitations in champlain it might make more sense to do this for now:
 - calculate the needed zoom level to fit both locations,
   zoom out if needed
 - pan to new location
 - zoom in to new location
This is still three separate movements instead of one, but now it would be a champlain implementation detail to improve that...


Any comments on that? Did I forget anything?

> The reason I'm generally a bit cautious about this approach is that it feels
> like we're adding API to workaround a bug...
> 
> Now, there may be good reasons to be able to fine tune the animation properties
> of the View, but we should make sure that really is the case: at least I'm not
> convinced yet (I'm not a champlain developer though). If we can't think of a
> real use case, then we should probably try to either workaround the issue in
> maps or actually fix whatever bugs are preventing us from having a useful
> function that "just works" and does the whole zoomout/pan/zoomin in champlain.
> 

I mostly agree with you, and I guess this discussion would benefit from Champlain developers input.

Is this not a real use case though? I'm not experienced in Maps, so I'm going after the comment block in mapLocation.js:

"Lets first ensure that both current and destination location are visible before we start the animated journey towards destination itself. We do this to create the zoom-out-then-zoom-in effect that many map implementations do. This not only makes the go-to animation look a lot better visually but also give user a good idea of where the destination is compared to current location."

And the code does ensure_visible with the current location and the search result, to zoom out to where both are visible and moving to the center of that bounding box. Then "starts the animated journey" by using the go_to function.

So it's a case of doing a journey on a map, with two stops. And in that use case being forced to have ease-in-out on both is troublesome and a place where being able to control the animation mode is useful.

The exception of the maps developer that wrote this was that Champlain would accommodate the request. 

> Basically, I think there should be a function like this in Champlain:
>   champlain_view_go_to_with_zoom (latitude, longitude, zoom_level);
> The zoom level is needed because only the application knows what the user
> wants. The implementation should respect champlain_view_stop_go_to (this is a
> bit trickier because of the zooming involved). It should do the animated
> movement that includes (if needed) 
>  - zooming out from old location to level where both locations
>    are visible and then zooming in to new location
>  - at the same time panning so it looks like a jump from one
>    lcoation to another
> In reality, knowing the limitations in champlain it might make more sense to do
> this for now:
>  - calculate the needed zoom level to fit both locations,
>    zoom out if needed
>  - pan to new location
>  - zoom in to new location
> This is still three separate movements instead of one, but now it would be a
> champlain implementation detail to improve that...
> 
> Any comments on that? Did I forget anything?

I agree that being able to zoom and pan at the same time would be cool and something that should be possible in Maps using Champlain as well.
Isn't what you describe as "this for now:" what ensure_visible is doing? With the problem that it does ease-in-out on every go_to?

Created attachment 248995 [details] [review]
Champlain additions, version 4

Created attachment 248996 [details] [review]
Specify calculated duration and animation-mode in maps

And this is how it may look if we calculcate the distance between the two locations and do some calculation to get a duration.

Review of attachment 248995 [details] [review]:

Please open a separate bug on libchamplain for patches on that and make this bug dependent on that one.

Don't take resolution in the wrong way, this is not a review. :)

Review of attachment 248996 [details] [review]:

::: src/mapLocation.js
@@ +71,3 @@
 
+        let distance = this._calcDistance(locations[1], locations[0]);
+        let duration = Math.min(distance * 10, 3000);

whats this calculation based on? It needs a comment explaining it.

@@ +83,3 @@
                     }));
+                this._view.go_to_full(this.latitude, this.longitude,
+                                      (duration / 2),

if we are always dividing by 2, why not just do that once at initialization?

@@ +102,3 @@
     },
+
+    _calcDistance: function(from, to) {

* geocode-glib provides a function for this already: geocode_location_get_distance_from. That however requires both arguments be GeocodeLocation so you'll still want to keep this function to transform 'from' and 'to' into  GeocodeLocation intances and call geocode_location_get_distance_from on them.

* 'from' is redundant here as you can just use 'this' here.

::: src/mapView.js
@@ +116,3 @@
     },
 
+    ensureVisible: function(locations, duration) {

* You forgot to update the call to this function below which makes me wonder if you have tested 'place search' against this patch?

* 'duration' should be called 'animationDuration' as from the call itself, its not clear what duration is for.

* Is 'duration' arg actually needed BTW? If you are going to use the same value in the other call, its then just a constant and we should then use a constant for it then.

@@ +142,3 @@
+        this.view.ensure_visible_full(bbox,
+                                      duration,
+                                      Champlain.GoToAnimationMode.LINEAR);

I am also wondering if it would make sense to simply add properties to ChamplainView to set duration and animation mode instead of adding '_full' variants of all functions?

(In reply to comment #15)
> Review of attachment 248995 [details] [review]:
> 
> Please open a separate bug on libchamplain for patches on that and make this
> bug dependent on that one.
> 
> Don't take resolution in the wrong way, this is not a review. :)

:)

Well it might just be me not being used to bugzilla based development. What I was after was more of some RFC style patches. Do see what Maps want to do with this issue. If Maps people felt that the approach of adding control over duration and animation mode in champlain was needed I would open the bug over there. Maybe this type of discussion/proposals belong in IRC or mailing lists?

Se reply below for more comments.

(In reply to comment #16)
> Review of attachment 248996 [details] [review]:
> 
> ::: src/mapLocation.js
> @@ +71,3 @@
> 
> +        let distance = this._calcDistance(locations[1], locations[0]);
> +        let duration = Math.min(distance * 10, 3000);
> 
> whats this calculation based on? It needs a comment explaining it.

Just a naive example of the speed changes when you can control the duration.
Obviously it would be a nicer function, if we want to do our own duration calculation. 


> 
> @@ +142,3 @@
> +        this.view.ensure_visible_full(bbox,
> +                                      duration,
> +                                      Champlain.GoToAnimationMode.LINEAR);
> 
> I am also wondering if it would make sense to simply add properties to
> ChamplainView to set duration and animation mode instead of adding '_full'
> variants of all functions?

I think that is a much nicer approach. And if Maps want to go this route, and allow for more control over what is now champlain internals, I could cook up a patch like that and add to champlain bugzilla.

But that assumes we want to do this. I know Mattias felt it was an improvement with these patches, but still not good enough (?). So maybe all this is futile.

So the discussion on how to continue with this issue remains. Should the zoom function in champlain be reworked?

Should all this be a bug filed agains champlain? With the Maps use-case described in the bug? Or is this solvable purely in Maps, or a combination like my patches. 

Comments?

(In reply to comment #17)
> (In reply to comment #15)
> > Review of attachment 248995 [details] [review] [details]:
> > 
> > Please open a separate bug on libchamplain for patches on that and make this
> > bug dependent on that one.
> > 
> > Don't take resolution in the wrong way, this is not a review. :)
> 
> :)
> 
> Well it might just be me not being used to bugzilla based development.

Its ok, not many people are.

> What I
> was after was more of some RFC style patches. Do see what Maps want to do with
> this issue. If Maps people felt that the approach of adding control over
> duration and animation mode in champlain was needed I would open the bug over
> there.

I think its also important to involve libchamplain developers from beginning, even if patches are RFC. Would avoid the situation of us deciding we want them after a lot of discussion and then libchamplain devs later tell us they are not acceptable.

> Maybe this type of discussion/proposals belong in IRC or mailing lists?

Don't think so, what I suggested is how things are usually done in GNOME projects.

(In reply to comment #18)
> (In reply to comment #16)
> > Review of attachment 248996 [details] [review] [details]:
> > 
> > ::: src/mapLocation.js
> > @@ +71,3 @@
> > 
> > +        let distance = this._calcDistance(locations[1], locations[0]);
> > +        let duration = Math.min(distance * 10, 3000);
> > 
> > whats this calculation based on? It needs a comment explaining it.
> 
> Just a naive example of the speed changes when you can control the duration.
> Obviously it would be a nicer function, if we want to do our own duration
> calculation. 
> 
> 
> > 
> > @@ +142,3 @@
> > +        this.view.ensure_visible_full(bbox,
> > +                                      duration,
> > +                                      Champlain.GoToAnimationMode.LINEAR);
> > 
> > I am also wondering if it would make sense to simply add properties to
> > ChamplainView to set duration and animation mode instead of adding '_full'
> > variants of all functions?
> 
> I think that is a much nicer approach. And if Maps want to go this route, and
> allow for more control over what is now champlain internals, I could cook up a
> patch like that and add to champlain bugzilla.
> 
> But that assumes we want to do this. I know Mattias felt it was an improvement
> with these patches, but still not good enough (?). So maybe all this is futile.

Did he concider this alternative when making the decision about this approach being good enough? How is all this futile just because we might do a different libchamplain API? You'll just need to rework your patches, which is what you should expect from reviews, especially since you yourself say that your patches are just RFC.

I could have saved you some time but I was on vacation when you started cooking these patches.

> So the discussion on how to continue with this issue remains. Should the zoom
> function in champlain be reworked?
> 
> Should all this be a bug filed agains champlain? With the Maps use-case
> described in the bug? Or is this solvable purely in Maps, or a combination like
> my patches. 

We already have this bug for Maps side, now we need a bug on libchamplain side. Please put your libchamplain patches in there and in the comment mention about the alternative I proposed. I'll try to participate in there too. The bug dependency will tell libchamplain devs why we need this new API.

Sorry for the late response - a deadly combination of holiday time, being busy with something else and hot weather ;-).

There are several things which complicate the go_to animation in libchamplain:

1. The tiles on the way from point A to point B are not loaded and the animation is too fast so you get empty tiles instead of moving map. I've just tried with Google's Android maps and it seems they prefetch the map (they use a vector map contrary to our bitmap tile-based map) before the animation starts - there's some delay before they start animating the map where I think they load some low-detail version of what's on the way.

2. While the map is panning, the tiles on the way are being downloaded and when they disappear, the download is cancelled. Unfortunately when panning over a large distance the tiles are entering and leaving the visible viewport too quickly so what happens is just a big number of GET requests followed by cancellation of the requests without actually displaying any tiles. All these requests can however slow-down the animation.

3. The zooming in libchamplain is done in steps - it's not fluid like in the Android maps - again because of the bitmap tiles. When the tiles are resized, the text descriptions get blurry and the map looks really bad.

4. The zoom-in/zoom-out animations currently work this way: the currently visible tiles are taken, inserted in an auxiliary zoom actor and this actor is then up/down-scaled with the center in the current mouse position. This works when you just zoom-in/zoom-out but if you want to zoom while panning this will be a problem because you pan outside of the previously visible area for which you don't have tiles and like in (1) you'll be zooming in the area without loaded tiles.

I haven't had time to check the attached patches yet but if they improve the situation, there should be no problem to apply them to libchamplain. I'm just afraid that properly fixing all the above issues is a huge amount of work and maybe the nicest and least annoying solution to go from place A to place B right now is just to avoid the animation completely ;-).

(In reply to comment #21)
> Sorry for the late response - a deadly combination of holiday time, being busy
> with something else and hot weather ;-).
> 
> There are several things which complicate the go_to animation in libchamplain:

This is a good summary of issues. If we cannot achieve nice results with the current zoom-actor-method, then no animation might be a better choice for the mid-term: Fixing #4 (so zooming would happen without a zoom-actor) looks like a big job indeed.

> 1. The tiles on the way from point A to point B are not loaded and the
> animation is too fast so you get empty tiles instead of moving map. I've just
> tried with Google's Android maps and it seems they prefetch the map (they use a
> vector map contrary to our bitmap tile-based map) before the animation starts -
> there's some delay before they start animating the map where I think they load
> some low-detail version of what's on the way.

I guess we could do the same thing with the zoom-actor approach? Load a small number of lower resolution tiles that cover the whole pan-zoom area and put that on the zoom-actor -- could still add the already loaded more accurate tiles on top of the lower resolution tile.
 
> 2. While the map is panning, the tiles on the way are being downloaded and when
> they disappear, the download is cancelled. Unfortunately when panning over a
> large distance the tiles are entering and leaving the visible viewport too
> quickly so what happens is just a big number of GET requests followed by
> cancellation of the requests without actually displaying any tiles. All these
> requests can however slow-down the animation.

Apart from internal request-throttling (which only helps with slow-down, not with missing tiles) I can only think of one solution: Use lower resolution tiles (as mentioned above): I guess every lower resolution level should cut the request number and file size by ~75%.

I've just played with Android, iOS and the web google maps (both the classical and the "new" version) a bit. 

In Android they don't seem to do anything special - they basically do what I described - zoom out, pan, and zoom into a void place without any map (which is then loaded). Of course, when the place was already visited before, the tiles are loaded from the cache so the target area is visible immediately. But even when it's in the cache the location between the origin and the target remains void.

All the remaining maps just simply jump to the target which I personally find nicer. This means I don't feel really motivated to implement this feature by myself but if someone has a patch with a reasonably looking animation I'll be happy to apply it and let the application developers decide what transition they wish to use.

Just one more comment to the lower-resolution tiles - this is of course possible (in the extreme case it could be just one tile which contains both the source and the target location) but at some point during the animation you have to somehow substitute the detailed map with the low res map, which might look strange. The Android implementation has a clear advantage here because they have a fluid zoom (see (3) in my previous post) and don't have to switch between different zoom levels.

(In reply to comment #20)
> > > 
> > > @@ +142,3 @@
> > > +        this.view.ensure_visible_full(bbox,
> > > +                                      duration,
> > > +                                      Champlain.GoToAnimationMode.LINEAR);
> > > 
> > > I am also wondering if it would make sense to simply add properties to
> > > ChamplainView to set duration and animation mode instead of adding '_full'
> > > variants of all functions?
> > 
> > I think that is a much nicer approach. And if Maps want to go this route, and
> > allow for more control over what is now champlain internals, I could cook up a
> > patch like that and add to champlain bugzilla.
> > 
> > But that assumes we want to do this. I know Mattias felt it was an improvement
> > with these patches, but still not good enough (?). So maybe all this is futile.
> 
> Did he concider this alternative when making the decision about this approach
> being good enough? How is all this futile just because we might do a different
> libchamplain API? You'll just need to rework your patches, which is what you
> should expect from reviews, especially since you yourself say that your patches
> are just RFC.
> 
> I could have saved you some time but I was on vacation when you started cooking
> these patches.


Oh, I expected to rework the patches, and I am very grateful for the review work! I meant that without the maps use-case the champlain API change might not make sense. But you are of course right that is up to the libchamplain people.

> 
> > So the discussion on how to continue with this issue remains. Should the zoom
> > function in champlain be reworked?
> > 
> > Should all this be a bug filed agains champlain? With the Maps use-case
> > described in the bug? Or is this solvable purely in Maps, or a combination like
> > my patches. 
> 
> We already have this bug for Maps side, now we need a bug on libchamplain side.
> Please put your libchamplain patches in there and in the comment mention about
> the alternative I proposed. I'll try to participate in there too. The bug
> dependency will tell libchamplain devs why we need this new API.

Sure! Will do that. Thanks!

I think we are getting distracted from the real issue here. While tile loading might in some cases slow down animations, I really have not seen that happening in Maps. I have seen the tile loading happening very slowly at slow networks (GUADEC) but I haven't seen it affecting animations.

Jussi has already described the issue very nicely in comment#2 so I won't get into details. Also, I think the patches from Jonas should solve the issue, just that we need to figure if the libchamplain API he is proposing is what we want to go for. There is also my alternative suggestion in comment#16 (at the end).

Jonas: Just a reminder: IIRC you agreed to put the libchamplain patches in a separate bug. If you have done it already, please add that bug to deps here. Also please rebase them so we could test.

Created attachment 251339 [details] [review]
Test patch for the libchamplain API proposal

Hi,

The proposal has been filed in a new libchamplain bug.
This patch allows you to test maps with LINEAR animation mode.

Thanks.

Created attachment 251567 [details] [review]
MapView: set view goto-animation-mode to LINEAR

The patch to add properties for animation mode and duration has landed in libchamplain master. So here is a patch to set the goto-animation-mode to LINEAR.

Please try it out and see if improves matters. And also think if we want to improeve the duration calculation over the default in champlain.

Thanks

Review of attachment 251567 [details] [review]:

Yeah, it does improve the animation for sure. We definitely need to improve on duration still. I think it makes perfect sense to have duration be based on zoom-level so maybe that needs to be improved in champlain itself? Also since we have two different animations involved, shouldn't we be needing to set mode and duration for ensure_visible() or is that done as part of goto-animation for some reason? If its the latter, we should document that in the new props of ChamplainView.

Yeah, ensure_visible in champlain ends with doing a go_to so it is the same property that controls the duration and mode for it.

Do you think that it shouldn't be? That we should make special properties for it?
In any case maybe that should be documented.

(In reply to comment #29)
> Yeah, ensure_visible in champlain ends with doing a go_to so it is the same
> property that controls the duration and mode for it.
> 
> Do you think that it shouldn't be? That we should make special properties for
> it?

No, I wasn't just sure which way it is.

> In any case maybe that should be documented.

Most definitely! Something like "Please note that animation of #champlain_view_ensure_visible also involves a 'goto' animation".

Patch rebased and pushed.