Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggling layers rearranges order of layers #19

Open
rdewit opened this issue Apr 13, 2015 · 9 comments · May be fixed by #50
Open

Toggling layers rearranges order of layers #19

rdewit opened this issue Apr 13, 2015 · 9 comments · May be fixed by #50

Comments

@rdewit
Copy link

rdewit commented Apr 13, 2015

In both the tiled and untiled mode, toggling the layers in the layer switcher rearrange the order of the layers in the WMS request.

How to reproduce:

  • Load example and notice the order of the layers param: timezones,lakesrivers,airports,statecap
  • Uncheck and subsequently check the 'Time Zones' layer
  • Notice that the layers param is now: lakesrivers,airports,statecap,timezones

This may be related to #6.

@sheppard
Copy link
Member

Yes, this would be good to address. The fix will probably be to create another array or object like source._subLayers that doesn't delete layers when they are removed. That way we'll be able to preserve the order that the layers were first added. We might also want an explicit source.setSubLayerOrder function. We could also think about having bringToFront() on Layer raise the Layer within the Source (in addition to/instead of raising the source) but that will probably be too confusing.

I restricted #6 to discussing support for the pane argument, so it's no longer related to this issue.

@jasperswart
Copy link

jasperswart commented Sep 4, 2017

Hi there. Maybe I can help out here. We have the same issue.

When multiple layers are added the ordering of the layers is not consistent. And when we pan the map, the ordering gets updated again in an arbitrary order.

Is that related?

I can put some time in this issue? @sheppard

Please note: When I use the default leaflet sms (always tiled) layers it works correctly!

@sheppard
Copy link
Member

sheppard commented Sep 5, 2017

Sure, if you want to start a pull request I can provide feedback and merge. I don't know why panning would update the layer order but it is likely related.

@hpfast
Copy link

hpfast commented Sep 14, 2017

I've done some work on this and have a first draft of a fix along the lines of what @sheppard suggested. @jasperswart Can you provide a bit more information on your issue of ordering getting changed when you pan the map? Ideally something I can reproduce so I can see if my solution covers that as well. It doesn't happen for me with the example in this repo.

@hpfast
Copy link

hpfast commented Sep 14, 2017

hmm, I see it happens for example when there are layers being added from several sources. My initial solution keeps layers within a single source in the right order, but I when there are layers from multiple sources they start changing order.

@hpfast
Copy link

hpfast commented Sep 14, 2017

I've implemented a fix for this for sublayers of one source: hpfast@618f04e

However, multiple sources are still switching around randomly. I've modified the default example to illustrate this in a separate branch. Three layers from two source get added to the map and to the layer control. The top layer (from source 2) sometimes appears underneath the layers from source 1, on both layer control use and map pan/zoom. My hunch is that it has to do with the technique happening around here: https://github.com/hpfast/leaflet.wms/blob/44b0e6a33f9c4c32ac6a3cfdd9c28c8f604975fa/src/leaflet.wms.js#L392

I'm not quite sure how to proceed with this one. For sublayers within a source it makes sense to track the state, since it's internal to the Source class. But for each source, in the end, a separate image is created with a L.imageOverlay. It seems to me that using the same technique as for sublayers will result in state moving too much outwards. I've also tried that approach out, but the only mechanism I can think of to implement this is zIndex and it doesn't seem to be doing anything.

Might it be an idea to instead create a separate pane for each wms.source?

@sheppard do you perhaps have any suggestions?

@hpfast hpfast linked a pull request Sep 21, 2017 that will close this issue
@hpfast
Copy link

hpfast commented Sep 21, 2017

Hi, I opened a pull request with a fix for this. As I mentioned there (#50), it doesn't address the problem mentioned later in this thread by @jasperswart -- namely, ordering of layers from multiple sources. I haven't yet been able to find a fix for that. I haven't been able to figure out (yet) how to re-add changing overlays to the map while maintaining order, because there are several places throughout the code where that happens, and I can't quite untangle the flow :)

From what I've seen so far, it seems like it might be tricky to get this working given the way this plugin is set up to expose virtual individual layers to the user and handle those smartly with a single source internally. (kind of the core idea of this plugin). The logic needed for that abstraction makes it difficult to maintain order in the way we can do trivially for sublayers of a source (maybe it can be done, I haven't found it yet).

So the call I can't make: should the matter of multiple sources being ordered be pursued further on this issue or on another one?

@sheppard
Copy link
Member

Thanks, I will take a look at the PR soon.

I would say the multiple sources ordering is a separate issue. It's probably related to the swapping out of internal L.Overlay instances to reduce layer flickering (a technique inspired by esri-leaflet). The easiest fix is probably just to add support for panes (#6).

@hpfast
Copy link

hpfast commented Sep 22, 2017

great.

Yes, I think that's what's causing the problem with multiple sources. Hadn't seen #6!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants