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

Add options to addRasterImage #692

Merged
merged 4 commits into from
Feb 18, 2024
Merged

Add options to addRasterImage #692

merged 4 commits into from
Feb 18, 2024

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Jul 3, 2020

The other layer methods have options parameters, not sure why addRasterImage didn't. Apparently @tim-salabim needs this for pane support. https://twitter.com/TimSalabim3/status/1279105173378531335

(Note: Lots of .Rd files changed, this is because I used a more recent roxygen version)

library(raster)

r <- raster(xmn = -2.8, xmx = -2.79, ymn = 54.04, ymx = 54.05, nrows = 30, ncols = 30)
values(r) <- matrix(1:900, nrow(r), ncol(r), byrow = TRUE)
crs(r) <- CRS("+init=epsg:4326")

leaflet() %>% addTiles() %>%
  addMapPane("hello", 100000) %>%
  addRasterImage(r, colors = "Spectral", opacity = 0.8, options = tileOptions(pane = "hello"))

PR task list:

  • Update NEWS
  • Add tests (where appropriate)
    • R code tests: tests/testthat/
    • Visual tests: R/zzz_viztest.R
  • Update documentation with devtools::document()

@jcheng5 jcheng5 requested a review from schloerke July 3, 2020 23:19
R/layers.R Outdated Show resolved Hide resolved
Copy link
Contributor

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to make sure it's suppose to be gridOptions() (or not). Otherwise, LGTM!

@tim-salabim
Copy link
Contributor

Thanks for the quick action!

I can confirm that this works, although, as @schloerke points out, we currently need to use tileOptions() to pass additional options.

library(raster)
library(leaflet)
library(leaflet.extras2)

r1 <- raster(xmn = -2.8, xmx = -2.79, ymn = 54.04, ymx = 54.05, nrows = 30, ncols = 30)
values(r1) <- matrix(1:900, nrow(r1), ncol(r1), byrow = TRUE)
crs(r1) <- CRS("+init=epsg:4326")

r2 <- raster(xmn = -2.8, xmx = -2.79, ymn = 54.04, ymx = 54.05, nrows = 30, ncols = 30)
values(r2) <- matrix(1:900, nrow(r2), ncol(r2), byrow = FALSE)
crs(r2) <- CRS("+init=epsg:4326")

leaflet(
  options = leafletOptions(maxZoom = 20)
) %>%
  addMapPane("left", zIndex = 1) %>%
  addMapPane("right", zIndex = 1) %>%
  addTiles(
    group = "base"
    , layerId = "baseid"
    , options = tileOptions(pane = "right")
  ) %>%
  addProviderTiles(
    providers$CartoDB.DarkMatter
    , group = "carto"
    , layerId = "cartoid"
    , options = tileOptions(pane = "left")
  ) %>%
  addRasterImage(
    r1
    , group = "leftImg"
    , opacity = 0.6
    , options = tileOptions(
      , pane = "left"
      , maxZoom = 20
    )
  ) %>%
  addRasterImage(
    r2
    , group = "rightImg"
    , opacity = 1
    , options = tileOptions(
      , pane = "right"
      , maxZoom = 20
    )
  ) %>%
  addLayersControl(overlayGroups = c("leftImg", "rightImg")) %>%
  addSidebyside(
    layerId = "sidecontrols"
    , rightId = "baseid"
    , leftId = "cartoid"
  )

Screenshot from 2020-07-04 10-43-56

FWIW, there have been PRs in the past for this:

#508 & #524

Not sure whether it's better to have a dedicated gridOptions() or if tileOptions() can be used instead.?

@jcheng5
Copy link
Member Author

jcheng5 commented Jul 10, 2020

Shoot, I didn't see the past PRs. 🤦‍♂️

I created gridOptions because tileOptions contains some options that don't apply for grid layers. Maybe that's not worth introducing another function for, though?

@tim-salabim
Copy link
Contributor

I think gridOptions makes things clearer. Tiles are usually something else than a regular grid/raster. So I vote for gridOptions.

@tim-salabim
Copy link
Contributor

Upon further investigating the GridLayer and TileLayer options, we should definitely have gridOptions. TileLayer inherits options from GridLayer, so it only seems logical to have gridOptions.

@jcheng5 jcheng5 added this to the v2.1 milestone Aug 25, 2020
@jcheng5
Copy link
Member Author

jcheng5 commented Aug 26, 2020

Rebase to master before merging

RWParsons added a commit to RWParsons/iTRAQI_app that referenced this pull request Apr 13, 2022
By not needing the pane option in the rasters, the specific version of leaflet (rstudio/leaflet#692) isn't required.
@rhijmans
Copy link
Contributor

I would also love to have gridOptions.

@roelmay
Copy link

roelmay commented Jan 20, 2023

Hi, Is there any update on the status on merging the raster options ("joe/feature/raster-options") for an earlier version of leaflet into the main and newest version of leaflet?

@rhijmans
Copy link
Contributor

It would be great to have gridOptions

@amb26
Copy link

amb26 commented Feb 21, 2023

I would also love to see the options argument added

@jeremyash
Copy link

Just re-iterating that options for addRasterImage would be great to have. I've been using 'joe/feature/raster-options' to have control over plotting order of raster images with panes, but I'm now also using terra and it's not compatible with those earlier functions. Thanks!

@jcheng5
Copy link
Member Author

jcheng5 commented Feb 10, 2024

@jeremyash Thanks for the nudge. I've now updated the joe/feature/raster-options branch, but something's completely borked with terra/raster on my travel computer, so I'm not actually able to test it. If you (or anyone reading this) could try it and confirm that it works with both terra and raster objects, I will merge this right away.

@jeremyash
Copy link

@jcheng5 Just ran some code using terra and raster objects, and it worked fine on my end. Really appreciate your responsiveness here! Many thanks!

@jcheng5 jcheng5 merged commit 5cfe70b into main Feb 18, 2024
11 checks passed
@jcheng5 jcheng5 deleted the joe/feature/raster-options branch February 18, 2024 21:13
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.

7 participants