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

editing + deleting in editFeatures does't work #48

Closed
tim-salabim opened this issue Jul 1, 2017 · 40 comments
Closed

editing + deleting in editFeatures does't work #48

tim-salabim opened this issue Jul 1, 2017 · 40 comments

Comments

@tim-salabim
Copy link
Member

@tim-salabim tim-salabim commented Jul 1, 2017

When I do

editFeatures(franconia[1:5, ]) %>% 
  mapview()

and in that session both delete a feature and edit another one, I get the error:

Error in CPL_get_bbox(obj, 3) : Not a matrix.

traceback:

22.
stop(structure(list(message = "Not a matrix.", call = CPL_get_bbox(obj, 
    3), cppstack = NULL), .Names = c("message", "call", "cppstack"
), class = c("Rcpp::not_a_matrix", "C++Error", "error", "condition"
))) 
21.
CPL_get_bbox(obj, 3) 
20.
structure(CPL_get_bbox(obj, 3), names = c("xmin", "ymin", "xmax", 
    "ymax")) 
19.
bbox.MtrxSetSetSet(obj) 
18.
structure(bb, class = "bbox", crs = st_crs(x)) 
17.
bb_wrap(bbox.MtrxSetSetSet(obj), obj) 
16.
st_bbox.sfc_MULTIPOLYGON(x) 
15.
st_bbox(x) 
14.
`[.sfc`(geom, i) 
13.
geom[i] 
12.
`[.sf`(orig, which(!(orig_ids %in% del_ids)), ) at merge.R#72
11.
orig[which(!(orig_ids %in% del_ids)), ] at merge.R#72
10.
merge_delete(structure(list(NUTS_ID = c("DE241", "DE242", "DE243", 
"DE244", "DE245"), SHAPE_AREA = c(0.00673601232111, 0.00842446859281, 
0.00598234136934, 0.00732948035553, 0.146698316001), SHAPE_LEN = c(0.392622499474, 
0.624726298357, 0.518547080489, 0.456981511253, 3.4819699025),  ... 
9.
eval(call(paste0("merge_", op), left_sf, sf_merge, c(edit_id = "layerId"))) 
8.
eval(call(paste0("merge_", op), left_sf, sf_merge, c(edit_id = "layerId"))) at edit.R#199
7.
f(init, x[[i]]) 
6.
Reduce(function(left_sf, op) {
    op <- tolower(op)
    if (op == "add") 
        sf_merge <- crud$finished ... at edit.R#172
5.
editFeatures.sf(franconia[1:5, ]) at edit.R#119
4.
editFeatures(franconia[1:5, ]) 
3.
eval(lhs, parent, parent) 
2.
eval(lhs, parent, parent) 
1.
editFeatures(franconia[1:5, ]) %>% mapview() 
@edzer
Copy link
Member

@edzer edzer commented Jul 1, 2017

That comes from sf; I didn't debug, but a reason might be that a POLYGON object has class MULTIPOLYGON, or vice versa, or something similar.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 1, 2017

Thanks @tim-salabim for the report, and thanks @edzer for confirming my suspicion. I think this relates to #32.

@edzer
Copy link
Member

@edzer edzer commented Jul 1, 2017

Doing sth similar, I see

Error in MtrxSetSet(x, dim, type = "MULTIPOLYGON", needClosed = TRUE) : 
  polygons not (all) closed

details:

 traceback()
32: stop("polygons not (all) closed")
31: MtrxSetSet(x, dim, type = "MULTIPOLYGON", needClosed = TRUE)
30: st_multipolygon(list(unclass(x)), dim = dim)
29: POLYGON2MULTIPOLYGON(x)
28: FUN(X[[i]], ...)
27: lapply(x, function(x) if (inherits(x, "POLYGON")) POLYGON2MULTIPOLYGON(x) else x)
26: st_cast_sfc_default(x)
25: st_cast.sfc(st_geometry(x), to, group_or_split = do_split)
24: st_cast(st_geometry(x), to, group_or_split = do_split)
23: st_cast.sf(x)
22: sf::st_cast(x)
21: sf::st_geometry(sf::st_cast(x))
20: getGeometryType(x)
19: standardColor(x)
18: vectorColors(x = x, zcol = zcol, colors = color, at = at, na.color = na.color)
17: leaflet_sf(x, map = map, zcol = zcol, color = color, col.regions = col.regions, 
        at = at, na.color = na.color, cex = cex, lwd = lwd, alpha = alpha, 
        alpha.regions = alpha.regions, na.alpha = na.alpha, map.types = map.types, 
        verbose = verbose, popup = popup, layer.name = layer.name, 
        label = label, legend = legend, legend.opacity = legend.opacity, 
        homebutton = homebutton, native.crs = native.crs, highlight = highlight, 
        maxpoints = maxpoints, ...)
16: .local(x, ...)
15: mapView(...)
14: mapView(...)
13: .Method(...)
12: eval(.dotsCall, env)
11: eval(.dotsCall, env)
10: standardGeneric("mapview")
9: mapview(.)
8: function_list[[k]](value)
7: withVisible(function_list[[k]](value))
6: freduce(value, `_function_list`)
5: `_fseq`(`_lhs`)
4: eval(quote(`_fseq`(`_lhs`)), env, env)
3: eval(quote(`_fseq`(`_lhs`)), env, env)
2: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
1: editFeatures(franconia[1:5, ]) %>% mapview()
> 
@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 2, 2017

@edzer what did you do? I cannot seem to reproduce the error you see.

@edzer
Copy link
Member

@edzer edzer commented Jul 2, 2017

Are you using the GH version of sf?

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 2, 2017

Yes, still the only error I get is Error in CPL_get_bbox(obj, 3) : Not a matrix.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 4, 2017

I can now replicate the polygons not (all) closed error. I think it only happens when you edit MULTIPOLYGONs (i.e. the big one or the one east of it. Maybe we need to cast them to POLYGON first, do the editing and then cast them back to MULTIPOLYGON before returning the object? Obviously, this only works for similar type collections (GEOMETRYs).

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 4, 2017

A quick trial seems to back my suspicion.

editFeatures(st_cast(franconia[1:5, ], "POLYGON")) %>%
  mapview()

does not seem to produce the polygons not (all) closed error.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 4, 2017

Darn, that is a troubling regression. I will research thoroughly tomorrow and hopefully at least trace the source.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 11, 2017

@edzer, @tim-salabim ok, I am going to need some help here to insure the correct behavior. After quite a bit of research, I don't think this is a regression in mapedit. Leaflet.Draw does not handle MULTIPOLYGON, so when we edit franconia, the edited features are returned as POLYGON GeoJSON. So, to reproduce the error without mapedit, we can simply

fran2 <- franconia
st_geometry(fran2[1,]) <- st_geometry(st_cast(franconia[1,],"POLYGON"))
plot(fran2)

which gives us

Error in CPL_get_bbox(obj, 3) : Not a matrix.

Compounding the Problem

In the simple case above with length==1, w could solve by attempting to cast or convert back to the original type before merging. Unfortunately, this does not work if we consider franconia[2,]
with length==2, which Leaflet.Draw returns as one feature of type POLYGON with two sets of coordinates. If I understand correctly, this will only happen with holes, or am I wrong?

image

If you tell me, this is ok, then we can just do as above and st_cast before merging. If it is not ok, it seems like we might have to make the decision to support holes, which we know Leaflet.Draw does not handle, or ignore holes and convert each set of coordinates to a polygon and make it a MULTIPOLYGON.

@edzer
Copy link
Member

@edzer edzer commented Jul 11, 2017

As we found out earlier, the

st_geometry(fran2[1,]) <- st_geometry(st_cast(franconia[1,],"POLYGON"))

causes trouble because it has a nested replacement function ([<- and st_geometry<-)

Why don't you simply use

st_cast(franconia, "POLYGON")

to cast franconia to POLYGON? OK, it changes franconia from 37 to 40 features, but isn't that what you want? In case you'd want to merge some of them together later on,

> lengths(st_geometry(franconia))
 [1] 1 2 1 1 2 1 1 1 1 1 1 1 1 1 1 1 2 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1

points you to the multi ones.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 11, 2017

That is where I need help from more experienced geo people in trying to determine the best "solution". I unfortunately don't have the domain expertise to know if cast to polygon first is acceptable. @tim-salabim, do you have a recommendation?

@edzer, I just can't stop myself with the nested assignment, but that does not seem to be the problem here. Any alternative gives us the same error, since it seems the problem is POLYGON amidst MULTIPOLYGON in sf.

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

fran2 <- franconia
st_geometry(fran2)[1] <- st_geometry(st_cast(franconia[1,],"POLYGON"))
plot(fran2)

fran3 <- franconia
st_geometry(fran3[1,]) <- st_geometry(st_cast(franconia[1,],"POLYGON"))
plot(fran3)

fran_g <- st_geometry(franconia)
fran_g[1] <- st_geometry(st_cast(franconia[1,],"POLYGON"))
plot(fran_g)
@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 11, 2017

@edzer, would the correct way to do this be this or a derivative?

fran_g <- st_geometry(franconia)
fran_g[1] <- st_geometry(st_cast(franconia[1,],"POLYGON"))
fran_g2 <- st_sfc(fran_g)
plot(fran_g2)
@edzer
Copy link
Member

@edzer edzer commented Jul 11, 2017

Yes, or the shorter:

fran_g <- st_geometry(franconia)
fran_g[1] <- st_cast(fran_g[1], "POLYGON")
fran_g2 <- st_sfc(fran_g)
plot(fran_g2)
@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 11, 2017

I will try to find some time to have a closer look at this tomorrow, though may not be able to investigate until Thursday. From a first rough look (and as I mentioned before) a cast to POLYGON seems inevitable (and is maybe also the case for the other MULTI* flavours?). It is a little trickier for collections of type GEOMETRY, especially if attempting to re-merge after the editing (if that even makes sense as users could delete all MULTI-parts so that nothing is left for merging). Maybe a suitable warning/documentation is needed to alert users that MULTI* features will be split into their individual parts and that it is up to the user to re-merge them after editing?

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

Some of this might be "solved" by fixing the merge as @edzer suggested. This will be only a partial fix, but I think it will be at least better than current. I will try in the morning.

edzer added a commit to r-spatial/sf that referenced this issue Jul 12, 2017
@edzer
Copy link
Member

@edzer edzer commented Jul 12, 2017

@timelyportfolio all of your examples with subsetting in it should now work - thanks for examples & the patience!

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

didn't know I was being helpful but delighted to know that I might have been :) This will make mapedit merge much easier and hopefully more "correct". I will test on the mapedit side now. Thanks!!!!!!

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

Now that my flawed assignment works :), we can focus on the optimal solution for mapedit. As expected, editing MULTIPOLYGON gives us a result, but that result is not quite right. Could mapedit smartly handle these in some way without first casting to POLYGON? I will explore, and if successful we might also be able to solve #32.

franconia[2,]

mapedit_franconia2

holes

mapedit_holes

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

In both cases, Leaflet.Draw, gives us POLYGON with length > 1.

In case 1 (franconia[2,]), the target for merge is MULTIPOLYGON. Can we simply st_cast(...ftr..., "MULTIPOLYGON")? It seems like this works with initial testing, but would need more iteration, testing, and feedback.

In case 2 (holes), the target for merge is POLYGON. Inspect the recorded edit, it seems this is returned correctly, but the structure after conversion is incorrect. Coordinates are merged into a single set.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 12, 2017

can you post the structure of how these two come back from Leaflet.Draw?

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

@tim-salabim, yes definitely. Will post once I remove my own ignorance/stupidity from potential source of error :)

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

Ok, @tim-salabim, will you look over my shoulder and see what I might be missing? The merged edit looks like this, which looks correct to me.

  .. ..$ feature  :Classes ‘sf’, ‘tbl_df’, ‘tbl’ and 'data.frame':	1 obs. of  3 variables:
  .. .. ..$ X_leaflet_id: int 49
  .. .. ..$ layerId     : chr "1"
  .. .. ..$ geometry    :sfc_POLYGON of length 1; first list element: List of 3
  .. .. .. ..$ : num [1:5, 1:2] 0 10 10 0 0 0 0 10 10 0
  .. .. .. ..$ : num [1:5, 1:2] 1 1.1 3.16 6.02 1 ...
  .. .. .. ..$ : num [1:5, 1:2] 4.48 3.08 7.43 7.25 4.48 ...
  .. .. .. ..- attr(*, "class")= chr  "XY" "POLYGON" "sfg"

... but when I str(st_geometry(pl_ed)), I get a single set/list of concatenated coordinates

sfc_POLYGON of length 1; first list element: List of 1
 $ : num [1:15, 1:2] 0 10 10 0 0 ...
 - attr(*, "class")= chr [1:3] "XY" "POLYGON" "sfg"
``
@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

Going to see if tibble is the problem.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 12, 2017

Yeah, my first thought also

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

working myself into a circle and don't want to tangle you up in the whirlpool. I think I copy/pasted wrong bit. Will wait to post again until I convince myself that I am showing correct pieces.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

I think I found source. Will report back.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

If you get some time, please test #52. If your tests all work, I think this issue might be resolved leaving only

  • a decision on forced cast to MULTIPOLYGON if target is MULTIPOLYGON potentially expanded to all MULTI* types

  • more discussion on #32 - with this fix we get closer to handling holes, since they can be edited, but not moved.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 12, 2017

Two observations:

  • yes, we should cast to MULTIPOLYGON when original feature came in as MULTIPOLYGON. There is no harm in having type MULTIPOLYGON even if only one feature (in case of deletion by user). I have not tested with other MULTI* types so far.
  • when I editFeatures(franconia[2, ]) then edit some stuff, save (the LeafletDraw save) and then activate editing again and edit some more, I get
Warning message:
In sf::st_geometry(orig2)[matched_id_rows] <- sf::st_geometry(edits) :
  number of items to replace is not a multiple of replacement length

and only the first edits are saved (though anything I add/draw after the first save is kept).

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 12, 2017

I've just tested MULTILINESTRING and MULTIPOINT.

  • MULTIPOINT won't work as leaflet doesn't support this type. Hence, users will need to cast to POINT even if simply trying to view in leaflet and hence also editing with editFeatures.
  • MULTIPLINESTRING works as long as there is only one feature, if there are indeed several features, then nothing seems to work (neither edit nor delete):
editFeatures(mapview::trails[1, ]) # works
editFeatures(mapview::trails[4, ]) # doesn't work
@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

@tim-salabim, I bet the edit_id column is causing some conflicts. Would you be ok if we remove the internally added edit_id column from the returned sf in editFeatures? I just pushed that change, and it seems to fix the problem.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 12, 2017

sure

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

working on the other multi* now

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

@tim-salabim, first quick exploration reveals Leaflet.Draw doesn't work with MULTILINESTRING of length > 1. Darn, this means we will have to cast to LINESTRING. Going to see if multiple LINESTRING with same id might offer some hope.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 12, 2017

Got cast to original implemented but discovered a new issue with MULTIPOLYGON and POLYGON length>1, so going to research that before attempting to resolve LINESTRING. The issue stems from multiple successive edits in a session with save in between each edit.

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Jul 13, 2017

I think I have previous bugs resolved, Now back to LINESTRING and MULTILINESTRING.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Jul 13, 2017

After a quick trial this morning I seem to be able to confirm that

  • casting to original type and
  • multiple edits (with intermediate saves) in one session

now work as expected.
Thanks!

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Aug 2, 2017

going to move the MULTILINESTRING issue to #62

@timelyportfolio
Copy link
Contributor

@timelyportfolio timelyportfolio commented Feb 26, 2018

@tim-salabim, I think we can close this, since the remaining unresolved MULTILINESTRING has been promoted.

@tim-salabim
Copy link
Member Author

@tim-salabim tim-salabim commented Feb 26, 2018

Agreed

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.