Add checks for (non-)simple polygons

[lehecht]

Shapes are checked for overlapping points. Rectangles need 4 vertices, lines need at least 2 vertices and polygons need at least 3 vertices that do not overlap. Another solution could be realized with JSTS.

Still missing: Solution for bowties, dangling lines and pinch points.

Open question: Should JSTS be used?

[mzur]

You should be able to detect "pinch points" with a check if the coordinateSetSize has the same size than the original coordinate array. If not, there are at least two identical points. If the annotation is still valid after superfluous points are removed, it should be created.

I think "dangling lines" could be cleaned with an additional check if 3 subsequent points have either the same x or y coordinates. If this is the case, delete the point in the middle.

"Bow ties" are still a problem and not easily detectable or fixable. Maybe we should try a solution with JSTS? But I don't want to include the whole library. Instead, only the necessary modules should be imported. Still, we should have a look at the additional bundle size. It's currently at 1.8 MB already and I really don't want it to become larger than 2 MB.

These checks should also be implemented for the video annotation tool. I think at some point we should consolidate the code of the two tools, as many things are duplicated.

Finally, the backend should get additional checks for the respective API endpoints, so "pinch points" and "dangling lines" are rejected.

[lehecht]

I think "dangling lines" could be cleaned with an additional check if 3 subsequent points have either the same x or y coordinates. If this is the case, delete the point in the middle.

It is possible to draw a horizontal or vertical line with the magic wand tool. So this check cannot be applied to this areas which makes it a bit more difficult.

[mzur]

I understood that the cleaning is particularly relevant for the polygons of the magic wand tool. Can you give an example for "3 subsequent points have either the same x or y coordinates" where the point in the middle should not be deleted?

Please use the "re-request review" button above once you want me to look over the code again.

[lehecht]

Currently if the user draws a self-intersecting polygon, the biggest part is kept and the rest is discarded. Should I also add an info-box for this? Maybe it is a bit confusing for the user when part of its drawing just disappears.

By the way, I have used the polygon area to determine the biggest part. There are cases where a polygon with more points can be smaller than the part with fewer one. If you don't like this approach, let me know.

[mzur]

Note to myself: JSTS seems to increase the bundle size from 1.77 MB to 2.11 MB.

[dlangenk]

Currently if the user draws a self-intersecting polygon, the biggest part is kept and the rest is discarded. Should I also add an info-box for this? Maybe it is a bit confusing for the user when part of its drawing just disappears.

By the way, I have used the polygon area to determine the biggest part. There are cases where a polygon with more points can be smaller than the part with fewer one. If you don't like this approach, let me know.

I think it is fine this way. This check is rather for involuntary use of self-intersecting polygons and then it should work just fine.

[mzur]

You didn't request a review yet but I'll still leave one 😉

1. While the static methods of PolygonValidator essentially work the same, you don't have to wrap them with a class. You can just declare the functions and export them from the file. This is just a comment, you don't have to change this since it's working fine.

2. makePolygonSimple still modifies the feature that is passed as an argument internally but also returns it. I think this is risky, see my comments below.

[mzur]

Overall it looks good! I have only one change request. I found that you can still create a non-simple polygon by modifying an existing polygon. So the cleanup code has to be added to this code path as well.

Screencast.from.07.11.2023.15.07.44.webm

I've also experimented with conditional loading of the JSTS dependency so the main JS file does not get that big. It is quite simple (just call await import('module') and webpack will automatically generate an additional file that is loaded there). However, the code needs to be updated to work with the async/await call which is quite tedious. I think I defer a change like this for a later time when the main JS file gets even larger (maybe 5 MB or so). BIIGLE is a "power user" tool and we don't pay for the bandwidth so a 2 MB JS file won't hurt anyone for now.

[mzur]

You can still create a non-simple polygon by modifying an existing one in the video annotation tool.

[lehecht]

Ok thanks for the explanation! I think I'll tinker with this a bit once you added the comments about "polishing" and added the validation step to modified video annotations, so I better understand what's going on 😉

I don't know if it helps, but I recorded the bug that I fixed with the polish method. It doesn't happen all the time when modifying a magic wand polygon, but there are cases. Unfortunately, I forgot to zoom in to show the intersection, but I think you will see it yourself when you check out the code.

Bildschirmaufzeichnung.vom.16.11.2023.mp4

[mzur]

Ok I think this is good to go now! The original issue mentioned that the backend should reject non-simple polygons but I think we just leave this check in the frontend only.