leaflet icon indicating copy to clipboard operation
leaflet copied to clipboard

[WIP] Add setPathStyle(map, group, pathOptions())

Open cmcaine opened this issue 6 years ago • 8 comments

This work is sponsored by Integrated Transport Planning Ltd and the World Bank.

Re-styling existing features on the map is very slow because the geometry is sent each time. This PR contains working code to solve that problem by adding a new method that restyles existing features.

The function that I've written is a proof of concept that doesn't exactly fit with the other Leaflet functions, so this PR is about sorting that out and exploring ways of delivering this optimisation transparently.

Context:

My work involves visualising various properties of features. I do that in a Shiny app by styling the features differently dependent on a variable of interest selected by the user.

Solution:

I can style existing elements with a bit of JS and the latency goes from ~30s for a large dataset to basically instantaneous.

// javascript/src/methods.js

/** Much more performant way to style loaded geometry */
methods.setStyle = function(group, styles) {
  window.map = this;
  console.log('SETSTYLE', group, styles);
  let layers = this.layerManager.getLayerGroup(group).getLayers();
  for (let i = 0; i < styles.length; i++) {
    layers[i].setStyle(styles[i]);
  }
}
# Call with a group and a vector of style lists of length N. The first N
# features of the group will be restyled.
#
# Example: 
#
#   renderLeaflet("map", {
#     leaflet() %>% addPolygons(data = zones, group = "zones", color = "red")
#   })
#   colour = "blue"
#   styles = lapply(pal(values), function(colour) {list(fillColor=colour, color=colour)})
#   leafletProxy("map") %>%
#     setStyle("zones", styles)
setStyle = function(map, group, styles) {
    invokeMethod(map, NULL, "setStyle", group, styles)
}

I think this is a valuable feature, but obviously the signature and behaviour of this function don't really match the rest of the API.

One way this optimisation could be delivered would be to work out server side if the geometry has already been sent. This could be done by comparing memory addresses of the geometry column for standard copy-on-write data.frames or could be turned on only if the user indicates immutability somehow.

Does that sound sensible? Is there a better way in R of quickly discovering if a large vector has been modified?

cmcaine avatar Jun 21 '18 12:06 cmcaine

Original post edited. I pressed enter by accident earlier :)

cmcaine avatar Jun 21 '18 12:06 cmcaine

The current CRAN implementation is to add new polygons to the existing map. Your proposed solution is to change the css style for existing objects within a group. Correct?

I like the setStyles idea and understand the speedup as it never has to communicate 'real' data back to the shiny server. I am leaning towards only having a single style within setStyle(group, style). With the speedup, it should be ok to call this multiple times with your groups. Would it be possible in your situation to expand your group names to account for this situation?

With setStyles, the style could be applied to any object layer (polylines, circles, etc.), not just polygons. Correct?

Addressing option two, immutable data or hashing what has been sent,... that is a rabbit hole that gets complicated quickly. I'd prefer to not go that direction. There's also the example where you want to actually add a second, duplicate polygon for some reason. This would be hard to distinguish from just wanting to restyle the existing data.

schloerke avatar Jun 21 '18 13:06 schloerke

The current CRAN implementation is to add new polygons to the existing map. Your proposed solution is to change the css style for existing objects within a group. Correct?

It calls leaflet's setStyle method, which may use css, but doesn't necessarily (e.g. for the canvas backend), but yeah, that's basically it.

I like the setStyles idea and understand the speedup as it never has to communicate 'real' data back to the shiny server.

I think the speedup is more that the shiny server doesn't send more data, rather than about data coming back to the server.

I am leaning towards only having a single style within setStyle(group, style). With the speedup, it should be ok to call this multiple times with your groups.

It's probably fast enough, but it's possible that leaflet will batch the redraw if you send multiple styles at once.

I think an interface like setStyle(map, "groupname", fillColor = "blue", color = "red", weight = 4) would be quite nice.

Would it be possible in your situation to expand your group names to account for this situation?

I don't quite understand this question. Could you clarify that for me?

With setStyles, the style could be applied to any object layer (polylines, circles, etc.), not just polygons. Correct?

I haven't tested that, but I'm pretty sure it will work.

Addressing option two, immutable data or hashing what has been sent,... that is a rabbit hole that gets complicated quickly. I'd prefer to not go that direction. There's also the example where you want to actually add a second, duplicate polygon for some reason. This would be hard to distinguish from just wanting to restyle the existing data.

One could just maintain a cache of geometry in the browser and modify add* to only send the geometry if it won't be in the cache.

We'd need functions for shiny to ask what's cached and methods.js/addLayers would have to be modified to understand the cache, but I think it could work ok.

Might be that really clearing the geometry and re-adding it is the problem, not the data transfer time, in which case we might want to have clearGroup actually just hide the elements? I think it's the data transfer and parsing, though.

cmcaine avatar Jun 21 '18 15:06 cmcaine

Would it be possible in your situation to expand your group names to account for this situation?

I don't quite understand this question. Could you clarify that for me?

I wasn't too clear. Mainly, I would prefer to set a single style, rather than N styles in a single function call. If setStyle(map, "groupname", fillColor = "blue", color = "red", weight = 4) would work for you, I am happy.


For api setup, I'd like to follow how most of the other functions handle the parameters. Since setStyle works for all Path objects (don't want to include geojson), then we can use pathOptions().

So something like

setPathStyle <- function(map, group, style = pathOptions()) {
  invokeMethod(map, NULL, "setStyle", group, style)
}

I'm not sold if it should be setStyle or setPathStyle.

schloerke avatar Jun 21 '18 20:06 schloerke

Hi @cmcaine

Could you ...

" 2. Ensure that you have signed the individual or corporate contributor agreement as appropriate. You can send the signed copy to [email protected]. " Excerpt from rstudio/httpuv package, which you had no reason to know existed.

Please let me know when you've sent the email.

Thank you for your help!

Best, Barret

schloerke avatar Jun 21 '18 20:06 schloerke

Performance still wasn't good enough when styling 20000 features at once. Turns out it's something to do with how shiny sends different data types between R and JS.

Shiny passes arrays much faster than objects. I don't know if the delay is in encoding or transmission. You can then reassemble the required objects in JS basically for free.

The same optimisation will probably be possible for addPolylines, etc. At the moment shiny sends an array of arrays of arrays of objects of arrays. Which is unnecessarily convoluted and requires the creation of N small objects and at least 4N small arrays.

I suspect that it will be faster to send a smaller number of larger arrays -- something like the format provided by st_coordinates -- or possibly to use a space efficient encoding scheme like Google's encoded polyline (plugin available for leaflet).

If I have time I'll look into it myself, but no guarantees :)

contributor agreement

I'm happy to sign this but it'll have to wait until I get access to a printer and check it off with the consultancy. ITP and the World Bank are both keen to release Open Source, so there should be no problem.

NEWS file, etc

Happy to do this too, haven't bothered yet because the code presented so far is just PoC.

For api setup, I'd like to follow how most of the other functions handle the parameters. Since setStyle works for all Path objects (don't want to include geojson), then we can use pathOptions().

Are you proposing that we have two functions, one accepting pathOptions and one with a signature as I described? I don't understand why we'd need both, maybe there's an internal API that you're talking about?

cmcaine avatar Jun 23 '18 08:06 cmcaine

Performance still wasn't good enough when styling 20000 features at once. Turns out it's something to do with how shiny sends different data types between R and JS.

I agree that sending data is not cheap and the structure could be less complicated. Leaflet.js currently implements multi polygons as arrays of (arrays of tuple arrays).


For api setup, I'd like to follow how most of the other functions handle the parameters. Since setStyle works for all Path objects (don't want to include geojson), then we can use pathOptions().

Are you proposing that we have two functions, one accepting pathOptions and one with a signature as I described? I don't understand why we'd need both, maybe there's an internal API that you're talking about?

I'd like one R function and one corresponding js function:

setPathStyle <- function(map, group, style = pathOptions()) {
  invokeMethod(map, NULL, "setPathStyle", group, style)
}

This function should not include labels for tooltips. I'd like to keep the function isolated to just updating Path object style. Tooltips have many more arguments and the current implementation is add tooltip only, not update label of existing tooltips.


Could you provide me with a small shiny app that has the 20k features and the styles that you want to display? I don't have a toy dataset that big to hit performance issues.

Thank you for your help!

schloerke avatar Jun 27 '18 14:06 schloerke

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: cmcaine
:x: markjd84
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Oct 02 '19 15:10 CLAassistant