qfes / rdeck

Deck.gl widget for R
https://qfes.github.io/rdeck
MIT License
97 stars 0 forks source link

Feature request: auto conversion to WGS84 CRS for features in `editor_options()` + `initial_bounds` based on feature #95

Open elipousson opened 1 year ago

elipousson commented 1 year ago

I have a quick feature request for the editor parameter of rdeck() where it would be great if the features object was automatically converted to the correct CRS and, if no initial bounds is provided, the feature is used to set the initial bounds. It could work something like this:

rdeck <- function(..., initial_bounds = NULL, editor = FALSE) {

  if (is.list(editor)) {
    if (is_wgs84(editor[["features"]])) {
      editor[["features"]] <- sf::st_transform(editor[["features"]], 4326)
    }

    initial_bounds <- initial_bounds %||% editor[["features"]]
  }

  rdeck::rdeck(
    ...,
    initial_bounds = initial_bounds,
    editor = editor
  )
}

Happy to open a pull request if you're open to this idea!

anthonynorth commented 1 year ago

I agree that having to think about CRS is annoying and it would be nice if that's just handled for you, but I think users should be alerted when their CRS isn't what rdeck expects, particularly given how slow transform can be.

Spatial data used in our maps are often not EPSG 4326, so we bump into the st_transform() dance all the time.

Perhaps an opt-in transform geometries, but be noisy about it (warning) could work. I'm currently rewriting some of the internals and part of that work will see {sf} removed as a dependency, so any auto-transform logic will need to be behind a rlang::check_installed() guard.

I'll give this a bit of thought.

elipousson commented 1 year ago

If you end up heading in this direction, let me know if I can help!

One small thought on noisy functions: if you use cli::cli_alert_warning() instead of warning() or cli_warn() it makes it easier to suppress the message by setting the cli.default_handler option to suppressMessages(). Using cli would also allow you to set the frequency of the alerts so it could be once per session instead of every time you run the function with a non EPSG 4326 object.

anthonynorth commented 10 months ago

I wonder if it's worth relaxing the ESPG:4326 constraint, instead being any crs that a coordinate transform from whatever provided to ESPG:4326 would be a noop? I think we can pre-compute that?

For transform, we can use wk::wk_trans_explicit() and sf::sf_project(). This should be much less overhead than converting whatever our geometries are (which aren't necessary sfc) to sfc, do a transform, then convert back. This also allows us to apply the auto transform just prior to serialisation which works directly with wk coords anyway.

Perhaps something like

trans_coords <- function(coords, from_crs, to_crs = wk::wk_crs_longlat()) {
  from <- wk::wk_crs_proj_definition(from_crs)
  to <- wk::wk_crs_proj_definition(to_crs)

  if (is_trans_noop(from, to)) {
    return(coords)
  }

  rlang::check_installed("sf")

  wk::wk_transform(
    coords,
    wk::wk_trans_explicit(
      sf::sf_project(from, to, vctrs::vec_data(coords$xy))
    )
  )
}

And where serialisation occurs

if (auto_transform) {
  # if have coords already
  geom_coords <- trans_coords(geom_coords, wk::wk_crs(geom))

  # if writing geojson, need to extract coords first. could be optimised a little
  wk::wk_coords(geom) <- trans_coords(xy_coords(geom), wk::wk_crs(geom))
}

Edit: fix code

anthonynorth commented 10 months ago

This is a little better, geojson and non-geojson paths are almost the same.

coord_trans_filter <- function(coords, from_crs, to_crs = wk::wk_crs_longlat()) {
  from <- wk::wk_crs_proj_definition(from_crs)
  to <- wk::wk_crs_proj_definition(to_crs)

  if (from == to || is_trans_noop(from, to)) {
    # identity
    return(wk::wk_trans_affine(diag(3)))
  }

  rlang::check_installed("sf")

  wk::wk_trans_explicit(
    sf::sf_project(from, to, vctrs::vec_data(coords$xy))
  )
}

# transform coords
geom_coords <- wk::wk_transform(geom_coords, coord_trans_filter(geom_coords, wk::wk_crs(geom)))
# transform geometry
geom <- wk::wk_transform(geom, coord_trans_filter(xy_coords(geom), wk::wk_crs(geom)))

Edit: Identity transform exists

elipousson commented 10 months ago

I don’t know the wk package hardly at all but I’m really curious to try this out.

anthonynorth commented 7 months ago

hypertidy/proj 0.5 will give us a lightweight transformer that works with all geometry formats rdeck supports (https://qfes.github.io/rdeck/reference/wk-geometry.html) and with minimal overhead.

I'm thinking auto-coordinate transforms should be an option, which defaults to an rlang::inform("not ogc:crs84 etc").