Merge pull request #769 from biigle/invalid-shapes

Reject invalid annotation shapes

app/Traits/HasPointsAttribute.php

@@ -29,18 +29,24 @@ public function validatePoints(array $points)
                 $valid = $size === 2;
                 break;
             case Shape::circleId():
-                $valid = $size === 3;
+                $valid = $size === 3 && intval(($points)[2]) !== 0;
                 break;
             case Shape::ellipseId():
             case Shape::rectangleId():
-                $valid = $size === 8;
+                $valid = $size === 8 && $this->countDistinctCoordinates($points) === 4;
+                break;
+            case Shape::polygonId():
+                $valid = $size % 2 === 0 && count($points) >= 8 && $this->countDistinctCoordinates($points) >= 3;
+                break;
+            case Shape::lineId():
+                $valid = $size % 2 === 0 && count($points) >= 4 && $this->countDistinctCoordinates($points) >= 2;
                 break;
             default:
                 $valid = $size > 0 && $size % 2 === 0;
         }
 
         if (!$valid) {
-            throw new Exception('Invalid number of points for shape '.$this->shape->name.'!');
+            throw new Exception('Invalid (number of) points for shape '.$this->shape->name.'!');
         }
 
         if ($this->shape_id === Shape::polygonId()) {
@@ -65,4 +71,19 @@ public function setPointsAttribute(array $points)
 
         $this->attributes['points'] = json_encode($points);
     }
+
+    /**
+     * Counts number of distinct points
+     * @param array $points containing the coordinates
+     * @return int number of distinct points
+     * **/
+    private function countDistinctCoordinates($points)
+    {
+        $points = collect($points);
+        // Use values to reset index
+        $x = $points->filter(fn ($x, $idx) => $idx % 2 === 0)->values();
+        $y = $points->filter(fn ($x, $idx) => $idx % 2 === 1)->values();
+        $coords = $x->zip($y)->unique();
+        return count($coords);
+    }
 }

resources/assets/js/annotations/annotatorContainer.vue

@@ -571,8 +571,27 @@ export default {
         dismissCrossOriginError() {
             this.crossOriginError = false;
         },
-        handleInvalidPolygon() {
-            Messages.danger(`Invalid shape. Polygon needs at least 3 non-overlapping vertices.`);
+        handleInvalidShape(shape) {
+            let count;
+            switch(shape){
+                case 'Circle':
+                    Messages.danger('Invalid shape. Circle needs non-zero radius');
+                    return;
+                case 'LineString':
+                    shape = 'Line'
+                    count = 2;
+                    break;
+                case 'Polygon':
+                    count = 'at least 3';
+                    break;
+                case 'Rectangle':
+                case 'Ellipse':
+                    count = 4;
+                    break;
+                default:
+                    return;
+            }
+            Messages.danger(`Invalid shape. ${shape} needs ${count} different points.`);
         },
     },
     watch: {

resources/assets/js/annotations/components/annotationCanvas.vue

@@ -46,6 +46,7 @@ import {defaults as defaultInteractions} from '@biigle/ol/interaction'
 import {getCenter} from '@biigle/ol/extent';
 import {shiftKeyOnly as shiftKeyOnlyCondition} from '@biigle/ol/events/condition';
 import {singleClick as singleClickCondition} from '@biigle/ol/events/condition';
+import { isInvalidShape } from '../utils';
 
 
 /**
@@ -515,44 +516,46 @@ export default {
             return this.convertPointsFromOlToDb(points);
         },
         handleNewFeature(e) {
-            if (this.hasSelectedLabel) {
-                let geometry = e.feature.getGeometry();
-                if (geometry.getType() === 'Polygon') {
-                    if (PolygonValidator.isInvalidPolygon(e.feature)) {
-                        this.$emit('is-invalid-polygon');
-                        // This must be done in the change event handler.
-                        // Not exactly sure why.
-                        this.annotationSource.once('change', () => {
-                            if (this.annotationSource.hasFeature(e.feature)) {
-                                this.annotationSource.removeFeature(e.feature);
-                            }
-                        });
-                        return;
+            if (!this.hasSelectedLabel) {
+                this.annotationSource.removeFeature(e.feature);
+                return;
+            }
+            
+            if (isInvalidShape(e.feature)) {
+                // This must be done in the change event handler.
+                // Not exactly sure why.
+                this.annotationSource.once('change', () => {
+                    if (this.annotationSource.hasFeature(e.feature)) {
+                        this.annotationSource.removeFeature(e.feature);
                     }
+                });
+                this.$emit('is-invalid-shape', e.feature.getGeometry().getType());
+                return;
+            }
 
-                    PolygonValidator.simplifyPolygon(e.feature);
-                }
+            let geometry = e.feature.getGeometry();
+            if (geometry.getType() === 'Polygon') {
+                PolygonValidator.simplifyPolygon(e.feature);
+            }
 
-                e.feature.set('color', this.selectedLabel.color);
+            e.feature.set('color', this.selectedLabel.color);
 
-                // This callback is called when saving the annotation succeeded or
-                // failed, to remove the temporary feature.
-                let removeCallback = () => {
-                    try {
-                        this.annotationSource.removeFeature(e.feature);
-                    } catch (e) {
-                        // If this failed, the feature was already removed.
-                        // Do nothing in this case.
-                    }
-                };
+            // This callback is called when saving the annotation succeeded or
+            // failed, to remove the temporary feature.
+            let removeCallback = () => {
+                try {
+                    this.annotationSource.removeFeature(e.feature);
+                } catch (e) {
+                    // If this failed, the feature was already removed.
+                    // Do nothing in this case.
+                }
+            };
+
+            this.$emit('new', {
+                shape: geometry.getType(),
+                points: this.getPoints(geometry),
+            }, removeCallback);
 
-                this.$emit('new', {
-                    shape: geometry.getType(),
-                    points: this.getPoints(geometry),
-                }, removeCallback);
-            } else {
-                this.annotationSource.removeFeature(e.feature);
-            }
         },
         deleteSelectedAnnotations() {
             if (!this.modifyInProgress && this.hasSelectedAnnotations && confirm('Are you sure you want to delete all selected annotations?')) {

resources/assets/js/annotations/ol/PolygonValidator.js

@@ -11,19 +11,6 @@ import Point from '@biigle/ol/geom/Point';
 import Polygon from '@biigle/ol/geom/Polygon';
 import Polygonizer from 'jsts/org/locationtech/jts/operation/polygonize/Polygonizer';
 
-/**
- * Checks if polygon consists of at least 3 unique points
- *
- * @param feature containing the polygon
- * @returns True if coordinates contains at least 3 unique points, otherwise false
- */
-export function isInvalidPolygon(feature) {
-    let polygon = feature.getGeometry();
-    let points = polygon.getCoordinates()[0];
-
-    return (new Set(points.map(xy => String([xy])))).size < 3;
-}
-
 /**
  * Makes non-simple polygon simple
  *

resources/assets/js/annotations/utils.js

@@ -0,0 +1,29 @@
+/**
+ * This function checks for invalid annotation shapes.
+ * 
+ * @param feature containing the video annotation to check
+ * @returns true, if a video annotation has an invalid shape, otherwise false.
+ * 
+ * **/
+let isInvalidShape = function (feature) {
+    let geometry = feature.getGeometry();
+    let points = [];
+    switch (geometry.getType()) {
+        case 'Circle':
+            return parseInt(geometry.getRadius()) === 0;
+        case 'LineString':
+            points = geometry.getCoordinates();
+            return (new Set(points.map(xy => String([xy])))).size < 2;
+        case 'Rectangle':
+        case 'Ellipse':
+            points = geometry.getCoordinates()[0];
+            return (new Set(points.map(xy => String([xy])))).size !== 4;
+        case 'Polygon':
+            points = geometry.getCoordinates()[0];
+            return (new Set(points.map(xy => String([xy])))).size < 3;
+        default:
+            return false;
+    }
+};
+
+export {isInvalidShape};

resources/assets/js/videos/components/videoScreen/drawInteractions.vue

@@ -1,10 +1,11 @@
 <script>
-import * as PolygonValidator from "../../../annotations/ol/PolygonValidator";
+import {simplifyPolygon} from "../../../annotations/ol/PolygonValidator";
 import DrawInteraction from '@biigle/ol/interaction/Draw';
 import Keyboard from '../../../core/keyboard';
 import Styles from '../../../annotations/stores/styles';
 import VectorLayer from '@biigle/ol/layer/Vector';
 import VectorSource from '@biigle/ol/source/Vector';
+import {isInvalidShape} from '../../../annotations/utils';
 /**
  * Mixin for the videoScreen component that contains logic for the draw interactions.
  *
@@ -163,20 +164,20 @@ export default {
             this.$emit('pending-annotation', null);
         },
         extendPendingAnnotation(e) {
+            if (isInvalidShape(e.feature)) {
+                // Disallow shapes with too few points.
+                this.$emit('is-invalid-shape', e.feature.getGeometry().getType());
+                // Wait for this feature to be added to the source, then remove it.
+                this.pendingAnnotationSource.once('addfeature', () => {
+                    this.pendingAnnotationSource.removeFeature(e.feature);
+                });
+                return;
+            }
+
             // Check polygons
             if (e.feature.getGeometry().getType() === 'Polygon') {
-                if (PolygonValidator.isInvalidPolygon(e.feature)) {
-                    // Disallow polygons with less than three non-overlapping points
-                    this.$emit('is-invalid-polygon')
-                    // Wait for this feature to be added to the source, then clear.
-                    this.pendingAnnotationSource.once('addfeature', () => {
-                        this.resetPendingAnnotation();
-                    });
-                    return;
-                }
-
                 // If polygon is self-intersecting, create simple polygon
-                PolygonValidator.simplifyPolygon(e.feature);
+                simplifyPolygon(e.feature);
             }
 
             let lastFrame = this.pendingAnnotation.frames[this.pendingAnnotation.frames.length - 1];

resources/assets/js/videos/components/videoScreen/modifyInteractions.vue

@@ -5,7 +5,8 @@ import ModifyInteraction from '@biigle/ol/interaction/Modify';
 import TranslateInteraction from '../../../annotations/ol/TranslateInteraction';
 import {shiftKeyOnly as shiftKeyOnlyCondition} from '@biigle/ol/events/condition';
 import {singleClick as singleClickCondition} from '@biigle/ol/events/condition';
-import * as PolygonValidator from "../../../annotations/ol/PolygonValidator";
+import {simplifyPolygon} from "../../../annotations/ol/PolygonValidator";
+import {isInvalidShape} from '../../../annotations/utils';
 
 const allowedSplitShapes = ['Point', 'Circle', 'Rectangle', 'WholeFrame'];
 
@@ -80,16 +81,14 @@ export default {
                     return this.featureRevisionMap[feature.getId()] !== feature.getRevision();
                 })
                 .map((feature) => {
+                    if (isInvalidShape(feature)) {
+                        this.$emit('is-invalid-shape', feature.getGeometry().getType());
+                        return;
+                    }
                     // Check polygons
                     if (feature.getGeometry().getType() === 'Polygon') {
-                        if (PolygonValidator.isInvalidPolygon(feature)) {
-                            // Disallow polygons with less than three non-overlapping points
-                            this.$emit('is-invalid-polygon')
-                            return;
-                        }
-                        
                         // If polygon is self-intersecting, create simple polygon
-                        PolygonValidator.simplifyPolygon(feature);
+                        simplifyPolygon(feature);
                     }
                     return {
                         annotation: feature.get('annotation'),

resources/assets/js/videos/videoContainer.vue

@@ -643,8 +643,27 @@ export default {
                 annotation.failTracking();
             }
         },
-        handleInvalidPolygon() {
-            Messages.danger(`Invalid shape. Polygon needs at least 3 non-overlapping vertices.`);
+        handleInvalidShape(shape) {
+            let count;
+            switch (shape) {
+                case 'Circle':
+                    Messages.danger('Invalid shape. Circle needs non-zero radius');
+                    return;
+                case 'LineString':
+                    shape = 'Line'
+                    count = 2;
+                    break;
+                case 'Polygon':
+                    count = 'at least 3';
+                    break;
+                case 'Rectangle':
+                case 'Ellipse':
+                    count = 4;
+                    break;
+                default:
+                    return;
+            }
+            Messages.danger(`Invalid shape. ${shape} needs ${count} different points.`);
         },
     },
     watch: {

resources/views/annotations/show.blade.php

@@ -95,7 +95,7 @@
             v-on:delete="handleDeleteAnnotations"
             v-on:measuring="fetchImagesArea"
             v-on:requires-selected-label="handleRequiresSelectedLabel"
-            v-on:is-invalid-polygon="handleInvalidPolygon"
+            v-on:is-invalid-shape="handleInvalidShape"
             ref="canvas"
             inline-template>
             @include('annotations.show.annotationCanvas')

resources/views/videos/show/content.blade.php

@@ -61,7 +61,7 @@
       v-on:attaching-active="handleAttachingLabelActive"
       v-on:swapping-active="handleSwappingLabelActive"
       v-on:seek="seek"
-      v-on:is-invalid-polygon="handleInvalidPolygon"
+      v-on:is-invalid-shape="handleInvalidShape"
       ></video-screen>
 <video-timeline
       ref="videoTimeline"

tests/php/Http/Controllers/Api/ImageAnnotationControllerTest.php

@@ -252,6 +252,24 @@ public function testStore()
         // policies are cached
         Cache::flush();
 
+        $response = $this->json('POST', "/api/v1/images/{$this->image->id}/annotations", [
+            'shape_id' => Shape::rectangleId(),
+            'label_id' => $label->id,
+            'confidence' => 1,
+            'points' => [844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44],
+        ]);
+        // shape is invalid
+        $response->assertStatus(422);
+
+        $response = $this->json('POST', "/api/v1/images/{$this->image->id}/annotations", [
+            'shape_id' => Shape::lineId(),
+            'label_id' => $label->id,
+            'confidence' => 1,
+            'points' => [844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44],
+        ]);
+        // shape is invalid
+        $response->assertStatus(422);
+
         $response = $this->post("/api/v1/images/{$this->image->id}/annotations", [
             'shape_id' => Shape::pointId(),
             'label_id' => $label->id,
@@ -336,6 +354,25 @@ public function update($url)
         $this->assertEquals(25, $this->annotation->points[1]);
     }
 
+    public function testUpdateInvalidPoints()
+    {
+        $this->beAdmin();
+
+        $this->annotation->points = [0, 1, 2, 3, 4, 5, 6, 7];
+        $this->annotation->shape_id = Shape::rectangleId();
+        $this->annotation->save();
+
+        $response = $this->json('PUT', "api/v1/image-annotations/{$this->annotation->id}", ['points' => [844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44]]);
+        $response->assertStatus(422);
+
+        $this->annotation->points = [0, 1, 2, 3, 4, 5, 6, 7];
+        $this->annotation->shape_id = Shape::lineId();
+        $this->annotation->save();
+
+        $response = $this->json('PUT', "api/v1/image-annotations/{$this->annotation->id}", ['points' => [844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44, 844.69, 1028.44]]);
+        $response->assertStatus(422);
+    }
+
     public function testUpdateValidatePoints()
     {
         $this->updateValidatePoints('api/v1/image-annotations');