Upgrade/leaflet v1.x#476
Upgrade/leaflet v1.x#476bhaskarvk wants to merge 74 commits intorstudio:masterfrom bhaskarvk:upgrade/leaflet-v1.x
Conversation
…do intersection with simple features (sf)
… editable spot for feature attributes
…non-standard styling as begin rewrite
… to use ControlStore
…Leaflet JS docs. Kept final pkg under 5MB
bhaskarvk
added
type: enhancement
|
Thank you @bhaskarvk and @timelyportfolio for all the hard work on this. I'm in the process of reviewing and will make this a priority this week. |
jcheng5
left a comment
jcheng5
left a comment
There was a problem hiding this comment.
Mostly easy fixes requested, except the question about colorNumeric/colorFactor for addRasterImage. We can discuss over vchat if you think that would help.
| #' @param method the method used for computing values of the new, projected raster image. | ||
| #' \code{"bilinear"} (the default) is appropriate for continuous data, | ||
| #' \code{"ngb"} - nearest neighbor - is appropriate for categorical data. | ||
| #' Ignored if \code{project = FALSE}. See \code{\link{projectRaster}} for details. |
There was a problem hiding this comment.
Do you think it's worth noting that this is only for the server-side reprojection step? On the client, resampling will be done for scaling purposes (bilinear for scaling down and nearest neighbor for scaling up, not configurable).
There was a problem hiding this comment.
Actually if this is for factors, colorNumeric is not appropriate either--it should be colorFactor instead. Would it make sense to automatically decide both ngb/bilinear and colorFactor/colorNumeric based on whether the raster is a factor, as @tim-salabim describes in #219 (comment)?
There was a problem hiding this comment.
I'll give this some thought.
There was a problem hiding this comment.
To clarify: I basically copied the parameter description from ?projectRaster from the raster package. I don't think leaflet should try to be smart and decide upon an interpolation method based on input type. In my opinion leaflet should provide a API that is flexible and lets the user decide which method to use. As I alluded to in my comment in #219 there are many instances where integer valued rasters are intended to be categorical. However, I don't think that all users are aware of the fact that there is a as.factor method in package raster. This basically boils down to users understanding their data types.
Regarding color matching, I think something like the following would be sufficient (I simply copied the default settings for colorFactor from ?colorFactor)
method <- match.arg(method, c("bilinear", "ngb"))
if (project) {
projected <- projectRasterForLeaflet(x, method)
} else {
projected <- x
}
bounds <- raster::extent(raster::projectExtent(raster::projectExtent(x, crs = sp::CRS(epsg3857)), crs = sp::CRS(epsg4326)))
if (!is.function(colors)) {
if (method == "ngb") {
colors <- colorFactor(colors, domain = NULL, levels = NULL, ordered = FALSE,
na.color = "#808080", alpha = FALSE, reverse = FALSE)
} else {
colors <- colorNumeric(colors, domain = NULL, na.color = "#00000000", alpha = TRUE)
}
}do the arg matching regardless of the state of argument project and use that to decide whether to map colors using colorNumeric or colorFactor.
@jcheng5 I am not sure I understand what you mean with your first comment? Where is that scaling being done?
There was a problem hiding this comment.
I don't think leaflet should try to be smart and decide upon an interpolation method based on input type. In my opinion leaflet should provide a API that is flexible and lets the user decide which method to use.
How about method = c("auto", "bilinear", "ngb")? I basically would like to ensure that if it's a factor, it gets ngb by default, and if it's numeric, it gets bilinear by default. But the user will need to tell us with integer if they want ngb.
@jcheng5 I am not sure I understand what you mean with your first comment? Where is that scaling being done?
It's done on the fly in JS as the canvas is rendered: https://github.com/bhaskarvk/leaflet/blob/fb7bc8178baab3faf662a07acfe060555b5d8bb4/javascript/src/methods.js#L1139-L1237
There was a problem hiding this comment.
method = c("auto", "bilinear", "ngb") is a good idea!
Thanks for the clarification on the scaling. Just to be completely sure I understand correctly, with server-side you mean before passing to JS? Hence, client is the JS side?
There was a problem hiding this comment.
Just to be completely sure I understand correctly, with server-side you mean before passing to JS? Hence, client is the JS side?
Yes and yes. Sorry for the confusion.
| ) | ||
| } | ||
|
|
||
| #' @describeIn map-methods Flys to a given location/zoom-level using smooth pan-zoom. |
There was a problem hiding this comment.
Good catch. btw flyTo and flyToBounds methods only work on an initialized map (i.e. a map with a center and zoom already set). So technically these methods are shiny only. I'll do some clean up here.
There was a problem hiding this comment.
Oh, OK--I was assuming it'd fly in from 0x0x0. In that case, I'd just throw an error for the local case.
inst/errors/errors.Rmd
Outdated
| @@ -0,0 +1,56 @@ | |||
| # Errors | |||
There was a problem hiding this comment.
Are any of these notes/errors still relevant? @timelyportfolio @bhaskarvk
There was a problem hiding this comment.
Mostly for @timelyportfolio to test and report. Kent, please also remove example files which use leaflet.extras from under inst/examples. If you want I can accept them as PRs on leaflet.extras. It doesn't make sense to have example code with downstream dependencies, unless @jcheng5 disagrees.
There was a problem hiding this comment.
Ok, I think I deleted all the tests, experiments, errors that I inadvertently left from my initial work. Sorry I forgot to remove earlier.
inst/examples/crosstalk_shiny.R
Outdated
| addTiles() %>% | ||
| addMarkers() | ||
|
|
||
| not_rendered <- TRUE |
There was a problem hiding this comment.
There was a problem hiding this comment.
should be deleted in ⬆️
| let ctGroup = this._byCrosstalkGroup[layerInfo.ctGroup]; | ||
| let layersForKey = ctGroup[layerInfo.ctKey]; | ||
| let idx = layersForKey ? layersForKey.indexOf(stamp) : -1; | ||
| let idx = layersForKey ? layersForKey.indexOf(+stamp) : -1; |
There was a problem hiding this comment.
This was @cpsievert's fix for #395, part of the PR #396 which I've merged with this PR.
There was a problem hiding this comment.
In that case I'd like to undo that conversion to numeric, and instead, convert the stamp to a string at the time that it's created/retrieved. layer-manager.js#42: let stamp = L.Util.stamp(layer); becomes let stamp = L.Util.stamp(layer) + "";
| "flyTo", | ||
| leaflet = { | ||
| map$x$flyTo = view | ||
| map$x$fitBounds = NULL |
There was a problem hiding this comment.
I don't think this is quite right; map$x$setView, map$x$fitBounds, map$x$flyTo, and map$x$flyToBounds all seem to be mutually exclusive--if you set one, I think you have to NULL the others. Perhaps better to introduce a function for this:
changeView <- function(map, setView = NULL, fitBounds = NULL, flyTo = NULL, flyToBounds = NULL) {
map$x$setView <- setView
map$x$fitBounds <- fitBounds
map$x$flyTo <- flyTo
map$x$flyToBounds <- flyToBounds
map
}Each of the setView, fitBounds, flyTo, and flyToBounds would call this function with the map plus one and only one of the other parameters.
There was a problem hiding this comment.
Yeah I think this part needs to be cleaned up a bit.
remove examples left from testing/experimentation
5 participants
@jcheng5 This PR supersedes #453. On top of #453 I've merged @timelyportfolio's #458. Then merged some other pending PRs #462, #405, #396, #393 which were pending for sometime.
I've verified almost everything from my side. But would appreciate one final round of testing from you and @timelyportfolio.
After this I'll work to migrate leaflet.extras and leaflet.esri to 1.x, and then we can release all 3 + mapview, mapedit to CRAN simultaneously. I am targeting a CRAN release early 2018, hopefully before rstudio::conf :)