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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cmcaine
Copy link

@cmcaine cmcaine commented Jun 21, 2018

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
Copy link
Author

cmcaine commented Jun 21, 2018

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

@schloerke
Copy link
Contributor

schloerke commented Jun 21, 2018

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 schloerke changed the title Add setStyle() [WIP [WIP] Add setStyle() Jun 21, 2018
@cmcaine
Copy link
Author

cmcaine commented Jun 21, 2018

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.

@schloerke
Copy link
Contributor

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
Copy link
Contributor

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 jj@rstudio.com.
"
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

@cmcaine
Copy link
Author

cmcaine commented Jun 23, 2018

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 cmcaine force-pushed the master branch 2 times, most recently from cef407a to 21f6b5a Compare June 26, 2018 10:37
@schloerke
Copy link
Contributor

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 schloerke changed the title [WIP] Add setStyle() [WIP] Add setPathStyle(map, group, pathOptions()) Jun 27, 2018
This work sponsored by [Integrated Transport
Planning](https://www.itpworld.net) and the World Bank.

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).
Should pass pathOptions but can't be bothered to fix it rn

Also fix devtools::document() unexporting setStyleFast.
@cmcaine cmcaine force-pushed the master branch 3 times, most recently from 79f0a1f to 54b1042 Compare July 12, 2018 22:10
@CLAassistant
Copy link

CLAassistant commented Oct 2, 2019

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.

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

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

Successfully merging this pull request may close these issues.

4 participants