add editFeatures function similar to selectFeatures

[timelyportfolio]

selectFeatures() by @tim-salabim is brilliant and helpful. editFeatures done properly could also be very nice, but will require a little more care to apply the edits, add, and deletes. I will try to describe my initial thoughts from quite a bit of experimentation.

immutability

editFeatures() should return a new sf with merged changes rather than attempting to mutate the original/source sf.

change original sf geometry

editFeatures() could add new a new non-primary geometry column for edits like a join might, add new property column to mark deletes, and add geometries by adding to the new edit column if done as stated in first item of the list with property column marker like deletes. However, I think it should simply merge the changes into the primary geometry column.

merge edits

Changing the geometry column of an sf is not well-documented, and I tried a whole lot of different ways. @tim-salabim though showed me the nifty little trick of assignment (timelyportfolio/r-spatial.org@bf13e81#commitcomment-22430626) with st_geometry that seems to work as expected. I would like to test this thoroughly, and if it works as it seems then add some examples and better documentation to sf::st_geometry().

[tim-salabim]

Surely editMap should return a new object with edited/changed geometry. Whether this should replace the original is up to the user (<-).

I kind of like the idea of appending a new geometry column... This way changes can be tracked easily (even intersecting old and new geometry is possible). I'd love to hear some thoughts by @edzer regarding this. If the mechanism of switching geometry_columns is working well, then this could, despite the overhead, be a really neat solution. We could also implement an argument allowing the user to choose whether to overwrite the original geometry or add a second geometry column.

[edzer]

Yeah, that's a nice use case for multiple geometry columns that I hadn't thought of. The constraint here is that a geometry column has the same number of geometries as there are features (rows in the table), and so if you'd delete a point you shouldn't remove it from the list but replace it with an empty geometry, e.g.

> st_geometrycollection()
GEOMETRYCOLLECTION()

or one of a more appropriate type such as

> st_multipolygon()
MULTIPOLYGON()

so that after editing the list still has the same length as it originally had. Not a bad idea, since we don't require list element IDs (list names) to identify features.

[tim-salabim]

Ok, so this should work not only work for editing (as I had originally thought) but also for deleting. Adding features would simply mean filling attribute columns with NAs I guess...

[edzer]

Yes, unless you add e.g. a point to an existing multipoint, a polygon to an existing multipolygon, or anything to an existing geometrycollection.

[timelyportfolio]

I love the idea that we could reproduce using the approach, but I foresee problems if there is a sequence such as add and then edit/delete additions. I think it is still do-able, but will require a different approach than what I had envisioned for editFeatures and what is already in editMap. Even with the merge, we might run into this trouble, and I thought I might add a sequence argument for a user to specify order of merge. In the above scenario, we would need to do add then delete, since delete cannot apply if evaluated first. It seems add -> edit -> delete would be best default.

For best reproducibility, we could record the actions/events with timestamp or sequential id, and then be able to play that back, but I think this would be a different set of functions targeting this concept. This approach would closely mirror something like Redux time traveller.

Or, we could try to write some functions to diff the output from editFeatures with the original sf, but I think the complexity of this is probably outside of the scope of mapedit. We could use mapview with multiple layers to examine changes.

I'd love to hear your feedback.

[timelyportfolio]

I guess no matter what both would be very helpful. I think the I am closer to a potential solution for merge all in one column, so given time constraint, I advocate for getting that in place before initial CRAN release and useR. Then, I will implement the record/playback method later.

[timelyportfolio]

devtools::install_github("timelyportfolio/mapedit@develop") contains a working editFeatures(). This one will be hard to test given potential number of combinations and order of operations. Also, I need to think through editMap now, since it in attempts with new features to merge internally by applying subsequent edits and deletes to drawn in finished. I think this still needs to happen even though it makes life a little more difficult in editFeatures.

[timelyportfolio]

I did a proof of concept in playback branch https://github.com/timelyportfolio/mapedit/tree/playback. This changes the return from edit* include an attribute recorder on the returned object.

[tim-salabim]

I can't seem to get the recording feature working?

[timelyportfolio]

@tim-salabim I forgot to note that I changed the recorder to be an attribute of the return value instead of a second item in the return list. Otherwise, the return value structure will be inconsistent and when using programatically unpredictable. Please let me know if the below code does not work.

# devtools::install_github("timelyportfolio/mapedit@playback")

library(mapedit)
library(mapview)
library(sf)

# for reproducibility and testing
#   a sample edit to test_sf
test_sf <- structure(
list(feature_type = c("polygon", "rectangle", "rectangle"
), id = c(77L, 89L, 94L), feature = structure(list(structure(list(
structure(c(8.7702, 8.7701, 8.7704, 8.7702, 50.8151, 50.8149,
50.8149, 50.8151), .Dim = c(4L, 2L))), class = c("XY", "POLYGON",
"sfg")), structure(list(structure(c(8.7705, 8.7705, 8.7709, 8.7709,
8.7705, 50.815, 50.8151, 50.8151, 50.815, 50.815), .Dim = c(5L,
2L))), class = c("XY", "POLYGON", "sfg")), structure(list(structure(c(8.7703,
8.7703, 8.7706, 8.7706, 8.7703, 50.8146, 50.8147, 50.8147, 50.8146,
50.8146), .Dim = c(5L, 2L))), class = c("XY", "POLYGON", "sfg"
))), n_empty = 0L, class = c("sfc_POLYGON", "sfc"), precision = 0, crs = structure(list(
epsg = 4326L, proj4string = "+proj=longlat +datum=WGS84 +no_defs"), .Names = c("epsg",
"proj4string"), class = "crs"), bbox = structure(c(8.7701, 50.8146,
8.7709, 50.8151), .Names = c("xmin", "ymin", "xmax", "ymax")))), row.names = c(NA,
3L), class = c("sf", "data.frame"), sf_column = "feature", agr = structure(c(NA_integer_,
NA_integer_), .Names = c("feature_type", "id"), .Label = c("constant",
"aggregate", "identity"), class = "factor"), .Names = c("feature_type",
"id", "feature")
)

edit_sf <- editFeatures(test_sf, record=TRUE)
attr(edit_sf, "recorder")

[tim-salabim]

@timelyportfolio yes it does work! Nice, I really like the recorder feature and the fact that it distinguishes between actions ("add", "edit", "delete").

[timelyportfolio]

Great, glad it works. Been trying to think of interesting or fun example all week. Also, thought flubber would be fun way to animate edits.

[tim-salabim]

Yeah, maybe we could have a fun function to flubberize changes :-)
An intresting us-case came up in this sf issue though I haven't had time to see whether it actually can be resolved using mapedit.

[edzer]

It would be a nice feature to be able to remove holes from polygons.

[tim-salabim]

OK, so it seems to work to delete individual vertices (by clicking on them - though rather buggy). But then I get the following error:

Error: nrow(x) == length(value) is not TRUE
In addition: Warning message:
drop ignored 

This was the setup

# devtools::install_github("nowosad/spData")
library(spData)
library(sf)
library(dplyr)
# library(tidyverse)
library(mapview)
library(mapedit)
library(leaflet.extras)

world_continents2 = world %>% 
  st_set_precision(10000) %>% 
  group_by(continent) %>% 
  summarise(pop = sum(pop, na.rm = TRUE), country_n = n())
plot(world_continents2[1]) 

editFeatures(world_continents2)

and I deleted a few vertices of the holes in Africa (you can verify that these are holes by zooming in quite far using mapview).

I think in many cases it would be desirable to be able to select vertices to be deleted by drawing a polygon (or rectangle) similar to this

[tim-salabim]

@timelyportfolio can we have editFeatures use the viewer instead of the miniPage?

[timelyportfolio]

I will add a viewer argument to all of the edit*/select* with paneViewer() as the default.

[tim-salabim]

Awesome!

[timelyportfolio]

@tim-salabim, @edzer is the editFeatures() function sufficient for now? if so, I will close, submit pull, and I will leave #26 open to track the reproducibility piece of this discussion.

[tim-salabim]

@timelyportfolio regarding editing of complete features, I think it is working well so far. Yet, when deleting individual vertices from polygons (haven't tried for lines) I get the above reported error. Any ideas?

[timelyportfolio]

I will debug the vertices issue today.

[timelyportfolio]

@tim-salabim, do you have a screenshot/screencast? Can you replicate with this smaller example from here?

# two POLYGONs, composed of three rings p1+p2 and p3
p1 <- cbind(x = c(0, 0, 0.75, 1,   0.5, 0.8, 0.69, 0), 
            y = c(0, 1, 1,    0.8, 0.7, 0.6, 0,    0))
p2 <- cbind(x = c(0.2, 0.2, 0.3, 0.5, 0.5, 0.2), 
            y = c(0.2, 0.4, 0.6, 0.4, 0.2, 0.2))
p3 <- cbind(x = c(0.69, 0.8, 1.1, 1.23, 0.69), 
            y = c(0, 0.6, 0.63, 0.3, 0))
library(sf)
g <- data.frame(two_polygons = 1:2)
g[["geometry"]] <- st_sfc(st_polygon(list(p1, p2[nrow(p2):1, ])), st_polygon(list(p3)))
g <- st_as_sf(g)

editFeatures(g)

I cannot seem to delete all the vertices from the hole in either case.

[tim-salabim]

No I can't. I think in my example above the hole got so small that it seemed to have disappeared. With your example I can only remove all vertices > 3 at which it stops deleting. Also, I noticed that hole vertices become un-editable when you move the enclosing polygon. Also, deleting hole vertices is a final deletion, i.e. it doesn't seem to respect pressing cancel to undo stuff. In any case, I think this is all not our problem at this point as this all stems from the Leaflet.Draw plugin if I'm not mistaken.

[timelyportfolio]

Ok, I noticed the same issues. I will plan to submit a pull request to master from develop with the editFeatures functionality. At that point, I plan to close this issue as long as you are ok with it.

[tim-salabim]

Yip. That is fine.

[timelyportfolio]

#33 closes