GNOME Bugzilla – Bug 760256
Make map animations slightly more tolerable
Last modified: 2016-01-30 10:43:31 UTC
A few things: * Reorder the animation callbacks. There's some sort of bizzarre race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming in instead of out to pan, and the map zooming in on an area different than the ending position. Reordering the expressions seems to make us win these races. * Relatedly, make the multiple animations act more like a single animation by having the go_to pan linear. That way we have ease in, linear, and ease out. The old way (ease in, ease out, ease out) produced a weird pause before zooming in the map. The ChamplainView API doesn't allow us to do much better unfortunately.
Created attachment 318408 [details] [review] Make map animations slightly more tolerable
Review of attachment 318408 [details] [review]: Thanks! What do you feel are missing in the ChamplainView API? We are all in the same family, if we perceive limits we should file bugs and/or fix it! For the subject line, since this is cotnained to a module, could we make it: "mapWalker: Improve travel animations" ? ::: src/mapWalker.js @@ +120,3 @@ */ this._view.goto_animation_mode = Clutter.AnimationMode.EASE_IN_CUBIC; + this._ensureVisible(fromLocation); What difference does moving this call do? I do not see it. Since the code below will trigger once it is completed. Is it just for readability? @@ +125,3 @@ + this._view.goto_animation_mode = Clutter.AnimationMode.LINEAR; + this._view.go_to(this.place.location.latitude, + this.place.location.longitude); Same for this, is it just preference? The way the code is laid out? It makes it a bit hard to see what has actually changed in this patch. @@ +127,3 @@ + this.place.location.longitude); + + Utils.once(this._view, 'animation-completed', (function() { animation-completed will nowadays trigger both on zoom completed and go-to completed, so this will now catch both of them, this is intended?
Review of attachment 318408 [details] [review]: I would prefer ChamplainView calls that cause an animation would return a handle to a particular animation so there's no guessing about which animations will trigger which callbacks. ::: src/mapWalker.js @@ +120,3 @@ */ this._view.goto_animation_mode = Clutter.AnimationMode.EASE_IN_CUBIC; + this._ensureVisible(fromLocation); As I mentioned in the commit message setting the callback after starting the animation helps to ensure that the animation we intend to follow is the actual one. With the callbacks set before the animation starts, the callbacks sometimes run at inappropriate times. @@ +127,3 @@ + this.place.location.longitude); + + Utils.once(this._view, 'animation-completed', (function() { this callback only gets triggered once. I figured the go-to was redundant because the go_to animation has no zoom component to it.
Review of attachment 318408 [details] [review]: Ok, thanks Hashem! For me this feel nice and ok. I would like the fix to the shortlog mentioned in last review though. And I would like Mattias opinion, will add to cc.
I tried looking at this just now but ran into some other bugs where the goto animation was buggy in all versions of maps (latst in F23, master and with your patch), but my initial thought is that I don't particularly like doing linear transition between points. Could you upload some video comparing the differences in behaviour between master and with your patches applied? But yeah, the transitions we currently have aren't stellar. :/ Regards, mattias
without patch: http://webm.land/w/nVbY/ with patch: http://webm.land/w/vK4H/
Created attachment 318540 [details] [review] mapWalker: Improve travel animations A few things: * Reorder the animation callbacks. There's some sort of bizzarre race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming in instead of out to pan, and the map zooming in on an area different than the ending position. Reordering the expressions seems to make us win these races. * Relatedly, make the multiple animations act more like a single animation by having the go_to pan linear. That way we have ease in, linear, and ease out. The old way (ease in, ease out, ease out) produced a weird pause before zooming in the map. The ChamplainView API doesn't allow us to do much better unfortunately.
Hm, I looked at the movies real quick now. First: I don't like the change to LINEAR very much, it feels too jerky (especially when doing a long pan). So, regarding the pause, we could definitely experiment with the animation modes but it's probably an issue separate from this one we want _some_ easing in the pan. Exactly how to stitch it together I'm not sure. Ideally we'd want a true pan-zoom like what you see in the gif here: http://leafletjs.com/2015/07/15/leaflet-1.0-beta1-released.html (didn't have time to find the real demo) but that needs some (maybe a lot) of work done to Champlain. Hope that makes sense. :) Then: Regarding the reorganization of the callbacks I guess the original intent was to set up the handlers first and then run the animation, but if an animation is already underway it will not work fully I guess. I wonder if this will bite us ever, like if the animation is reeeealy short. Hm. If this helps and someone else can confirm it I'm for that change! :) Regards, Mattias
Yeah I agree that the jerky animations aren't nice. But for some reason the EASE_IN we have don't actually EASE_IN during the go-to phase (I think it wastes the EASE_IN in the zoom phase?), so when we had the EASE_OUT, it was imbalanced. Animations like in that leafletjs link would be ideal, but as you allude to, I don't think Champlain supports simultaneous animated pan and zoom. I don't think this change will be any worse than what was there before. Before almost always it did the wrong thing. Now it mostly does the right thing. Ideally Champlain would allow explicitly pairing an animation with a callback, but that's a lot more work.
(In reply to Hashem Nasarat from comment #9) > Yeah I agree that the jerky animations aren't nice. But for some reason the > EASE_IN we have don't actually EASE_IN during the go-to phase (I think it > wastes the EASE_IN in the zoom phase?), so when we had the EASE_OUT, it was > imbalanced. this._view.goto_animation_mode = Clutter.AnimationMode.EASE_IN_CUBIC; this._ensureVisible(fromLocation); [So ensureVisible ends up as view.ensure_visible, and as I rememver improving the champlain docs it now says for the animation-mode property: "* Please note that animation of #champlain_view_ensure_visible also "* involves a 'goto' animation. it is the go_to part of ensure_visible that gets the ease_in_cubic. that go_to travels until both points (from and to) are visible] Utils.once(this._view, 'animation-completed', (function() { [this will catch both animation-completed::zoom and animation-completed::go_to so it might be an uncertain point, maybe this needs a ::go_to?] this._view.goto_animation_mode = Clutter.AnimationMode.LINEAR; this._view.go_to(this.place.location.latitude, this.place.location.longitude); Utils.once(this._view, 'animation-completed', (function() { [Maybe the animation-completed::zoom from last trip is not done?] this._view.goto_animation_mode = Clutter.AnimationMode.EASE_OUT_CUBIC; this.zoomToFit(); this._view.goto_animation_mode = Clutter.AnimationMode.EASE_IN_OUT_CUBIC; this.emit('gone-to'); }).bind(this)); }).bind(this)); There might be some uncertainty left maybe we need to use specific signals instead of the composite animation-completed? > > Animations like in that leafletjs link would be ideal, but as you allude to, > I don't think Champlain supports simultaneous animated pan and zoom. > > I don't think this change will be any worse than what was there before. > Before almost always it did the wrong thing. Now it mostly does the right > thing. Ideally Champlain would allow explicitly pairing an animation with a > callback, but that's a lot more work.
> Animations like in that leafletjs link would be ideal, but as you allude to, > I don't think Champlain supports simultaneous animated pan and zoom. > > I don't think this change will be any worse than what was there before. > Before almost always it did the wrong thing. Now it mostly does the right > thing. Ideally Champlain would allow explicitly pairing an animation with a > callback, but that's a lot more work. No Champlain does not support panning and zooming, and it is burdensome to do really well here without vector tiles. And I am more partial to not trying something like this, really. Maybe go all LINEAR, or just have one animation-mode through out and try to get things smooth. Not letting perfect being the enemy of good...
(In reply to Jonas Danielsson from comment #11) > > Animations like in that leafletjs link would be ideal, but as you allude to, > > I don't think Champlain supports simultaneous animated pan and zoom. > > > > I don't think this change will be any worse than what was there before. > > Before almost always it did the wrong thing. Now it mostly does the right > > thing. Ideally Champlain would allow explicitly pairing an animation with a > > callback, but that's a lot more work. > > No Champlain does not support panning and zooming, and it is burdensome to > do really well here without vector tiles. > > And I am more partial to not trying something like this, really. I agree. > Maybe go all LINEAR, or just have one animation-mode through out and try to get > things smooth. > Not letting perfect being the enemy of good... In short, my opinion is that while the pause before zooming in looks weird it looks even more weird / unnatural with a linear pan. So in short, ACK on the first part if it helps make the animation more robust. :) But please don't make the panning linear. :)
Created attachment 318775 [details] [review] mapWalker: Improve travel animations A few things: * Reorder the animation callbacks. There's some sort of bizzarre race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming in instead of out to pan, and the map zooming in on an area different than the ending position. Reordering the expressions seems to make us win these races. * Relatedly, make the multiple animations act more like a single animation by having the go_to pan linear. That way we have ease in, linear, and ease out. The old way (ease in, ease out, ease out) produced a weird pause before zooming in the map. The ChamplainView API doesn't allow us to do much better unfortunately.
Created attachment 318776 [details] [review] mapWalker: Improve travel animations Reorder the animation callbacks. There's some sort of bizzarre race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming in instead of out to pan, and the map zooming in on an area different than the ending position. Reordering the expressions seems to make us win these races. The ChamplainView API doesn't allow us to do much better unfortunately.
Created attachment 318785 [details] [review] mapWalker: Improve travel animations Reorder the animation callbacks. There's some sort of race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming-in instead of out to pan, and the map zooming-in on an area different than the ending position. Reordering the expressions seems to make us win these races. The ChamplainView API doesn't allow us to do much better unfortunately.
Review of attachment 318785 [details] [review]: ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { should this be animation-completed::go-to? To avoid being catched by the zoom or go-to depending on distance? Since ensure_visible has both zoom and go-to?
Review of attachment 318785 [details] [review]: Thanks! Attaching a v3 ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { Yes, you're right. Libchamplain seems to do the right thing in either case, but it's good to be correct.
Created attachment 319395 [details] [review] mapWalker: Improve travel animations Reorder the animation callbacks. There's some sort of race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming-in instead of out to pan, and the map zooming-in on an area different than the ending position. Reordering the expressions seems to make us win these races. The ChamplainView API doesn't allow us to do much better unfortunately.
Created attachment 319402 [details] [review] mapWalker: Improve travel animations Reorder the animation callbacks. There's some sort of race condition where the callbacks trigger sometimes for different animations than the one you expect. This leads to things like the map zooming-in instead of out to pan, and the map zooming-in on an area different than the ending position. Reordering the expressions seems to make us win these races. The ChamplainView API doesn't allow us to do much better unfortunately so this is a quick bandaid until there's time for bigger changes to be made.
Review of attachment 318785 [details] [review]: Most recent patch works best. ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { Actually I did some more testing and changing this to ::go-to produced an occasional weird pause in the middle of the animation. http://webm.land/w/kBII/ So it should stay 'animation-completed'.
Review of attachment 319402 [details] [review]: Thanks, just one question, see below! ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { It is this one that I am most concerned about. Since ensureVisible does both zoom and go-to, this should maybe be animation-completed::go-to?
Review of attachment 319402 [details] [review]: ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { Well that is encouraging,your comment about the pause, then we do have a race here? Between zoom and go-to. I think maybe the pause is intentional? Since the algo is: "Travel until both points are visible, then go-to the next point" How does it look travelling, for instance between, Usa and europe Instead? And if the pause is really bad, should it be ::zoom instead? I dislike having it race.
Review of attachment 319402 [details] [review]: ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { It shouldn't be ::zoom because some ensure_visible calls don't ::zoom, but they always ::go-to. In the case where there is no ::zoom then the next time you zoom in (e.g. via mouse scroll) it triggers the code below.
Review of attachment 319402 [details] [review]: ::: src/mapWalker.js @@ +124,1 @@ Utils.once(this._view, 'animation-completed', (function() { Then we should def. have this as ::go-to and try to minimize or eliminate the pause. Because I think it is correct in theory. Mattias, any suggestions?
Relatedly, now that my mouse scroll is working I see that ChamplainView offers a private view_set_zoom_level_at() function which zooms in at a point that isn't the center. If we modified some code we could make a new animation which could be something like zoom out from center until point a and b are shown, then zoom in at point b. Instead of the current: zoom out at center until point b is visible, center point b, zoom in at center. But really the zoom animations with Champlain suck, and I just want animations like https://www.mapbox.com/blog/placing-labels/ Do we need vector tiles for that to happen?
(In reply to Hashem Nasarat from comment #25) > Relatedly, now that my mouse scroll is working I see that ChamplainView > offers a private view_set_zoom_level_at() function which zooms in at a point > that isn't the center. > > If we modified some code we could make a new animation which could be > something like zoom out from center until point a and b are shown, then zoom > in at point b. > Instead of the current: zoom out at center until point b is visible, center > point b, zoom in at center. > > But really the zoom animations with Champlain suck, and I just want > animations like https://www.mapbox.com/blog/placing-labels/ Do we need > vector tiles for that to happen? Sure, file a bug against Champlain to make that a public function maybe? It might be useful as you say. And yes, that is not possible really with bitmap tiles. Jonas
Older discussion about this in libchamplain bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=698505
Attachment 319402 [details] pushed as 2712c8e - mapWalker: Improve travel animations