After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 577597 - The map should wrap horizontally
The map should wrap horizontally
Status: RESOLVED FIXED
Product: libchamplain
Classification: Core
Component: view
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: libchamplain-maint
libchamplain-maint
: 762867 765948 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-04-01 09:37 UTC by Pierre-Luc Beaudoin
Modified: 2016-08-14 12:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
view: Implement horizontal wrapping (7.32 KB, patch)
2014-04-05 11:25 UTC, Jonas Danielsson
none Details | Review
Implement horizontal wrapping (8.35 KB, patch)
2014-04-05 11:33 UTC, Jonas Danielsson
none Details | Review
Implement horizontal wrapping (8.35 KB, patch)
2014-04-06 08:51 UTC, Jonas Danielsson
none Details | Review
view: Make tile_map functions hash table agnostic (3.63 KB, patch)
2014-04-07 20:43 UTC, Jonas Danielsson
none Details | Review
Implement horizontal wrap (12.61 KB, patch)
2014-04-07 20:43 UTC, Jonas Danielsson
none Details | Review
The middle bubble is cut off (53.30 KB, image/png)
2014-04-07 22:21 UTC, Jiri Techet
  Details
Implement horizontal wrap (14.33 KB, patch)
2014-04-08 11:35 UTC, Jonas Danielsson
none Details | Review
Implement horizontal wrap (14.45 KB, patch)
2014-04-09 12:43 UTC, Jonas Danielsson
none Details | Review
zoom animation is hard (1.78 KB, patch)
2014-04-09 12:45 UTC, Jonas Danielsson
none Details | Review
Implement horizontal wrap (16.70 KB, patch)
2014-04-10 06:30 UTC, Jonas Danielsson
none Details | Review
view: Make tile_map functions hash table agnostic (3.63 KB, patch)
2014-04-10 20:29 UTC, Jonas Danielsson
none Details | Review
view: Add visible tiles hash table (5.55 KB, patch)
2014-04-10 20:29 UTC, Jonas Danielsson
none Details | Review
view: Implement and add horizontal-wrap property (18.69 KB, patch)
2014-04-10 20:29 UTC, Jonas Danielsson
none Details | Review
view: Implement and add horizontal-wrap property (18.25 KB, patch)
2014-04-11 11:24 UTC, Jonas Danielsson
none Details | Review
launcher-gtk: Add horizontal wrap toggle button (1.46 KB, patch)
2014-04-11 11:25 UTC, Jonas Danielsson
none Details | Review
Use map_width and not map_size in update_clones (1.47 KB, patch)
2014-04-23 21:20 UTC, Jonas Danielsson
none Details | Review
Position user layers on the center of the viewport (2.62 KB, patch)
2014-04-23 21:20 UTC, Jonas Danielsson
none Details | Review
Relocation bug (1.64 KB, text/plain)
2016-07-14 15:04 UTC, Marius Stanciu
  Details
path wrapping (27.62 KB, image/png)
2016-07-25 17:48 UTC, Marius Stanciu
  Details

Description Pierre-Luc Beaudoin 2009-04-01 09:37:30 UTC
When panning from West to East, the map should wrap so that it doesn't break.  The Earth is round so it doesn't make sens not to wrap.
Comment 1 Jonas Danielsson 2014-04-02 11:49:25 UTC
Jiri: what is your thoughts on this? What do you think it would take to implement this?
Comment 2 Jiri Techet 2014-04-04 10:21:16 UTC
Hello Jonas - eager to implement this feature? Great ;-). 

I just had a look at the code and maybe there's a relatively easy way:

Have a look at line 145 in ChamplainView - this shows the hierarchy of Champlain ClutterActors. There's something like this now.

/* viewport_container */  
  /* background_layer */
  /* zoom_layer */
  /* map_layer */
  /* user_layers */

In the case of horizontal wrapping, it might be worth introducing something like this:

/* viewport_container */
  /* master container */
    /* background_layer */
    /* zoom_layer */
    /* map_layer */
    /* user_layers */
  /* clone of master 1 */
  /* clone of master 2 */
  /* etc */

The cloned actors would be placed on the left/right of the master and display the same, including all layers with markers and so on. The big advantage of this approach is that you don't have to implement any changes in the map sources or layers - there remains only one "true" map view (which is used for loading and all the rest). Otherwise there would be problems with e.g. loading a single tile multiple times from the maps source, having to display all the layers multiple times and so on. This was my original thought how to implement it and found it quite painful which is why I didn't do anything about this feature for such a long time.

Of course, the code positioning the clones is missing - the clone creation, destruction and reposition will have to be added (probably to viewport_pos_changed_cb()).

Yeah, and if you try to experiment with it, just a note that at the end of resize_viewport() you have to first remove the min/max values so the viewport can move along the x axis freely without any borders.

If the above idea works, it might not be that bad to implement time-wise. Want to give it a try? I'm here to help if you run into some problems.
Comment 3 Jonas Danielsson 2014-04-05 11:25:10 UTC
Thanks Jiri!

I've done something similar to those lines. And will post a patch of the concept for review. Do you think this should be behind a property? PROP_HORIZONTAL_WRAP that is default FALSE?
Comment 4 Jonas Danielsson 2014-04-05 11:25:40 UTC
Created attachment 273614 [details] [review]
view: Implement horizontal wrapping
Comment 5 Jonas Danielsson 2014-04-05 11:27:21 UTC
Review of attachment 273614 [details] [review]:

::: champlain/champlain-view.c
@@ +2051,3 @@
+   * FIXME: re-factor this function to handle fragmentation in where tiles are
+   * to be loaded.
+   */

load_visible_tiles can't really handle the visible tiles being "non-linear", so for now I just load all tiles when we are wrapping. This causes some loading time for the wrap.

I want to rewrite it to handle things better so that we only load the tiles needed. But I haven't for now :) Any tips?
Comment 6 Jonas Danielsson 2014-04-05 11:33:12 UTC
Created attachment 273615 [details] [review]
Implement horizontal wrapping
Comment 7 Jonas Danielsson 2014-04-05 11:34:15 UTC
Also needed to change the get_longitude of map_source to manage negative and > map_width values of x.

I might be missing more corner cases like this, review and test is welcome.
Comment 8 Jonas Danielsson 2014-04-05 12:31:48 UTC
Additional note: I've only tested it with the GNOME Maps application.
Comment 9 Jonas Danielsson 2014-04-06 08:51:04 UTC
Created attachment 273647 [details] [review]
Implement horizontal wrapping
Comment 10 Mattias Bengtsson 2014-04-06 11:35:10 UTC
I tried this out briefly. Some observations:
1) There seems to be some bugs when panning where the area left or right to the datum line won't be drawn. If you pan around a bit it will start drawing again.
2) While zooming in the area left or right of the datum line won't draw until the zoom transition is finished.
3) Zooming in and out on the datum line sometimes make Maps hang (no debug warnings).

Here's a movie showing the above issues: https://www.youtube.com/watch?v=eBlbU3GTSZI

It seems to work pretty good in all other situations though. Great work Jonas!
Comment 11 Mattias Bengtsson 2014-04-06 11:48:19 UTC
Review of attachment 273647 [details] [review]:

I'm not familiar with champlain yet so this review might just be a bunch of stupid questions. Bear with me. :)

::: champlain/champlain-map-source.c
@@ +562,3 @@
+  /* in case of wrap */
+  if (x < 0)
+  cols = champlain_map_source_get_column_count (map_source, zoom_level);

Could this line ever result in an x that is still less than map_width? Ie. are there any corner cases where you'd actually want to do something like:
  
  while(x < 0) x += map_width;

?

@@ +564,3 @@
+      x += map_width;
+  else if (x > map_width)
+  map_width = tile_size * cols;

Same here in that case.
Comment 12 Jonas Danielsson 2014-04-06 14:46:20 UTC
(In reply to comment #10)
> I tried this out briefly. Some observations:
> 1) There seems to be some bugs when panning where the area left or right to the
> datum line won't be drawn. If you pan around a bit it will start drawing again.
> 2) While zooming in the area left or right of the datum line won't draw until
> the zoom transition is finished.
> 3) Zooming in and out on the datum line sometimes make Maps hang (no debug
> warnings).
> 
> Here's a movie showing the above issues:
> https://www.youtube.com/watch?v=eBlbU3GTSZI
> 
> It seems to work pretty good in all other situations though. Great work Jonas!

Thanks for testing it. I will keep working on it and address your issues. Some of this might be related to the load_visible_tiles function that I haven't made wrap aware. I just make it load all tiles when it suspects wrapping. Which makes it slow. I am working on a better version of load_visible_tiles.

The larger point is if we want to implement like this. There are two clones placed two the left and two the right to make it appear that there is wrapping going on. This isn't noticeable in Maps, at least for me. Since we limit how much a user is able to zoom out. If you try this in the Champlain launcher-gtk demo and zoom out you will see the three maps aligned in a row. It might also be visible on HIDPI systems in Maps, I guess? If you are able too see all three Map containers at the same time.

So we might want to go a whole other way to implement this. Maybe. Jiri?
Comment 13 Jonas Danielsson 2014-04-06 14:47:30 UTC
Review of attachment 273647 [details] [review]:

::: champlain/champlain-map-source.c
@@ +562,3 @@
+  /* in case of wrap */
+  if (x < 0)
+      x += map_width;

No it is not possible. I don't think at least. The lower and upp bounds on the hadjustment should make sure that there is no way to get those kind of values.
Comment 14 Mattias Bengtsson 2014-04-06 15:32:10 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > I tried this out briefly. Some observations:
> > 1) There seems to be some bugs when panning where the area left or right to the
> > datum line won't be drawn. If you pan around a bit it will start drawing again.
> > 2) While zooming in the area left or right of the datum line won't draw until
> > the zoom transition is finished.
> > 3) Zooming in and out on the datum line sometimes make Maps hang (no debug
> > warnings).
> > 
> > Here's a movie showing the above issues:
> > https://www.youtube.com/watch?v=eBlbU3GTSZI
> > 
> > It seems to work pretty good in all other situations though. Great work Jonas!
> 
> Thanks for testing it. I will keep working on it and address your issues. Some
> of this might be related to the load_visible_tiles function that I haven't made
> wrap aware. I just make it load all tiles when it suspects wrapping. Which
> makes it slow. I am working on a better version of load_visible_tiles.

Ok, cool.

> The larger point is if we want to implement like this. There are two clones
> placed two the left and two the right to make it appear that there is wrapping
> going on. This isn't noticeable in Maps, at least for me. Since we limit how
> much a user is able to zoom out. If you try this in the Champlain launcher-gtk
> demo and zoom out you will see the three maps aligned in a row. It might also
> be visible on HIDPI systems in Maps, I guess? If you are able too see all three
> Map containers at the same time.

Hm ok. Yeah it /sounds/ like a hacky solution but if it works fine maybe it also is fine.
About Hi-Dpi since 3.12 clutter stuff is automatically scaled to 2x if you have a hidpi screen so the result should be (and for me is) the same. And yeah stuff get's really weird when you zoom all the way out in launcher-gtk.
Comment 15 Mattias Bengtsson 2014-04-06 15:32:43 UTC
(In reply to comment #13)
> Review of attachment 273647 [details] [review]:
> 
> ::: champlain/champlain-map-source.c
> @@ +562,3 @@
> +  /* in case of wrap */
> +  if (x < 0)
> +      x += map_width;
> 
> No it is not possible. I don't think at least. The lower and upp bounds on the
> hadjustment should make sure that there is no way to get those kind of values.

Ok cool!
Comment 16 Jonas Danielsson 2014-04-07 20:43:13 UTC
Created attachment 273744 [details] [review]
view: Make tile_map functions hash table agnostic

With this change we can operate on other hash tables of tiles as well.
Comment 17 Jonas Danielsson 2014-04-07 20:43:21 UTC
Created attachment 273745 [details] [review]
Implement horizontal wrap
Comment 18 Jonas Danielsson 2014-04-07 20:44:46 UTC
This do not work well at all :) Jiri: what do you think of the approach? Right now I can't determine how to deal with the adjustment to make panning work a long side the wrapping.
Comment 19 Jiri Techet 2014-04-07 22:20:10 UTC
Hi Jonas, cool! (Sorry for the delay, I've been too busy during the weekend.) There are some rough corners but I think this is the right approach.

1. About the left-right clones - I think there shouldn't be just 2 of them but rather an array of something like (view_size/map_size) clones. The number should be adjusted when (a) zoom level changes and (b) ChamplainView size changes.

2. When zooming in/out, the clones should be placed to the zoom_actor to animate the whole map, not just the master.

3. There's a problem with the layers that on the border of two wraps the contents of the layer can be cut-off (see the middle bubble in the attached screenshot). Maybe a clone of map_layer could be taken independently of user_layers and these clones placed on top of map_layer clones (background_layer and zoom_layer should be global, not per clone).

Did I miss any other problems (load_visible_tiles() should be improved of course)?
Comment 20 Jiri Techet 2014-04-07 22:21:00 UTC
Created attachment 273751 [details]
The middle bubble is cut off
Comment 21 Jonas Danielsson 2014-04-08 11:35:14 UTC
Created attachment 273786 [details] [review]
Implement horizontal wrap
Comment 22 Jonas Danielsson 2014-04-08 11:37:53 UTC
(In reply to comment #19)
> Hi Jonas, cool! (Sorry for the delay, I've been too busy during the weekend.)
> There are some rough corners but I think this is the right approach.


Ok, let's see if we can fix it up then :)

> 
> 1. About the left-right clones - I think there shouldn't be just 2 of them but
> rather an array of something like (view_size/map_size) clones. The number
> should be adjusted when (a) zoom level changes and (b) ChamplainView size
> changes.
> 

I've added this to the new version.

> 2. When zooming in/out, the clones should be placed to the zoom_actor to
> animate the whole map, not just the master.
> 

I've stared a bit on the zoom animation code but I haven't really worked out how to do this yet. Any more pointers?

> 3. There's a problem with the layers that on the border of two wraps the
> contents of the layer can be cut-off (see the middle bubble in the attached
> screenshot). Maybe a clone of map_layer could be taken independently of
> user_layers and these clones placed on top of map_layer clones
> (background_layer and zoom_layer should be global, not per clone).
> 

Yeah I see. I don't really understand what you mean with the clones tho.
Could you elaborate?

> Did I miss any other problems (load_visible_tiles() should be improved of
> course)?

Not sure, I've fixed an issue with the adjustment since last patch. Are you fine with the general approach of re-positioning the viewport on "wrap"?
Comment 23 Jiri Techet 2014-04-08 20:27:03 UTC
(In reply to comment #22)
> (In reply to comment #19)
> > Hi Jonas, cool! (Sorry for the delay, I've been too busy during the weekend.)
> > There are some rough corners but I think this is the right approach.
> 
> 
> Ok, let's see if we can fix it up then :)
> 
> > 
> > 1. About the left-right clones - I think there shouldn't be just 2 of them but
> > rather an array of something like (view_size/map_size) clones. The number
> > should be adjusted when (a) zoom level changes and (b) ChamplainView size
> > changes.
> > 
> 
> I've added this to the new version.

Nice!

> 
> > 2. When zooming in/out, the clones should be placed to the zoom_actor to
> > animate the whole map, not just the master.
> > 
> 
> I've stared a bit on the zoom animation code but I haven't really worked out
> how to do this yet. Any more pointers?

Alright, zoom animation - I did almost throw my PC out of the window when implementing it - it's pretty tricky to get it right. Before the animation starts, the visible tiles are placed into zoom_actor, which is an auxiliary container, which is then placed into zoom_overlay_actor. zoom_overlay_actor is on top of everything so from now on, ordinary tiles aren't visible and you see just the contents of zoom_overlay_actor. Now the auxiliary zoom_actor starts scaling into the desired size according to the zoom level. When finished, (the scaled) zoom_actor is removed from zoom_overlay_actor and placed into zoom_layer. Since zoom_layer is behind map layers (and the clones), there is the zoomed-in low resolution view which gets filled in by tiles for the new zoom level (aligning the zoomed in view and the newly loaded tiles so that the scaled image was at the same position as the high resolution tiles was a complete hell). Finally, zoom_layer gets covered by the new tiles completely.

From the above it's clear that zoom_layer is global for the whole view and not per clone so it should be moved out of the clones (so is background_layer, which can be used to display some pattern, e.g. chessboard at places where tiles aren't loaded). Instead of just tiles, all the clones should be moved into the zoom_actor. Right now I can't think of anything else that should be done so the zoom animation works but I'm sure you'll run into many problems on the way :-/.

> 
> > 3. There's a problem with the layers that on the border of two wraps the
> > contents of the layer can be cut-off (see the middle bubble in the attached
> > screenshot). Maybe a clone of map_layer could be taken independently of
> > user_layers and these clones placed on top of map_layer clones
> > (background_layer and zoom_layer should be global, not per clone).
> > 
> 
> Yeah I see. I don't really understand what you mean with the clones tho.
> Could you elaborate?

What I had in my mind was to change the actor hierarchy to something like this:

/* ChamplainView */
   /* kinetic_scroll */
       /* viewport */
           /* viewport_container */
               /* background_layer */
               /* zoom_layer */
               /* master_map_layer */
               /* clones of master_map_layer, left */
               /* clones of master_map_layer, right */
               /* master_user_layers */
               /* clones of master_user_layers, left */
               /* clones of master_user_layers, right */
   /* zoom_overlay_actor */
   /* license_actor */ 

background_layer and zoom_layer shouldn't be cloned (see above). If you clone map_layer independently of user_layers and place them in this order, user_layers and their clones will be above map_layer and its clones. 

> 
> > Did I miss any other problems (load_visible_tiles() should be improved of
> > course)?
> 
> Not sure, I've fixed an issue with the adjustment since last patch. Are you
> fine with the general approach of re-positioning the viewport on "wrap"?

It's probably the easiest thing to do. It's a bit slow but that shouldn't matter much - it doesn't make much sense to scroll the zoomed-out map and see the same map again and again anyway...
Comment 24 Jonas Danielsson 2014-04-09 12:43:07 UTC
Created attachment 273886 [details] [review]
Implement horizontal wrap
Comment 25 Jonas Danielsson 2014-04-09 12:45:31 UTC
Created attachment 273887 [details] [review]
zoom animation is hard
Comment 26 Jonas Danielsson 2014-04-09 12:46:32 UTC
So the first new patch here should fix the user layers being on top.

The second is a naive attempt to add the clones to the zoom_actor :)
I need to stare at it some more to get it right.
Comment 28 Jonas Danielsson 2014-04-10 06:32:25 UTC
Hi again, please try this latest patch and see how the zoom animation feels.

If we like the general approach we could see if and how the patch should be split up into smaller pieces and maybe if this should be under a property.
Comment 29 Jonas Danielsson 2014-04-10 06:35:33 UTC
Review of attachment 273954 [details] [review]:

::: champlain/champlain-view.c
@@ +155,3 @@
   ClutterActor *user_layers;                    /* user_layers */
+  GList *user_clones_left;                      /* user_layers clones left */
+  GList *user_clones_right;                     /* user_layers clones right */

These GLists should not be here.
Comment 30 Jonas Danielsson 2014-04-10 20:29:04 UTC
Created attachment 274036 [details] [review]
view: Make tile_map functions hash table agnostic

With this change we can operate on other hash tables of
tiles as well.
Comment 31 Jonas Danielsson 2014-04-10 20:29:13 UTC
Created attachment 274037 [details] [review]
view: Add visible tiles hash table

Keep the currently visible tiles in a hash table for easy look up.
This will enable us to keep track of tiles that aren't necessarily
align continously.
Comment 32 Jonas Danielsson 2014-04-10 20:29:24 UTC
Created attachment 274038 [details] [review]
view: Implement and add horizontal-wrap property

To give the impression of horizontal wrap clones of the map- and
user layers will be placed next to the real layers. When we cross over
to them we will re-position the viewport to give the illusion of wrapping
the map. The same trick is needed for the zoom actor.
Comment 33 Jonas Danielsson 2014-04-11 11:24:59 UTC
Created attachment 274076 [details] [review]
view: Implement and add horizontal-wrap property

To give the impression of horizontal wrap clones of the map- and
user layers will be placed next to the real layers. When we cross over
to them we will re-position the viewport to give the illusion of wrapping
the map. The same trick is needed for the zoom actor.
Comment 34 Jonas Danielsson 2014-04-11 11:25:05 UTC
Created attachment 274077 [details] [review]
launcher-gtk: Add horizontal wrap toggle button
Comment 35 Jonas Danielsson 2014-04-11 11:25:41 UTC
Review of attachment 274077 [details] [review]:

I tried to add this to the "launcher" demo as well but it did not work, any ideas why, Jiri?
Comment 36 Jiri Techet 2014-04-13 16:04:45 UTC
Hi Jonas, finally had some time to have a look at the patches in more detail. I've made some changes and pushed the current code into the branch "wrap" of libchamplain. Most notably, the code didn't work as you probably expected with the left-right clones - left cones were never displayed because the master is always moved to the very left. This is however OK in my opinion and simplifies things a bit so I just adjusted the code to display the right number of right clones and removed left clones completely. Please check the rest of the changes I made if you agree with them and if I didn't introduce some more problems.

The problems I'm aware of:

1. When playing with the maps, I realized that the clones of user_layers are completely static, i.e. the bubbles cannot be clicked or moved. This would be really confusing to the user because he wouldn't know which one of the multiple copies is clickable. For this reason I have decided it would be best to display just the master of user_layers without any clones. Right now the master is displayed on top of the master map_layer on the very left but should be moved on top of the clone which is in the middle of the map. I haven't had time to finish this but if you have some time to spend, you can have a look at this (I won't have much time during the next week I'm afraid).
2. Zoom in/out is a bit broken - especially when using mouse wheel on top of some clone distant from the master map view. Also when zooming out, sometimes the clones disappear for some reason (maybe they are covered by some other actors, I don't know). The zoom animation also doesn't work perfectly well when zooming with mouse on top of some clone. All the zoom stuff is really tricky to make right, I've spent something like three hours looking at it without any working output - needs more time.
3. Launcher demo - I think there's some problem with the clones - this one doesn't use ClutterGtk but Clutter directly - maybe we aren't initializing Clutter correctly or something similar, will have to check.

If you make any changes, it's probably best to make them on top of the wrap branch.
Comment 37 Jonas Danielsson 2014-04-13 19:40:48 UTC
(In reply to comment #36)
> Hi Jonas, finally had some time to have a look at the patches in more detail.
> I've made some changes and pushed the current code into the branch "wrap" of
> libchamplain. Most notably, the code didn't work as you probably expected with
> the left-right clones - left cones were never displayed because the master is
> always moved to the very left. This is however OK in my opinion and simplifies
> things a bit so I just adjusted the code to display the right number of right
> clones and removed left clones completely. Please check the rest of the changes
> I made if you agree with them and if I didn't introduce some more problems.
> 

Thanks for review and fixing the code up!

> The problems I'm aware of:
> 
> 1. When playing with the maps, I realized that the clones of user_layers are
> completely static, i.e. the bubbles cannot be clicked or moved. This would be
> really confusing to the user because he wouldn't know which one of the multiple
> copies is clickable. For this reason I have decided it would be best to display
> just the master of user_layers without any clones. Right now the master is
> displayed on top of the master map_layer on the very left but should be moved
> on top of the clone which is in the middle of the map. I haven't had time to
> finish this but if you have some time to spend, you can have a look at this (I
> won't have much time during the next week I'm afraid).

Sure I'll look at it. My time is fragmented because I am currently on parental leave, taking care of my kid. So the hacking time I get is concentrated to when he sleeps :) That's why my contributions here are so fragmented, sorry bout that.

> 2. Zoom in/out is a bit broken - especially when using mouse wheel on top of
> some clone distant from the master map view. Also when zooming out, sometimes
> the clones disappear for some reason (maybe they are covered by some other
> actors, I don't know). The zoom animation also doesn't work perfectly well when
> zooming with mouse on top of some clone. All the zoom stuff is really tricky to
> make right, I've spent something like three hours looking at it without any
> working output - needs more time.

Yeah, I think we need more eyes and testers to get wrapping in without any regressions. I'll see if I can scare up any Maps devs to help.


> 3. Launcher demo - I think there's some problem with the clones - this one
> doesn't use ClutterGtk but Clutter directly - maybe we aren't initializing
> Clutter correctly or something similar, will have to check.
> 
> If you make any changes, it's probably best to make them on top of the wrap
> branch.

Will do, thanks again!
Comment 38 Jonas Danielsson 2014-04-21 20:14:05 UTC
(In reply to comment #37)

> > 
> > 1. When playing with the maps, I realized that the clones of user_layers are
> > completely static, i.e. the bubbles cannot be clicked or moved. This would be
> > really confusing to the user because he wouldn't know which one of the multiple
> > copies is clickable. For this reason I have decided it would be best to display
> > just the master of user_layers without any clones. Right now the master is
> > displayed on top of the master map_layer on the very left but should be moved
> > on top of the clone which is in the middle of the map. I haven't had time to
> > finish this but if you have some time to spend, you can have a look at this (I
> > won't have much time during the next week I'm afraid).
> 
> Sure I'll look at it. My time is fragmented because I am currently on parental
> leave, taking care of my kid. So the hacking time I get is concentrated to when
> he sleeps :) That's why my contributions here are so fragmented, sorry bout
> that.


This is tricky. It's possible to position the user-layer somewhat correct. But the result is not all that great I think. It's hard to determine where something should be visible when multiple "clones" are visible. And also the way we do the x_to_wrap_x makes the dragging spazz out, since the marker layer asks for x_to_longitude and longitude_to_x from the ChamplainView the coords will be "wrong" for the user-layer and we will not be able to drag stuff across clone lines.

I wonder if we can make this work with this approach or if we need to go another route. It's to bad it will not work to just clone the user-layer.
Comment 39 Jiri Techet 2014-04-22 19:54:17 UTC
(In reply to comment #38)
> (In reply to comment #37)
> 
> > > 
> > > 1. When playing with the maps, I realized that the clones of user_layers are
> > > completely static, i.e. the bubbles cannot be clicked or moved. This would be
> > > really confusing to the user because he wouldn't know which one of the multiple
> > > copies is clickable. For this reason I have decided it would be best to display
> > > just the master of user_layers without any clones. Right now the master is
> > > displayed on top of the master map_layer on the very left but should be moved
> > > on top of the clone which is in the middle of the map. I haven't had time to
> > > finish this but if you have some time to spend, you can have a look at this (I
> > > won't have much time during the next week I'm afraid).
> > 
> > Sure I'll look at it. My time is fragmented because I am currently on parental
> > leave, taking care of my kid. So the hacking time I get is concentrated to when
> > he sleeps :) That's why my contributions here are so fragmented, sorry bout
> > that.
> 
> 
> This is tricky. 

Yeah, I know, that's why I left it to you ;-). I think it would be possible somewhat but of corse when you are on the border, you get just one copy of the layer. But I think the old google maps did the same as well (the new ones show markers in all the wraps).

> It's possible to position the user-layer somewhat correct. But
> the result is not all that great I think. It's hard to determine where
> something should be visible when multiple "clones" are visible. And also the
> way we do the x_to_wrap_x makes the dragging spazz out, since the marker layer
> asks for x_to_longitude and longitude_to_x from the ChamplainView the coords
> will be "wrong" for the user-layer and we will not be able to drag stuff across
> clone lines.

True, but maybe it's not a big problem in practice.

> 
> I wonder if we can make this work with this approach or if we need to go
> another route. It's to bad it will not work to just clone the user-layer.

Ideas? The only one I have is to make all the layers wrap-aware and let them draw all the copies but I'm afraid this is too much work.
Comment 40 Jonas Danielsson 2014-04-23 21:20:33 UTC
Created attachment 275001 [details] [review]
Use map_width and not map_size in update_clones

https://bugzilla.gnome.org/show_bug.cgi?id=577596
Comment 41 Jonas Danielsson 2014-04-23 21:20:38 UTC
Created attachment 275002 [details] [review]
Position user layers on the center of the viewport

https://bugzilla.gnome.org/show_bug.cgi?id=577596
Comment 42 Jonas Danielsson 2014-04-23 21:22:32 UTC
Sorry about the wrong bug id, miss fired git bz :)

These two are meant for the wrap branch. If you have time please test and see if the behavior of the user layers seem fine.

I will look into the other issues when I have time to spare.
Comment 43 Jiri Techet 2014-04-29 14:14:20 UTC
Thanks. 

There's one problem that when moving the markers, they aren't wrapped correctly (neither is the path when drawn). Try to run the launcher-gtk to set zoom level to 1, move the markers a bit so the path is visible and start panning the viewport - the path will start disappearing in the middle of the viewport as you move (and wrap is performed too often when you move the markers). 

I guess the reason is champlain_view_longitude_to_x() which the layers call to get x based on longitude - I've tried to add the same offset as used in position_user_layers() but this doesn't work - apparently some more adjustments are needed but I didn't have time to explore more.
Comment 44 Jiri Techet 2016-02-29 15:29:44 UTC
*** Bug 762867 has been marked as a duplicate of this bug. ***
Comment 45 Jonas Danielsson 2016-02-29 20:01:04 UTC
Btw Jiri,

Would you like to co-mentor this if we turned it into an Outreachy-project or GSoC?
Comment 46 Jiri Techet 2016-02-29 20:49:41 UTC
Yeah, sure - it's a bit embarrassing this isn't in libchamplain yet.

Actually the patches you made and I improved a bit were "almost" there - it's just that the "almost" -> completely transition may still need quite some work and involve some annoying games with coordinates...
Comment 47 Jonas Danielsson 2016-03-07 07:26:48 UTC
This has now been added as an idea to GSoC and Outrechy:

https://wiki.gnome.org/Outreach/SummerOfCode/2016/Ideas
https://wiki.gnome.org/Outreach/Outreachy/2016/MayAugust

Title:
Maps and libchamplain: Make the map wrap horizontally in the libchamplain map widget

And with a note:
Note: There might be room and time to find other areas of improvement in the libchamplain Map widget.
Comment 48 Jiri Techet 2016-03-09 11:53:11 UTC
Thanks Jonas. And yes there's always room for other improvements :-).
Comment 49 Zeeshan Ali 2016-05-04 18:03:27 UTC
*** Bug 765948 has been marked as a duplicate of this bug. ***
Comment 50 Marius Stanciu 2016-06-21 16:09:46 UTC
Hi!

Done with exams and everything, I'm now fully dedicated and ready to catch up!
I've started pushing some of my work to my github repository at:
https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap

I've fixed the zooming animation stuttering and the marker dragging behaving weirdly. Now i'm working on making the user layer seamless over the clones.
I've spent a lot of time trying different approaches, and I've come up with 2 viable ones:


1) Jiri's suggestion to make the layers wrap-aware and let them draw their copies.
I've almost implemented this for the marker layer in the github wrap branch, you can test it. It works fine, but there is a noticeable disadvantage: implementing a custom layer now requires the developer to also code the clone stuff.


2) Cloning events at champlain_view level:
We could use the static user layer clones we tried before. We then catch all mouse events that occur on the clones and synthesize them over the original layer.

Using this method would mean layers don't have to be wrap-aware, and clones would have the exact behavior of the originals. Also this should be way more efficient than cloning and moving actors individually (performance hasn't been an issue though)

The event cloning thing in champlain_view would look something like this(pseudocode):

mouse_event_cb(event) {
 cloned_event = event
 cloned_event.x = x_to_wrap_x (event.x)
 original_actor = get_actor_at_pos(cloned_event.x, cloned_event.y)
 cloned_event.source = original_actor
 put(cloned_event)
 return true
}

The problem with this is that clutter only provides the "get_actor_at_pos" thingie at stage level(global coordinates) via the clutter_stage_get_actor_at_pos() function. This only works if the actor is visible on screen (doesn't work for negative coordinates, e.g. a marker that is not visible in the current viewport).

So I've been thinking I'd go over to Clutter and propose to add(and implement)
clutter_actor_get_child_at_pos() (a function that gets an actor's child based on its position, relative to it). This is certainly a feature clutter lacks.

What do you think? is it worth the hassle to implement the clutter function or the first method seems feasible enough ? :D
Comment 51 Jonas Danielsson 2016-06-21 18:12:13 UTC
(In reply to Marius Stanciu from comment #50)
> Hi!
> 
> Done with exams and everything, I'm now fully dedicated and ready to catch
> up!
> I've started pushing some of my work to my github repository at:
> https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap
> 

Great! Hope your exams went well!

> I've fixed the zooming animation stuttering and the marker dragging behaving
> weirdly. Now i'm working on making the user layer seamless over the clones.
> I've spent a lot of time trying different approaches, and I've come up with
> 2 viable ones:
> 
> 
> 1) Jiri's suggestion to make the layers wrap-aware and let them draw their
> copies.
> I've almost implemented this for the marker layer in the github wrap branch,
> you can test it. It works fine, but there is a noticeable disadvantage:
> implementing a custom layer now requires the developer to also code the
> clone stuff.
> 

I think any approach that does not give us wrapping seamlessly in custom layers is a bit lacking.


> 
> 2) Cloning events at champlain_view level:
> We could use the static user layer clones we tried before. We then catch all
> mouse events that occur on the clones and synthesize them over the original
> layer.
> 

What would this mean exactly? What will pop-up if I press a cloned mapmarker in Maps for instance?

> Using this method would mean layers don't have to be wrap-aware, and clones
> would have the exact behavior of the originals. Also this should be way more
> efficient than cloning and moving actors individually (performance hasn't
> been an issue though)
> 

You can try loading a custom geojson layer with all mcdonalds in the US in Maps to check performance :)

> The event cloning thing in champlain_view would look something like
> this(pseudocode):
> 
> mouse_event_cb(event) {
>  cloned_event = event
>  cloned_event.x = x_to_wrap_x (event.x)
>  original_actor = get_actor_at_pos(cloned_event.x, cloned_event.y)
>  cloned_event.source = original_actor
>  put(cloned_event)
>  return true
> }
> 
> The problem with this is that clutter only provides the "get_actor_at_pos"
> thingie at stage level(global coordinates) via the
> clutter_stage_get_actor_at_pos() function. This only works if the actor is
> visible on screen (doesn't work for negative coordinates, e.g. a marker that
> is not visible in the current viewport).
> 
> So I've been thinking I'd go over to Clutter and propose to add(and
> implement)
> clutter_actor_get_child_at_pos() (a function that gets an actor's child
> based on its position, relative to it). This is certainly a feature clutter
> lacks.
> 
> What do you think? is it worth the hassle to implement the clutter function
> or the first method seems feasible enough ? :D

If a function is reasonable I am sure you could get it into Clutter :) You can ask ebassi on GNOME irc if he approves.

Nice work!
Comment 52 Marius Stanciu 2016-06-23 09:51:06 UTC
The function ended up to be very complicated to implement (for a reasonable complexity) and out of desperation I've come up with a better and efficient approach. You can test it at https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap.


The way this works is the following:
Initially the user_layer is placed on the left, and static clones are wrapped on the right.

Whenever the cursor enters a clone, the clone is swapped with the original user_layers actor. This way, the original user_layers actor is always under the cursor and it is ready for any mouse event.

Also, no wrap-aware layers, so that's a big plus :D

The only issue remaining is the path layer being cut for some reason. From what I can tell, the path layer is only rendered partially, (only the visible part is rendered). I'll post any updates as soon as I figure how to fully render it :)
Comment 53 Jiri Techet 2016-06-23 10:15:12 UTC
(In reply to Marius Stanciu from comment #50)
> Hi!
> 
> Done with exams and everything, I'm now fully dedicated and ready to catch
> up!
> I've started pushing some of my work to my github repository at:
> https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap
> 
> I've fixed the zooming animation stuttering and the marker dragging behaving
> weirdly. 

Yep, works pretty nice now, good job.

> Now i'm working on making the user layer seamless over the clones.
> I've spent a lot of time trying different approaches, and I've come up with
> 2 viable ones:
> 
> 
> 1) Jiri's suggestion to make the layers wrap-aware and let them draw their
> copies.
> I've almost implemented this for the marker layer in the github wrap branch,
> you can test it. It works fine, but there is a noticeable disadvantage:
> implementing a custom layer now requires the developer to also code the
> clone stuff.

Almost works - there are just some problems with z ordering I think (sometimes the markers get obscured by the map when moved horizontally across map clone borders).

> 
> 
> 2) Cloning events at champlain_view level:
> We could use the static user layer clones we tried before. We then catch all
> mouse events that occur on the clones and synthesize them over the original
> layer.
> 
> Using this method would mean layers don't have to be wrap-aware, and clones
> would have the exact behavior of the originals. Also this should be way more
> efficient than cloning and moving actors individually (performance hasn't
> been an issue though)
> 
> The event cloning thing in champlain_view would look something like
> this(pseudocode):
> 
> mouse_event_cb(event) {
>  cloned_event = event
>  cloned_event.x = x_to_wrap_x (event.x)
>  original_actor = get_actor_at_pos(cloned_event.x, cloned_event.y)
>  cloned_event.source = original_actor
>  put(cloned_event)
>  return true
> }
> 
> The problem with this is that clutter only provides the "get_actor_at_pos"
> thingie at stage level(global coordinates) via the
> clutter_stage_get_actor_at_pos() function. This only works if the actor is
> visible on screen (doesn't work for negative coordinates, e.g. a marker that
> is not visible in the current viewport).
> 
> So I've been thinking I'd go over to Clutter and propose to add(and
> implement)
> clutter_actor_get_child_at_pos() (a function that gets an actor's child
> based on its position, relative to it). This is certainly a feature clutter
> lacks.
> 
> What do you think? is it worth the hassle to implement the clutter function
> or the first method seems feasible enough ? :D

Do we always want to clone all events? Imagine we (or someone) might want to add some right-click event showing a popup - with event cloning the event would be triggered for all the clones and the popup would show everywhere which is something we don't want. I'm slightly worried cloning the events might cause some unexpected problems in how users expect the markers work.

Another thing is if the change requires clutter modification, it will take some time before this feature gets to users because it will depend on the latest (future) clutter release. I also prefer being slightly conservative regarding dependencies so if someone wants to use the latest version of libchamplain, it doesn't require a completely bleeding edge distribution.

We currently have just two layers (path, marker) so it means doing the work just twice which doesn't sound that horrible (also, if there appears to be some shared code or pattern appearing in both implementations, the shared part could be moved to the super-class ChamplainLayer). I also don't think many users (if any) implement their own layers. The only useful missing layer (for which we have a bug request somewhere) is a clustering layer which groups many markers together and displays just a single marker instead (but I guess the implementation of this layer could be simplified by the fact that in this case moving the clustered markers doesn't make much sense so all the event-related code could be dropped for such a layer).

So I'd personally prefer the first option (but you may convince me about the opposite).
Comment 54 Jiri Techet 2016-06-23 10:20:56 UTC
(In reply to Marius Stanciu from comment #52)
> The function ended up to be very complicated to implement (for a reasonable
> complexity) and out of desperation I've come up with a better and efficient
> approach. You can test it at
> https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap.

Just a note - I probably tested this version when writing the comment above.
Comment 55 Jiri Techet 2016-06-23 10:35:03 UTC
(In reply to Marius Stanciu from comment #52)
> The function ended up to be very complicated to implement (for a reasonable
> complexity) and out of desperation I've come up with a better and efficient
> approach. You can test it at
> https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap.
> 
> 
> The way this works is the following:
> Initially the user_layer is placed on the left, and static clones are
> wrapped on the right.
> 
> Whenever the cursor enters a clone, the clone is swapped with the original
> user_layers actor. This way, the original user_layers actor is always under
> the cursor and it is ready for any mouse event.

Isn't there a problem when a marker's pointer starts in one instance of a map and overlaps to another one on the right? E.g. if at zoom level 0 you drag the "New York" marker so it points to New Zealand and afterwards you drag it from the position where it's overlapping to the map clone on the right. Does the correct layer get "activated"?

It seems to work most of the time for me but sometimes I'm unable to drag the markers.
Comment 56 Marius Stanciu 2016-06-23 13:59:09 UTC
Ah you're right. when markers are split on the border between two slots, only one half works. Damn, this seems hard to fix. I'll try to come up with something. Thanks :)
Comment 57 Marius Stanciu 2016-06-24 19:56:47 UTC
Done! This was quite challenging to fix but it should work now (you can test it at https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap).

The way this works is the following:

Swapping user layer slots depending on cursor position is still the same as i explained earlier.

Now, whenever a click is picked up on the real user layer, it is swapped successively with its right and left neighbors (if there are any) and the area at the event coordinates is sampled for a reactive child actor(eg a marker).

If such actor is found, the event is redirected to it.
Comment 58 Marius Stanciu 2016-06-30 17:00:49 UTC
I've come up with a fix for the path layer being cut-off on the clones.

The problem:

After studying the code a bit I've realized that the path layer is not really true to the scene-graph approach clutter normally uses (children transformations are not independent of parent transforms). This is because the layer drawing ( redraw_path and invalidate_canvas functions in champlain-path-layer.c) is dependent on the viewport's origin and size.

I've tried making it independent, but that meant drawing over the whole map(not only the currently visible area). It worked nicely with the map-wrapping, but for high zoom levels it was reaaaaally inefficient.

The solution:
I've added an extra canvas in the path layer (the path layer is now a map-wide actor with 2 canvas children). One is placed on the right, containing the paths visible on the original layer, and the other on the left, containing what is visible on the first clone. (the other clones won't contain any extra stuff)
The areas that are not visible in the viewport are not drawn (empty space between the canvases).

Now when the user_layers are cloned in champlain-view, clones will also display the missing path-layer section.

Notes:
This makes the path-layer somewhat clone-aware, but only to a low degree (it just knows there is more stuff visible other than what is visible on the original layer). Unfortunately, I don't see a way to make the path layer fully clone unaware since it depends on viewport position and size.

You can test it at the usual: https://github.com/StanciuMarius/Libchamplain-map-wrapping/tree/wrap
Comment 59 Jiri Techet 2016-07-05 10:03:43 UTC
Good, the path layer seems to be working much better now.

However, there's still a problem - try the following:

1. start the launcher-gtk demo
2. enable wrapping
3. zoom in to the maximum zoom level
4. zoom out to the mimimum zoom level

Now you will notice that some tiles are missing and the markers aren't draggable.

I suspect this is because of viewport "relocation" - some background first. Ideally, there would just be a simple viewport actor that covers the whole world onto which you'd place the tiles and this actor would then move based on how the user drags the map. The problem is we need quite a huge precision to do such a thing for the maximum zoom level - there are 2^18 tiles at the highest zoom level each of which is 256 pixels (i.e. 2^8) so to place the actors correctly on the viewport we'd need at least a 26-bit number. Unfortunately clutter uses floats with 24-bit mantissa so we are slightly doomed (what happens is that the precision isn't enough and at the highest zoom levels there start appearing gaps between tiles).

The solution is to have kind of segment-offset addressing. There's one more actor inserted in the viewport (see "child" inside champlain-viewport) which is shifted by the "anchor" value. Inside it, tiles are placed. So we gain the needed precision by having 2 float values - the anchor value plus the tile placement value.

This complicates stuff - we have to watch if we got to the border (*) of the viewport child actor when panning and if so, relocate it so the anchor changes so what the user sees is in the middle of the viewport child actor. ChamplainViewport fires the "relocated" signal on which ChamplainView reacts in view_relocated_cb() and notifies others with the "layer-relocated" on which layers should react by redrawing themselves because coordinates change. 

Relocation is also performed when zooming in-out but just when needed - have a look at champlain_viewport_set_origin() - at small zoom levels the values are always smaller than ANCHOR_LIMIT and no relocation happens here which is why you don't see effects of relocation on smaller zoom levels.

To trigger relocation more frequently, I suggest just setting the ANCHOR_LIMIT value to something much smaller (e.g. 1024) so you see its effects all the time.

---

* ANCHOR_LIMIT in the code. This isn't real hard-size of the actor, just some value that makes sure the anchor value doesn't get too large so we don't lose precision because of it. Right now it's set to G_MAXINT16 which should fit comfortably into float.
Comment 60 Jiri Techet 2016-07-05 10:09:53 UTC
There's one more issue (maybe related):

1. Zoom out to zoom level 0
2. Enable wrap
3. Pan to the right for some time
4. Zoom in

Usually what you see is one instance of the map correctly zoomed in but for the wrap there's just the blurred zoom actor map and not the actual map for this zoom level.

And by the way when panning the map, I'm getting

Clutter-CRITICAL **: clutter_canvas_set_size: assertion 'width >= -1 && height >= -1' failed

in the console - probably just some check missing.
Comment 61 Marius Stanciu 2016-07-14 15:04:33 UTC
Created attachment 331514 [details]
Relocation bug

After spending A LOT of time on this, I've come to the conclusion that the bug (clones not displaying properly after relocation) is actually caused by a clutter bug.

I've isolated the scenario in the attached code snippet to prove myself it's not anything unexpected.

The problem is that clones seem to not render at all if the source actor is too big (offsetting the tile within the map_layer like relocation does, makes the map_layer actor allocation bigger, I think). This seems to be tied with the size of it's children somehow. Try changing the ANCHOR to a value smaller than the TILE_SIZE and (eg. 399) and the clone will suddenly appear.


This behavior only appears to happen with clones, so a workaround on champlain_view would be to clone each tile individually, and then add them to a wrapper actor.
Do you think this is a good work-around? Would you proceed differently?
Comment 62 Jiri Techet 2016-07-16 18:35:39 UTC
(In reply to Marius Stanciu from comment #61)
> Created attachment 331514 [details]
> Relocation bug
> 
> After spending A LOT of time on this

This is exactly why I think it was a great idea from Jonas to make this a GSOC project to make someone do the dirty work for us ;-).

(Great job by the way!)

> I've come to the conclusion that the
> bug (clones not displaying properly after relocation) is actually caused by
> a clutter bug.
> 
> I've isolated the scenario in the attached code snippet to prove myself it's
> not anything unexpected.
> 
> The problem is that clones seem to not render at all if the source actor is
> too big (offsetting the tile within the map_layer like relocation does,
> makes the map_layer actor allocation bigger, I think). This seems to be tied
> with the size of it's children somehow. Try changing the ANCHOR to a value
> smaller than the TILE_SIZE and (eg. 399) and the clone will suddenly appear.

I would maybe rather suspect clipping in clutter - clutter tries not to draw actors outside the visible area and I guess

  clutter_actor_set_position (tile, -ANCHOR, -ANCHOR);

makes it think (for the purpose of the clone) the actor is outside the visible area even though it's not because the container is shifted the opposite direction.

But this is definitely a question worth asking on the clutter mailing list - maybe they know some workaround, maybe it's a bug that should be fixed in clutter and maybe it's something that has to be that way (clutter sometimes behaves in an unexpected way because it works with GPU).
 
> This behavior only appears to happen with clones, so a workaround on
> champlain_view would be to clone each tile individually, and then add them
> to a wrapper actor.
> Do you think this is a good work-around? Would you proceed differently?

For lower zoom levels we could always make sure the anchor value is zero (anchor isn't needed because the precision of a single float is enough) so this problem wouldn't appear there. But there is still a danger someone would zoom-in at the border and we'd probably get this problem at higher zoom levels.

Otherwise I have no smart idea related to this so probably the workaround will be necessary.
Comment 63 Marius Stanciu 2016-07-18 14:23:18 UTC
More progress to report!

1. After trying some random stuff, I've actually stumbled upon a better hot-fix for the presumed "clutter bug" mentioned above. It seems that setting a static size (using clutter_actor_set_size) for the source of the clones (map_layer, in our case) fixes the rendering issue nicely, without raising any other problems. (no idea why it works though)

2. Markers not responding to events after relocation: This was actually caused by the fact that after relocation, user layers render at negative coordinates (because of the anchor offset). Clutter actors do not receive any mouse events at negative coordinates so the "enter-event" for clone swapping was a no-go anymore. As a fix I used the pointer "motion-event" instead and calculated which clone has to be swapped from the event coordinates.

3. The path layer not rendering properly after relocation was an easy fix, just had to take the anchor in consideration in redraw_path. Also that meant I had to introduce a new function to the view (champlain_view_get_viewport_anchor). Not sure adding an extra API function was the right approach.

4. Fixed the "Clutter-CRITICAL **: clutter_canvas_set_size: assertion 'width >= -1 && height >= -1' failed" error. It was indeed a missing check.

5. Blurred zoom actor displaying instead of clones: the zoom_actor is destroyed in a callback when the last tile finishes downloading. If no tile is downloaded, then that callback never occurs, so the zoom_actor is not destroyed (still contains the scaled clones) and covers the high-res clones. As a fix, I've added a check in the zoom-animation-finished callback for that scenario.
Comment 64 Jiri Techet 2016-07-22 10:15:37 UTC
(In reply to Marius Stanciu from comment #63)
> More progress to report!

Great! It works really well now - I haven't spotted any problems at all! Are you aware of anything else that should be addressed?

> 
> 1. After trying some random stuff, I've actually stumbled upon a better
> hot-fix for the presumed "clutter bug" mentioned above. It seems that
> setting a static size (using clutter_actor_set_size) for the source of the
> clones (map_layer, in our case) fixes the rendering issue nicely, without
> raising any other problems. (no idea why it works though)

Still it would be good to ask the clutter guys if it is a bug and if the current approach with setting the actor size is correct (it would be stupid if it was something that works just by accident and breaks in the next clutter release because of some internal change).

> 3. The path layer not rendering properly after relocation was an easy fix,
> just had to take the anchor in consideration in redraw_path. Also that meant
> I had to introduce a new function to the view
> (champlain_view_get_viewport_anchor). Not sure adding an extra API function
> was the right approach.

I'd prefer not to expose the anchor thing (I did my best to hide it as much as possible in the API) but I don't see a way how to avoid it. So it's OK I think.

---

I went through the patches and in general they look good. I'll have minor comments here and there but nothing major. However, first it should be brought to some mergable state with respect to master. There are several options:

1. Rebasing the patch set on top of master
2. Merging master into the wrap branch (which will resolve all conflicts and wrap will then be easily mergable to master).
3. Partially one of the above plus manually re-creating some patches on top of that.

I think the ChamplainView stuff will be fine but what I suspect will cause bigger merge conflicts will be ChamplainPathLayer - Jonas introduced the possibility to export maps to a bitmap and I'm afraid this will clash with your patches quite badly (another thing is that the bitmap export should work with wrapping too). So you might want to perform the merge/rebase up to the point where you started modifying the path layer and re-create the path layer patches on top of it. (Also you might consider rebasing the wrap branch first and moving the path layer patches to the very top so the git-mergable portion with master is as big as possible). Just whatever makes your life easier.
Comment 65 Marius Stanciu 2016-07-25 14:12:36 UTC
(In reply to Jiri Techet from comment #64)

> you aware of anything else that should be addressed?

Actually, yeah:
1) Map source overlays(the thing where you add transparent maps on top) don't seem to clone properly and I haven't found the cause yet.
From what I understand, tiles from the overlaid map sources are added to the same actor as normal tiles, the champlain_view->priv->map_layer. The clones are built off that and should contain the overlays as well, but...they don't. Any ideas? I probably missed something so no biggie if you don't.

2) Now that wrapping is available, should paths also be allowed to wrap? Maybe someone would like to draw some airplane routes which don't circle the whole globe to get between two close points on the border. Have a look at attached image to see what i mean. Or maybe this case should be handled by the user by splitting the path into two.

> Still it would be good to ask the clutter guys if it is a bug and if the
> current approach with setting the actor size is correct (it would be stupid
> if it was something that works just by accident and breaks in the next
> clutter release because of some internal change).

I'll definitely ask on their IRC.

> I went through the patches and in general they look good. I'll have minor
> comments here and there but nothing major. However, first it should be
> brought to some mergable state with respect to master. There are several
> options:

When trying to merge master I've stumbled upon a conflict I'm not really sure how to approach:

Master has a new function, champlain_view_get_world, that from my understanding returns the bounding box of what is visible. Thing is, with wrapping enabled, two separate areas are visible at the same time (one on the right side of the original map_layer and the other on the left side of the clone).

Returning one bounding box containing both areas would return the whole world most of the time (at least horizontal-wise) so it would make the function useless. Returning two bounding boxes could work maybe. Any ideas?
Comment 66 Marius Stanciu 2016-07-25 17:48:49 UTC
Created attachment 332115 [details]
path wrapping

Oups, forgot to attach :D
Comment 67 Jiri Techet 2016-07-26 20:42:57 UTC
(In reply to Marius Stanciu from comment #65)
> (In reply to Jiri Techet from comment #64)
> 
> > you aware of anything else that should be addressed?
> 
> Actually, yeah:
> 1) Map source overlays(the thing where you add transparent maps on top)
> don't seem to clone properly and I haven't found the cause yet.
> From what I understand, tiles from the overlaid map sources are added to the
> same actor as normal tiles, the champlain_view->priv->map_layer. The clones
> are built off that and should contain the overlays as well, but...they
> don't. Any ideas? I probably missed something so no biggie if you don't.

Doesn't it have something to do with transparency? Do you get the clones visible if you make them non-transparent (just for testing)? Otherwise there's nothing very special about them as far as I know.

Even if it was some clutter problem and you didn't manage to get the overlay tiles working, it's not too critical. At the moment the servers providing overlay tiles are rather unreliable and I don't think many people use them. We can just document that overlay tiles don't work with wrapped maps.

> 
> 2) Now that wrapping is available, should paths also be allowed to wrap?
> Maybe someone would like to draw some airplane routes which don't circle the
> whole globe to get between two close points on the border. Have a look at
> attached image to see what i mean. Or maybe this case should be handled by
> the user by splitting the path into two.

I have been thinking about this but with the current API we don't know what he really wants - he might want either of the two cases from the screenshots. Moreover, to really draw nearest distances using lines, we'd have to have some support of "geodesic lines":

https://www.google.com/search?q=geodesic+line&espv=2&biw=1318&bih=806&source=lnms&tbm=isch&sa=X&ved=0ahUKEwi3i-_V6JDOAhUCKcAKHW1SAs8Q_AUIBygC#imgrc=Wvg_1lJCgVi3NM%3A

(e.g. the shortest distance from the north of Russia to the north of Canada is along a meridian which projects to a curve in the Mercator projection)

The path support we have is only suitable for short distances where straight lines are more or less preserved and such paths won't probably wrap. So probably not much to worry about with the current path layer implementation.

That doesn't mean I want to discourage you from improving the stuff if you have some ideas, but as the current code seems to be working reasonably well, I'd prefer getting it to a mergeable state (and merge it) and doing improvements like that later.

> 
> > Still it would be good to ask the clutter guys if it is a bug and if the
> > current approach with setting the actor size is correct (it would be stupid
> > if it was something that works just by accident and breaks in the next
> > clutter release because of some internal change).
> 
> I'll definitely ask on their IRC.
> 
> > I went through the patches and in general they look good. I'll have minor
> > comments here and there but nothing major. However, first it should be
> > brought to some mergable state with respect to master. There are several
> > options:
> 
> When trying to merge master I've stumbled upon a conflict I'm not really
> sure how to approach:
> 
> Master has a new function, champlain_view_get_world, that from my
> understanding returns the bounding box of what is visible. Thing is, with
> wrapping enabled, two separate areas are visible at the same time (one on
> the right side of the original map_layer and the other on the left side of
> the clone).
> 
> Returning one bounding box containing both areas would return the whole
> world most of the time (at least horizontal-wise) so it would make the
> function useless. Returning two bounding boxes could work maybe. Any ideas?

I'd just return the same value as in the non-wrap case - users probably don't want to write a different code for the wrap case and the non-wrap case. I believe this function was added to make sure the current zoom level is sufficient to cover certain area and zoom out if not - nothing too bad will happen if it's zoomed out a little more than needed. This behavior could be mentioned in the documentation of the function.
Comment 68 Marius Stanciu 2016-07-27 14:01:22 UTC
(In reply to Jiri Techet from comment #66)
Done merging!

Used git merge on half of the commits (until the path-layer ones), solved the conflicts, and then cherry-picked the rest.

Was planning on squashing some commits (commits that later get changed by other commits) to make the reviewing work easier for you, but it turned out to be a real ordeal. ( i'm certainly still in the learning phase of doing git stuff efficiently).

Now as far as what goes next...
You mentioned you had some minor comments on my work. You could comment directly on git-hub master commits, I then  patch all the raised issues, do the finishing touches, and maybe then update the gnome Champlain repository? Does that sound OK?
Comment 69 Jiri Techet 2016-07-30 23:10:59 UTC
(In reply to Marius Stanciu from comment #68)
> (In reply to Jiri Techet from comment #66)
> Done merging!
> 
> Used git merge on half of the commits (until the path-layer ones), solved
> the conflicts, and then cherry-picked the rest.
> 
> Was planning on squashing some commits (commits that later get changed by
> other commits) to make the reviewing work easier for you, but it turned out
> to be a real ordeal. ( i'm certainly still in the learning phase of doing
> git stuff efficiently).
> 
> Now as far as what goes next...
> You mentioned you had some minor comments on my work. You could comment
> directly on git-hub master commits, I then  patch all the raised issues, do
> the finishing touches, and maybe then update the gnome Champlain repository?
> Does that sound OK?

Sounds good. I'm just on holiday now and will have limited time until I come back on Wednesday next week.

For the review it would be best if you create a pull request on github - I think the official gnome github repository mirrors are read-only so you won't be able to create it here

https://github.com/GNOME/libchamplain

but I've just forked it here

https://github.com/techee/libchamplain

and you could create the pull request there. I'm just not sure how you created your repo and if github allows you to create the pull request (it might not know it's the same project) - if not, you'll have to fork (directly on github with the "fork" button in the top-right corner) either  of the above two repos, clone it on your machine, fetch into it the patches from your original repository and push it back to github. Then you should be able to create the pull request.

(I noticed you merged your wrap branch into master and not master into wrap which I suggested [just to resolve the merge conflicts]. It's not really a problem in this case but with git the workflow is that you work on your branch all the time and I as a maintainer do the final merge to master. So if you create the new repo, it's maybe better to put the patches to a branch and keep master reflect the actual master of libchamplain. This is really just a formal thing now because I can fetch the changes from master too but if you start working on some other project and will prepare several independent patch sets, you can just keep them in their own branches and keep master unmodified).
Comment 70 Jonas Danielsson 2016-08-01 06:34:01 UTC
Awesome work!

I just tried this out in Maps and it feels nice. I tried to export an image and I got a segmentation fault though. Is this supposed to work with surface exporting atm?
Comment 71 Jonas Danielsson 2016-08-01 06:36:27 UTC
> > Master has a new function, champlain_view_get_world, that from my
> > understanding returns the bounding box of what is visible. Thing is, with
> > wrapping enabled, two separate areas are visible at the same time (one on
> > the right side of the original map_layer and the other on the left side of
> > the clone).
> > 
> > Returning one bounding box containing both areas would return the whole
> > world most of the time (at least horizontal-wise) so it would make the
> > function useless. Returning two bounding boxes could work maybe. Any ideas?
> 
> I'd just return the same value as in the non-wrap case - users probably
> don't want to write a different code for the wrap case and the non-wrap
> case. I believe this function was added to make sure the current zoom level
> is sufficient to cover certain area and zoom out if not - nothing too bad
> will happen if it's zoomed out a little more than needed. This behavior
> could be mentioned in the documentation of the function.

get_world does return the bounding box of the world. It is used to limit what is possible to see/use. The use-case was when Maps starts with offline tiles that does not cover the whole world. We want to limit the view to that.

I would say disable wrap if world is not equal to the whole world. And have get_world return the same as it ever would, as said.
Comment 72 Marius Stanciu 2016-08-01 20:13:46 UTC
(In reply to Jiri Techet from comment #69)

>For the review it would be best if you create a pull request on github - I >think the official gnome github repository mirrors are read-only so you won't >be able to create it here

Added a pull request on techee/libchamplain.

(In reply to Jonas Danielsson from comment #70)
> I just tried this out in Maps and it feels nice. I tried to export an image
> and I got a segmentation fault though. Is this supposed to work with surface
> exporting atm?

Haven't worked on surface exporting on the version you tested, but I'll patch it as soon as we're done with the merging stuff. However, I wasn't able to reproduce a segfault in Maps though. Can you tell me the context in which you got it?


I also tested Maps with wrapping and the only bug I found was that marker popups would appear on original markers only, even if click occurred on cloned marker. I'll look for a fix.

>I would say disable wrap if world is not equal to the whole world. And have >get_world return the same as it ever would, as said.

Sounds like a plan! I'll commit after merge stuff.
Comment 73 Jiri Techet 2016-08-14 12:43:47 UTC
I've just merged the wrap branch from Marius (thanks!), see

https://github.com/techee/libchamplain/pull/2

As a result I'm closing this bug as it is implemented now. If anyone experiences some problems with the wrap code, please open a new bug for it.