GNOME Bugzilla – Bug 753395
Add property to limit the view to a bounding box
Last modified: 2015-08-30 23:34:31 UTC
Hi, So I would like the option to limit the view to a bounding box. So that no tiles are loaded outside of the bbox, and we cannot scroll outside of the bbox. This will be useful if one want to do a map source that only uses offline data from some bounding box. For instance.
Created attachment 308952 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
Created attachment 308959 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
Created attachment 309018 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
Thanks Jonas for the patch - the feature definitely makes sense. I've tried to put this into minimal-gtk.c: champlain_view_center_on(gtk_champlain_embed_get_view(GTK_CHAMPLAIN_EMBED(widget)), 0, 0); ChamplainBoundingBox *bbox = champlain_bounding_box_new(); bbox->left = -40; bbox->right = 40; bbox->bottom = -40; bbox->top = 40; champlain_view_set_world(gtk_champlain_embed_get_view(GTK_CHAMPLAIN_EMBED(widget)), bbox); but I get just an empty map. Am I doing something wrong? I have a few additional notes to the patch: 1. The bounding box setter should clamp the provided bounding box to valid ranges for latitude and longitude. 2. Centering to the middle of the bounding box should happen only if the current center lies outside the bounding box. The center should be kept where it is if it's inside the bounding box. 3. The above center latitude/longitude should be set even if the map isn't realized yet. 4. Maybe it would make sense to have also a getter for the property (and the initial value of the property should be set to max latitude/longitude values instead of NULL). 5. Just a formal thing - I've noticed there's "else {" somewhere in the patch - put { on the next line.
(In reply to Jiri Techet from comment #4) > Thanks Jonas for the patch - the feature definitely makes sense. I've tried > to put this into minimal-gtk.c: > Great! I was actually working on an updated version of this patch! > > champlain_view_center_on(gtk_champlain_embed_get_view(GTK_CHAMPLAIN_EMBED(wid > get)), 0, 0); > ChamplainBoundingBox *bbox = champlain_bounding_box_new(); > bbox->left = -40; > bbox->right = 40; > bbox->bottom = -40; > bbox->top = 40; > > > champlain_view_set_world(gtk_champlain_embed_get_view(GTK_CHAMPLAIN_EMBED(wid > get)), bbox); > > > but I get just an empty map. Am I doing something wrong? > Maybe, the center_on things are not working properly and the latitude/longitude is not inside the new "world" so no tiles are loaded. My new version might fix that. Since the center_on will only work if the view is realized. > I have a few additional notes to the patch: > > 1. The bounding box setter should clamp the provided bounding box to valid > ranges for latitude and longitude. Ok. > > 2. Centering to the middle of the bounding box should happen only if the > current center lies outside the bounding box. The center should be kept > where it is if it's inside the bounding box. Ok! > > 3. The above center latitude/longitude should be set even if the map isn't > realized yet. Ok! > > 4. Maybe it would make sense to have also a getter for the property (and the > initial value of the property should be set to max latitude/longitude values > instead of NULL). Agreed. There was a bug with the map_source_get_y that prevented me from doing this. Hint: We need to fix CHAMPLAIN_MAX|MIN_LATITUDE. Mercatur projection only goes to 85.0113 apparently. > > 5. Just a formal thing - I've noticed there's "else {" somewhere in the > patch - put { on the next line. Ok! Thanks!
Created attachment 309380 [details] [review] defines: Max latitude should be 85.05113 So it seems that the map_source_get_y breaks down when given values close to +-90. Some googling brought me to the conclusion that we should have a MIN/MAX latitude of 85.05113, since that is what the Mercator projection truncates latitude to. See https://en.wikipedia/wiki/Mercator_projection for more information,
Created attachment 309381 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
(In reply to Jonas Danielsson from comment #5) > (In reply to Jiri Techet from comment #4) > > Thanks Jonas for the patch - the feature definitely makes sense. I've tried > > to put this into minimal-gtk.c: > > > > Great! I was actually working on an updated version of this patch! > > > > > champlain_view_center_on(gtk_champlain_embed_get_view(GTK_CHAMPLAIN_EMBED(wid > > get)), 0, 0); > > ChamplainBoundingBox *bbox = champlain_bounding_box_new(); > > bbox->left = -40; > > bbox->right = 40; > > bbox->bottom = -40; > > bbox->top = 40; > > > > > > champlain_view_set_world(gtk_champlain_embed_get_view(GTK_CHAMPLAIN_EMBED(wid > > get)), bbox); > > > > > > but I get just an empty map. Am I doing something wrong? > > > > Maybe, the center_on things are not working properly and the > latitude/longitude is not inside the new "world" so no tiles are loaded. My > new version might fix that. Since the center_on will only work if the view > is realized. > So ok, I know what is going on. Since zoom level 0 only has 1 tile, and that tile covers more than your world bounding box, you will not get any tiles. So that is a decision to make, do you clamp it so you get at least _1_ tile or do you see that as a feature. You bounded the world, so you can't see that zoom level. Try scroll zooming in and you should see tiles.
Created attachment 309391 [details] [review] defines: Min/max latitude should be 85.05113 So it seems that the map_source_get_y breaks down when given values close to +-90. Some googling brought me to the conclusion that we should have a MIN/MAX latitude of 85.05113, since that is what the Mercator projection truncates latitude to. See https://en.wikipedia/wiki/Mercator_projection for more information,
Created attachment 309392 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
> So that is a decision to make, do you clamp it so you get at least _1_ tile or do you see that as a feature. I would say it should be shown. Or maybe in other words - if the bounding box border lies somewhere within a tile, the tile should be loaded (should apply at any zoom level). Will check the patch tomorrow.
I think there's a small problem in resize_viewport() - when you compute lower_/upper_ x/y, the second component of MIN/MAX is wrong IMO. The second component says what should be visible if map is smaller than viewport - e.g. at zoom level 0. In this case the map should get off the screen only by one half of the visible size (i.e. at zoom level 0 at least 128px of the tile are always visible). The problem is that in your case the variable "width" isn't really width but value which corresponds to max_x. First, I'd suggest to rename "width"/"height" variables to something else because it's misleading, let's say x_last/y_last (and maybe x/y to x_first/y_first). Second, in the second component of the MIN/MAX use (x_last-x_first) where "width" is and similar for the height too. One more minor formal thing - in the header move the getter to other getters and the setter to other setters. Otherwise the patch looks good.
Created attachment 309885 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
(In reply to Jiri Techet from comment #12) Thanks for review! > I think there's a small problem in resize_viewport() - when you compute > lower_/upper_ x/y, the second component of MIN/MAX is wrong IMO. The second > component says what should be visible if map is smaller than viewport - e.g. > at zoom level 0. In this case the map should get off the screen only by one > half of the visible size (i.e. at zoom level 0 at least 128px of the tile > are always visible). The problem is that in your case the variable "width" > isn't really width but value which corresponds to max_x. > > First, I'd suggest to rename "width"/"height" variables to something else > because it's misleading, let's say x_last/y_last (and maybe x/y to > x_first/y_first). Second, in the second component of the MIN/MAX use > (x_last-x_first) where "width" is and similar for the height too. > Thanks! Though I could not get this to really work with (x_last - y_last), I think instead we should use x_last and y_last, such as I did with width/height, but use clearer name. But Maybe I just messed up. With this both maps and minimal-gtk seemed to work, I did not get it to work with the x_last-y_last strategy. > One more minor formal thing - in the header move the getter to other getters > and the setter to other setters. > Done! > Otherwise the patch looks good.
Created attachment 309886 [details] [review] ChamplainView: Add world bounding box property This adds a property to limit the world to a Champlain BoundingBox.
So the latest version seems better, and should also address the tile bounds thing. You can check with your minimal-gtk example.
How does this look? Any chance of a release with this included before GNOME 3.18 you think? Would like to add some semi-hidden offline tile support to Maps and this would be nice to have then. Thanks!
Yeah, I'd say it can go in (hope I don't break any freeze rule as libchamplain is in external dependencies). I've just merged the patches but also made some minor fixes - please check if they are alright.