Rapid will let you draw self-intersecting polygons

mvexel commented 1 year ago

Description

Unlike iD, Rapid will happily let you draw (and upload) a self-intersecting polygon. There is no warning in the upload dialog that there is an invalid polygon in the changeset.

https://user-images.githubusercontent.com/120452/205717049-ba3d6290-a1ec-441a-96fe-57ae194ed46e.mov

PROOF!

Version

2.00-alpha3

What browser are you seeing the problem on? What version are you running?

Firefox 107 Mac

Steps to reproduce

Enter area drawing mode
Draw a self-intersecting polygon
optional: upload to OSM

The browser URL at the time you encountered the bug

https://mapwith.ai/rapid-v2-alpha3#background=EsriWorldImagery&datasets=fbRoads,msBuildings&disable_features=boundaries&map=21.60/40.74016/-111.86084

Bonkles commented 1 year ago

Two issues here: One, we should be showing a validation error with this. If not, that's a problem I think.

Two requires a bit of Story time!

Way back when, before iD/RapiD had validations, we would prevent self-intersecting ways from being drawn. if the user drew one, we'd just delete it and back it out from history.
Now that we have validations, we could just let them draw the Bad Thing and call it out as a validation.

I'm leaning towards just re-introducing the 'self-intersecting way' check at the close of the draw gesture and doing what we used to do, perhaps with a quick toast message saying 'hey, you can't do that'.

@bhousel @mvexel what do you think?

1) Allow self-intersecting ways but ensure validations fire correctly when drawn 2) Utterly prevent as we used to, explain why we removed the shape

bhousel commented 1 year ago

Ah yeah this is tricky, but yeah Ben's story gets the details exactly right - For now, I took that "check for self intersections" code out of the drawing code because it really didn't belong there.

We could probably put it back , but I'd like to implement it more wrapped up in a neat function like checkPointsForSelfIntersections() that we can call from the validator and also sometimes while drawing (but not every animation frame).

We also might restore the "nope" shapes - which was how we implemented those 🚫 cursors in the past when the user tried to actually move the shape there. For now those are also out, but the new renderer is also much faster at letting us know what shape and segment the user is over, so I'd just need a few days to think about it.

We have an opportunity now to draw lines as separate segments, which would let us know better whether the user is trying to connect to something they shouldn't - also make it possible to implement a JOSM-style extrude drawing mode eventually.

mvexel commented 1 year ago

I think the way iD does it makes most sense to me as a user, simply not letting me click to add a point to an area feature if doing that would result in an invalid polygon.

bhousel commented 1 year ago

I think the way iD does it makes most sense to me as a user, simply not letting me click to add a point to an area feature if doing that would result in an invalid polygon.

Cool, yeah I think we all agree that it would be great to put these geometry checks back in, so that the user gets feedback as they draw.

facebook / Rapid