GNOME Bugzilla – Bug 722865
Redundant zoom out at end of goto animation
Last modified: 2014-02-04 16:02:12 UTC
1. Search for "Montreal", select the first result (the city) 2. The map scrolls and zooms in. 3. Zoom in to the maximum. 4. Search for "Brossard", select the 1st result ("Brossard, Canada") Result: the map zooms out, scrolls sloooooowly towards brossard, and then it zooms out *again* (that 2nd zoom-out sounds like an unintended bug). Expected result: scroll should be much faster (the zoom-out was instantaneous!) and it shouldn't zoom out twice.
This should work much better in latest git (as of yesterday I think :)), though it's still not as smooth as we want it (we'd like to zoom and pan simultaneously). The zooming out at the end of the pan is indeed a bug though. Jonas, could you take a look at this when you get time?
As Mattias said, the animation duration is new in git, so please try again with latest git and see if you still feel it's to slow. The zooming out at the end is the expected behaviour of the code as it looks at the moment. The way Maps do a trip from one location to another is: * Zoom out and pan so that both locations are visible. (ensure_visible) * Go to the new location. * Zoom to the level appropriate for the accuracy of the location. (zoomToFit) The way zoomToFit works right now is a bit odd tho. It looks at the accuracy property of the location and tries to set a zoom-level based on that. But the search-results we get from geocode-forward will all have the accuracy property set to GEOCODE_LOCAITON_ACCURACY_UNKNOWN. So the code in zoomToFit: if (this.accuracy === Geocode.LOCATION_ACCURACY_UNKNOWN) zoom = 11; // Accuracy is usually city-level when unknown else if (this.accuracy <= Geocode.LOCATION_ACCURACY_STREET) zoom = 16; else if (this.accuracy <= Geocode.LOCATION_ACCURACY_CITY) zoom = 11; else if (this.accuracy <= Geocode.LOCATION_ACCURACY_REGION) zoom = 10; else if (this.accuracy <= Geocode.LOCATION_ACCURACY_COUNTRY) zoom = 6; else zoom = 3; Will, as I read the code, always set the zoom to 11 in cases like this. Suggestions for other behaviour would be welcome. To solve your particular case setting the zoom-level to the same as it was before would be nice. But that would probably not be the best all around case. Jonas
Maybe the solution would be to have geocode-glib look at the type of the place and set a ACCURACY based on that.
(In reply to comment #3) > Maybe the solution would be to have geocode-glib look at the type of the place > and set a ACCURACY based on that. Actually, accuracy is a bit wrong in terms of geocoding results. We should expose a bounding box for places. Nominatim as support for that.
(In reply to comment #0) > 1. Search for "Montreal", select the first result (the city) > 2. The map scrolls and zooms in. > 3. Zoom in to the maximum. > 4. Search for "Brossard", select the 1st result ("Brossard, Canada") > > Result: the map zooms out, scrolls sloooooowly towards brossard, I'll assume this part is fixed, unless you tell me otherwise. :) However, lets have 1 bug for each issue so please file another one if its still slow. > and then it > zooms out *again* (that 2nd zoom-out sounds like an unintended bug). Lets keep this bug for this issue.
(In reply to comment #5) > > > and then it > > zooms out *again* (that 2nd zoom-out sounds like an unintended bug). > > Lets keep this bug for this issue. Is it a bug tho? We have a zoom-level for cities that was honoured here (accuracy unknown equals zoom-level for city) In this case the map was zoomed in manually to the max zoom-level and then we went to the new location. Should we really keep the max zoom-level upon arrival? Or is the case that our zoom-level for cities is wrong?
(In reply to comment #4) > (In reply to comment #3) > > Maybe the solution would be to have geocode-glib look at the type of the place > > and set a ACCURACY based on that. > > Actually, accuracy is a bit wrong in terms of geocoding results. We should > expose a bounding box for places. Nominatim as support for that. Yeah, we can use the semi-new GeocodeBoundingBox and have a property on GeocodePlace. I can look into it.
(In reply to comment #6) > (In reply to comment #5) > > > > > > and then it > > > zooms out *again* (that 2nd zoom-out sounds like an unintended bug). > > > > Lets keep this bug for this issue. > > > Is it a bug tho? We have a zoom-level for cities that was honoured here > (accuracy unknown equals zoom-level for city) > > In this case the map was zoomed in manually to the max zoom-level and then we > went to the new location. Should we really keep the max zoom-level upon > arrival? > > Or is the case that our zoom-level for cities is wrong? I think the bug is about the fact that we should have different zoom levels for different kinds of results. So if the result is a street and you are already zoomed-in on a street, there shouldn't be any zooming-out at the end of goto animation. (In reply to comment #7) > Yeah, we can use the semi-new GeocodeBoundingBox and have a property on > GeocodePlace. I can look into it. I was hoping you'd say that. :)
(In reply to comment #7) > (In reply to comment #4) > > (In reply to comment #3) > > > Maybe the solution would be to have geocode-glib look at the type of the place > > > and set a ACCURACY based on that. > > > > Actually, accuracy is a bit wrong in terms of geocoding results. We should > > expose a bounding box for places. Nominatim as support for that. > > Yeah, we can use the semi-new GeocodeBoundingBox and have a property on > GeocodePlace. I can look into it. There is a bug now at: https://bugzilla.gnome.org/show_bug.cgi?id=723095 But I'm not sure how we use it best in Maps. I tested to use ensure_visible from Champlain on the bounding-box instead of zoomToFit for the example given in OP. The result was the same, Maps travels to Brossard and then zooms out to ensure the entire Brossard is visible. The result when going to countries was bad tho, zooming in on what I guess was the center of the country. So more thought and work needs to be done :) But the bounding-box property on GeocodePlace might be useful in either case.
(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > Maybe the solution would be to have geocode-glib look at the type of the place > > > > and set a ACCURACY based on that. > > > > > > Actually, accuracy is a bit wrong in terms of geocoding results. We should > > > expose a bounding box for places. Nominatim as support for that. > > > > Yeah, we can use the semi-new GeocodeBoundingBox and have a property on > > GeocodePlace. I can look into it. > > There is a bug now at: https://bugzilla.gnome.org/show_bug.cgi?id=723095 > > But I'm not sure how we use it best in Maps. I tested to use ensure_visible > from Champlain on the bounding-box instead of zoomToFit for the example given > in OP. > The result was the same, Maps travels to Brossard and then zooms out to ensure > the entire Brossard is visible. Did you ensure that ensure_visible call takes bounding box coordinates instead of location coordinates of the new place? If that doesn't help, perhaps we should only do two ensure_visible instead of goto? First one to make both existing and new places visible and then one to make only the new place visible and then do a zoomToFit?
(In reply to comment #10) > > If that doesn't help, perhaps we should only do two ensure_visible instead of > goto? First one to make both existing and new places visible and then one to > make only the new place visible and then do a zoomToFit? Without involving the bounding-box of the place at all? There are some problems with using them atm. UserLocation (which is a MapLocation) does not have a concept of bounding-box(?) from geoclue. And the recent/favorite places do not have bounding-boxed stored. The problem with the approach you describe is that ensure_visible does not center on the place. So we do not actually go there before we zoom.
(In reply to comment #11) > (In reply to comment #10) > > > > > If that doesn't help, perhaps we should only do two ensure_visible instead of > > goto? First one to make both existing and new places visible and then one to > > make only the new place visible and then do a zoomToFit? > > Without involving the bounding-box of the place at all? No, always involving those instead. :) > There are some problems > with using them atm. UserLocation (which is a MapLocation) does not have a > concept of bounding-box(?) from geoclue. It can be easily added as an option field. If its available, we use it; if its not, we simply don't. > And the recent/favorite places do not have bounding-boxed stored. Thats also fixable, isn't it? > The problem with the approach you describe is that ensure_visible does not > center on the place. So we do not actually go there before we zoom. Ah, that is true. Whatever we do to solve this issue, we really should ensure visibility of the whole place.
Perhaps the answer is to modify champlain_view_ensure_visible to adjust the zoom appropriately. It actually already adjusts the zoom, but only if its needed to ensure box is on the visible map, while the name of the function suggests that it should ensure visibility of the bounding box itself, which is different IMO. Or a seperate API could be added that does this.
Ok, So some patches on the way. * Add bounding-box to placeStore. * Make mapLocation take a place instead of a location. * Use the place.bounding_box and if there is one use that in theensure_visibility operation instead of the coords. Only the bounding_box of the destination though. Since we can't be sure that the champlainView currently is on a place. (maybe we could have some fix for that later?). * Use the place_type to do zoomToFit instead of accuracy Nothing of this will fix the issue of the OP. The case of zooming in to the max of Montreal and then going to Brossard. Both Montreal and Brossard will have zoom-level 11, but the manual zooming in throws us of on this short trip. The last patch does an extra zoomToFit to avoid that, if we want to.
Created attachment 267534 [details] [review] placeStore: store place bounding_box
Created attachment 267535 [details] [review] Use Place instead of Location in mapLocation
Created attachment 267536 [details] [review] mapLocation: use place bbox for ensure visible
Created attachment 267537 [details] [review] mapLocation: use placeType in zoomToFit
Created attachment 267538 [details] [review] mapLocation: add extra zoomToFit in goto This helps us avoid a redundant zoom out in cases where we are zoomed in tighter than the destination requires.
Review of attachment 267534 [details] [review]: ack
Review of attachment 267535 [details] [review]: How come this patch doesn't touch UserLocation? that class derives from MapLocation.
Review of attachment 267536 [details] [review]: Some nitpicks on commit log: * 'for ensure visible' -> 'for ensureVisible' * since shortlog isn't exactly very explanatory, better explanation in details section would be nice * also rationale for the change in description would be nice ::: src/mapLocation.js @@ +106,3 @@ + let newBox = this.bbox.copy(); + newBox.extend(fromLocation.latitude, fromLocation.longitude); + this._view.ensure_visible(newBox, true); I think we should have a seperate function for this: ensureAreaVisible or ensureBoxVisible @@ +109,3 @@ + } else { + this._mapView.ensureVisible([fromLocation, this]); + } and perhaps we should rename ensureVisible to ensureLocationsVisible and put all this code in another function which we call call ensureVisible.
Review of attachment 267537 [details] [review]: ::: src/mapLocation.js @@ +148,3 @@ + zoom = 11; + break; + } Thats not all the place types we have, do we?
Review of attachment 267538 [details] [review]: ::: src/mapLocation.js @@ +94,3 @@ + // Avoid redundant zoom out, for case where we are zoomed in + // more than the destination location requires. + this.zoomToFit(); would this imply view moving to the new location, then zooming out (because of ensure_visible)? If not, doesn't it make the ensure_visible call useless?
Review of attachment 267535 [details] [review]: It does not touch UserLocation because it doesn't have to, the init function of mapLocation does not add anything new that UserLocation needs to take in consideration.
Review of attachment 267536 [details] [review]: ::: src/mapLocation.js @@ +106,3 @@ + let newBox = this.bbox.copy(); + newBox.extend(fromLocation.latitude, fromLocation.longitude); + this._view.ensure_visible(newBox, true); Sure! @@ +109,3 @@ + } else { + this._mapView.ensureVisible([fromLocation, this]); + } Sounds ok!
Review of attachment 267537 [details] [review]: ::: src/mapLocation.js @@ +148,3 @@ + zoom = 11; + break; + } No, we have more, could be added. But I've been thinking. If we could add a function to libchamplain, something like: champlain_view_get_bounding_box_for_zoom_level () We could use that to find the first zoom-level where the bounding-box of the place is covered by the bounding-box of the view. And use that for zoomToFit. Thoughts?
Review of attachment 267538 [details] [review]: ::: src/mapLocation.js @@ +94,3 @@ + // Avoid redundant zoom out, for case where we are zoomed in + // more than the destination location requires. + this.zoomToFit(); No, not really this solves sort of a special case, which this bug has as an example. Montreal and Brossard is close to one another. And the zoom-out needed to ensure both visible is less then the zoom-out for CITY which Brossard is, or even less than the zoom-out for ensure-visible of Brossards bounding-box. And this happens because we are manually zoomed-in to the max at Montreal. So the zoom-out at the beginning, before the call to ensure_visible for the both places, makes sure we do not have to zoom-out after we arrive.
Review of attachment 267538 [details] [review]: Another solution could be just to set the zoom-level for CITY higher. :)
Review of attachment 267535 [details] [review]: ack
Review of attachment 267537 [details] [review]: ::: src/mapLocation.js @@ +148,3 @@ + zoom = 11; + break; + } Sounds good actually.
Review of attachment 267538 [details] [review]: ::: src/mapLocation.js @@ +94,3 @@ + // Avoid redundant zoom out, for case where we are zoomed in + // more than the destination location requires. + this.zoomToFit(); Ah ok, thanks for explaining. I think i like your idea about new champlain api that should make this workaround redundant? Although I think that already exists: champlain_view_get_bounding_box().
Review of attachment 267538 [details] [review]: Yes, champlain_view_get_bounding_box exists. But it goes by the current zoom_level. I'm cooking a patch atm that adds champlain_view_get_bounding_box_for_zoom_level that will use the same static method inside. We want to go through different zoom_levels to see if the fit is right, without actually zooming in. Otherwise we will zoom in to far/not enough while searching I imagine.
Review of attachment 267538 [details] [review]: ... Or rather, I can't find a mechanism to know when a zoom is complete, so it's hard to write the function that zooms and re-tries on fail. Would you rather like the adding of some sort of zoom-complete? Or hmm, maybe one could go by notify on zoom-level. I guess that would work. What do you think?
(In reply to comment #36) > Review of attachment 267538 [details] [review]: > > ... Or rather, I can't find a mechanism to know when a zoom is complete, so > it's hard to write the function that zooms and re-tries on fail. There is animation-completed signal. There should be a 'zoom' detail on it and if not, we could add that so you can hook to "animation-completed:zoom". > Would you rather like the adding of some sort of zoom-complete? > > Or hmm, maybe one could go by notify on zoom-level. I guess that would work. This would tell you that zoom changed but not that animation for it is complete or not.
Created attachment 267627 [details] [review] mapLocation: use place bbox for ensuring visible Use the bounding box of the GeocodePlace in the ensure visible operation. This will make sure that we reach a zoom-level that includes the whole of the new place we are travelling to.
Created attachment 267628 [details] [review] mapLocation: set zoom-level to cover place bbox Find and use the first zoom-level that covers the place bounding box.
Please try out the behavior with the latest patch and see if it works well.
(In reply to comment #37) > (In reply to comment #36) > > Review of attachment 267538 [details] [review] [details]: > > > > ... Or rather, I can't find a mechanism to know when a zoom is complete, so > > it's hard to write the function that zooms and re-tries on fail. > > There is animation-completed signal. There should be a 'zoom' detail on it and > if not, we could add that so you can hook to "animation-completed:zoom". > > > Would you rather like the adding of some sort of zoom-complete? > > > > Or hmm, maybe one could go by notify on zoom-level. I guess that would work. > > This would tell you that zoom changed but not that animation for it is complete > or not. I did not want to go this way, because then we would have to zoom in to a level, find out it doesn't cover any more and zoom out. Or zoom in to the max and then gradually zoom out to find the best fit.
(In reply to comment #41) > (In reply to comment #37) > > (In reply to comment #36) > > > Review of attachment 267538 [details] [review] [details] [details]: > > > > > > ... Or rather, I can't find a mechanism to know when a zoom is complete, so > > > it's hard to write the function that zooms and re-tries on fail. > > > > There is animation-completed signal. There should be a 'zoom' detail on it and > > if not, we could add that so you can hook to "animation-completed:zoom". > > > > > Would you rather like the adding of some sort of zoom-complete? > > > > > > Or hmm, maybe one could go by notify on zoom-level. I guess that would work. > > > > This would tell you that zoom changed but not that animation for it is complete > > or not. > > I did not want to go this way, because then we would have to zoom in to a > level, find out it doesn't cover any more and zoom out. Or zoom in to the max > and then gradually zoom out to find the best fit. Seems to fix the issue with the sample case here but seems now we don't zoom in even when we should. E.g after step#4 in comment#0, search for "South Norwood Hill, London" and select the result. Its a street so we should be zooming in but we don't zoom at all. Perhaps its because of the unhandled place type cases in zoomToFit?
(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #37) > > > (In reply to comment #36) > > > > Review of attachment 267538 [details] [review] [details] [details] [details]: > > > > > > > > ... Or rather, I can't find a mechanism to know when a zoom is complete, so > > > > it's hard to write the function that zooms and re-tries on fail. > > > > > > There is animation-completed signal. There should be a 'zoom' detail on it and > > > if not, we could add that so you can hook to "animation-completed:zoom". > > > > > > > Would you rather like the adding of some sort of zoom-complete? > > > > > > > > Or hmm, maybe one could go by notify on zoom-level. I guess that would work. > > > > > > This would tell you that zoom changed but not that animation for it is complete > > > or not. > > > > I did not want to go this way, because then we would have to zoom in to a > > level, find out it doesn't cover any more and zoom out. Or zoom in to the max > > and then gradually zoom out to find the best fit. > > Seems to fix the issue with the sample case here but seems now we don't zoom in > even when we should. E.g after step#4 in comment#0, search for "South Norwood > Hill, London" and select the result. Its a street so we should be zooming in > but we don't zoom at all. Perhaps its because of the unhandled place type cases > in zoomToFit? Can you specify "don't zoom at all", when I do it it zooms in. Do you mean it zoom in more? Also, if you select it from recent there is chance that there is no bounding-box. So to be sure select it from search result or remove the places file.
(In reply to comment #43) > (In reply to comment #42) > > (In reply to comment #41) > > > (In reply to comment #37) > > > > (In reply to comment #36) > > > > > Review of attachment 267538 [details] [review] [details] [details] [details] [details]: > > > > > > > > > > ... Or rather, I can't find a mechanism to know when a zoom is complete, so > > > > > it's hard to write the function that zooms and re-tries on fail. > > > > > > > > There is animation-completed signal. There should be a 'zoom' detail on it and > > > > if not, we could add that so you can hook to "animation-completed:zoom". > > > > > > > > > Would you rather like the adding of some sort of zoom-complete? > > > > > > > > > > Or hmm, maybe one could go by notify on zoom-level. I guess that would work. > > > > > > > > This would tell you that zoom changed but not that animation for it is complete > > > > or not. > > > > > > I did not want to go this way, because then we would have to zoom in to a > > > level, find out it doesn't cover any more and zoom out. Or zoom in to the max > > > and then gradually zoom out to find the best fit. > > > > Seems to fix the issue with the sample case here but seems now we don't zoom in > > even when we should. E.g after step#4 in comment#0, search for "South Norwood > > Hill, London" and select the result. Its a street so we should be zooming in > > but we don't zoom at all. Perhaps its because of the unhandled place type cases > > in zoomToFit? > > Can you specify "don't zoom at all", when I do it it zooms in. Do you mean it > zoom in more? Nope, i don't see any zoom-in happening. It just goes there. Same for "Montreal" now actually. > Also, if you select it from recent there is chance that there is no > bounding-box. So to be sure select it from search result or remove the places > file. I just tried after `rm ~/.cache/geocode-glib/*` to be sure and there is still no zoom-in.
Created attachment 267777 [details] [review] mapLocation: set zoom-level to cover place bbox Find and use the first zoom-level that covers the place bounding box.
Review of attachment 267777 [details] [review]: ack ::: src/mapLocation.js @@ +189,3 @@ }, + _boxCovers: function(coverBox) { I'd just name the arg 'box' but up to you. @@ +190,3 @@ + _boxCovers: function(coverBox) { + if (this.bbox === null) this shoulnd't be the case so I'd use g_return_val_if_fail here.
With the latest patches, the zoom issue is fixed and things look much better now, including fixing of the test case in comment#0.
Review of attachment 267627 [details] [review]: ack
Review of attachment 267534 [details] [review]: wrong status :)
Attachment 267534 [details] pushed as dc867c7 - placeStore: store place bounding_box Attachment 267535 [details] pushed as 838bcf9 - Use Place instead of Location in mapLocation Attachment 267537 [details] pushed as 6cfae29 - mapLocation: use placeType in zoomToFit Attachment 267627 [details] pushed as 1def9bc - mapLocation: use place bbox for ensuring visible Attachment 267777 [details] pushed as f6163e3 - mapLocation: set zoom-level to cover place bbox