Merge pull request #468 from ThomasJClark/nt

Fix NTPublishOperation problems

Author: ThomasJClark

core/src/main/java/edu/wpi/grip/core/operations/composite/BlobsReport.java

@@ -60,7 +60,7 @@ public Mat getInput() {
         return this.input;
     }
 
-    @NTValue(key = "x")
+    @NTValue(key = "x", weight = 0)
     public double[] getX() {
         final double[] x = new double[blobs.size()];
         for (int i = 0; i < blobs.size(); i++) {
@@ -69,7 +69,7 @@ public double[] getX() {
         return x;
     }
 
-    @NTValue(key = "y")
+    @NTValue(key = "y", weight = 1)
     public double[] getY() {
         final double[] y = new double[blobs.size()];
         for (int i = 0; i < blobs.size(); i++) {
@@ -78,7 +78,7 @@ public double[] getY() {
         return y;
     }
 
-    @NTValue(key = "size")
+    @NTValue(key = "size", weight = 2)
     public double[] getSize() {
         final double[] sizes = new double[blobs.size()];
         for (int i = 0; i < blobs.size(); i++) {

core/src/main/java/edu/wpi/grip/core/operations/composite/ContoursReport.java

@@ -63,7 +63,7 @@ private synchronized Rect[] computeBoundingBoxes() {
         return boundingBoxes.get();
     }
 
-    @NTValue(key = "area")
+    @NTValue(key = "area", weight = 0)
     public double[] getArea() {
         final double[] areas = new double[(int) contours.size()];
         for (int i = 0; i < contours.size(); i++) {
@@ -72,7 +72,7 @@ public double[] getArea() {
         return areas;
     }
 
-    @NTValue(key = "centerX")
+    @NTValue(key = "centerX", weight = 1)
     public double[] getCenterX() {
         final double[] centers = new double[(int) contours.size()];
         final Rect[] boundingBoxes = computeBoundingBoxes();
@@ -82,7 +82,7 @@ public double[] getCenterX() {
         return centers;
     }
 
-    @NTValue(key = "centerY")
+    @NTValue(key = "centerY", weight = 2)
     public double[] getCenterY() {
         final double[] centers = new double[(int) contours.size()];
         final Rect[] boundingBoxes = computeBoundingBoxes();
@@ -92,7 +92,7 @@ public double[] getCenterY() {
         return centers;
     }
 
-    @NTValue(key = "width")
+    @NTValue(key = "width", weight = 3)
     public synchronized double[] getWidth() {
         final double[] widths = new double[(int) contours.size()];
         final Rect[] boundingBoxes = computeBoundingBoxes();
@@ -102,7 +102,7 @@ public synchronized double[] getWidth() {
         return widths;
     }
 
-    @NTValue(key = "height")
+    @NTValue(key = "height", weight = 4)
     public synchronized double[] getHeights() {
         final double[] heights = new double[(int) contours.size()];
         final Rect[] boundingBoxes = computeBoundingBoxes();
@@ -112,7 +112,7 @@ public synchronized double[] getHeights() {
         return heights;
     }
 
-    @NTValue(key = "solidity")
+    @NTValue(key = "solidity", weight = 5)
     public synchronized double[] getSolidity() {
         final double[] solidities = new double[(int) contours.size()];
         Mat hull = new Mat();

core/src/main/java/edu/wpi/grip/core/operations/composite/LinesReport.java

@@ -77,7 +77,7 @@ public List<Line> getLines() {
         return this.lines;
     }
 
-    @NTValue(key = "x1")
+    @NTValue(key = "x1", weight = 0)
     public double[] getX1() {
         final double[] x1 = new double[lines.size()];
         for (int i = 0; i < lines.size(); i++) {
@@ -86,7 +86,7 @@ public double[] getX1() {
         return x1;
     }
 
-    @NTValue(key = "y1")
+    @NTValue(key = "y1", weight = 1)
     public double[] getY1() {
         final double[] y1 = new double[lines.size()];
         for (int i = 0; i < lines.size(); i++) {
@@ -95,7 +95,7 @@ public double[] getY1() {
         return y1;
     }
 
-    @NTValue(key = "x2")
+    @NTValue(key = "x2", weight = 2)
     public double[] getX2() {
         final double[] x2 = new double[lines.size()];
         for (int i = 0; i < lines.size(); i++) {
@@ -104,7 +104,7 @@ public double[] getX2() {
         return x2;
     }
 
-    @NTValue(key = "y2")
+    @NTValue(key = "y2", weight = 3)
     public double[] getY2() {
         final double[] y2 = new double[lines.size()];
         for (int i = 0; i < lines.size(); i++) {
@@ -113,7 +113,7 @@ public double[] getY2() {
         return y2;
     }
 
-    @NTValue(key = "length")
+    @NTValue(key = "length", weight = 4)
     public double[] getLength() {
         final double[] length = new double[lines.size()];
         for (int i = 0; i < lines.size(); i++) {
@@ -122,7 +122,7 @@ public double[] getLength() {
         return length;
     }
 
-    @NTValue(key = "angle")
+    @NTValue(key = "angle", weight = 5)
     public double[] getAngle() {
         final double[] angle = new double[lines.size()];
         for (int i = 0; i < lines.size(); i++) {

core/src/main/java/edu/wpi/grip/core/operations/networktables/NTNumber.java

@@ -15,7 +15,7 @@ public NTNumber(Number number) {
         this.number = number.doubleValue();
     }
 
-    @NTValue
+    @NTValue(weight = 0)
     public double getValue() {
         return number;
     }

core/src/main/java/edu/wpi/grip/core/operations/networktables/NTPublishOperation.java

@@ -1,6 +1,8 @@
 package edu.wpi.grip.core.operations.networktables;
 
 import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Ordering;
 import com.google.common.eventbus.EventBus;
 import edu.wpi.first.wpilibj.networktables.NetworkTable;
 import edu.wpi.first.wpilibj.tables.ITable;
@@ -9,8 +11,8 @@
 import java.io.InputStream;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.Arrays;
+import java.util.Comparator;
 import java.util.Optional;
 import java.util.function.Function;
 
@@ -26,7 +28,7 @@ public class NTPublishOperation<S, T extends NTPublishable> implements Operation
 
     private final Class<S> type;
     private final Function<S, T> converter;
-    private final List<Method> ntValueMethods = new ArrayList<>();
+    private final ImmutableList<Method> ntValueMethods;
 
     /**
      * Create a new publish operation for a socket type that implements {@link NTPublishable} directly
@@ -49,15 +51,27 @@ public NTPublishOperation(Class<S> socketType, Class<T> reportType, Function<S,
         this.type = checkNotNull(socketType, "Type was null");
         this.converter = checkNotNull(converter, "Converter was null");
 
-        // Any accessor method with an @NTValue annotation can be published to NetworkTables.
-        for (Method method : reportType.getDeclaredMethods()) {
-            if (method.getAnnotation(NTValue.class) != null) {
-                if (method.getParameters().length > 0) {
+        Comparator<Method> byWeight = Comparator.comparing(method -> method.getAnnotation(NTValue.class).weight());
+
+        // Any accessor method with an @NTValue annotation can be published to NetworkTables.  We sort them by their
+        // "weights" in order to avoid the issue of different JVM versions returning methods in a different order.
+        this.ntValueMethods = ImmutableList.copyOf(Arrays.asList(reportType.getDeclaredMethods()).stream()
+                .filter(method -> method.getAnnotation(NTValue.class) != null)
+                .sorted(byWeight)
+                .iterator());
+
+        // In order for NTPublishOperation to call the accessor methods, they must all have no parameters
+        this.ntValueMethods.stream()
+                .filter(method -> method.getParameterCount() > 0)
+                .findAny()
+                .ifPresent(method -> {
                     throw new IllegalArgumentException("@NTValue method must have 0 parameters: " + method);
-                }
+                });
 
-                ntValueMethods.add(method);
-            }
+        // The weight thing doesn't help us if two methods have the same weight, since the JVM could put them in either
+        // order.
+        if (!Ordering.from(byWeight).isStrictlyOrdered(this.ntValueMethods)) {
+            throw new IllegalArgumentException("@NTValue methods must have distinct weights: " + reportType);
         }
     }
 
@@ -117,21 +131,36 @@ public void perform(InputSocket<?>[] inputs, OutputSocket<?>[] outputs) {
 
         // Get a subtable to put the values in.  Each NTPublishable has multiple properties that are published (such as
         // x, y, width, height, etc...), so they're grouped together in a subtable.
-        final ITable subtable;
+        final ITable table, subtable;
         synchronized (NetworkTable.class) {
-            subtable = NetworkTable.getTable("GRIP").getSubTable(subtableName);
+            table = NetworkTable.getTable("GRIP");
+            subtable = table.getSubTable(subtableName);
         }
 
         // For each NTValue method in the object being published, put it in the table if the the corresponding
         // checkbox is selected.
         try {
             for (Method method : ntValueMethods) {
                 String key = method.getAnnotation(NTValue.class).key();
-                if ((Boolean) inputs[i++].getValue().get()) {
-                    subtable.putValue(key, method.invoke(value));
+                boolean publish = (Boolean) inputs[i++].getValue().get();
+
+                if (key.isEmpty()) {
+                    // If there is no key specified, put the value directly in the key of this report instead of a
+                    // subtable
+                    if (publish) {
+                        table.putValue(subtableName, method.invoke(value));
+                    } else {
+                        table.delete(subtableName);
+                    }
                 } else {
-                    subtable.delete(key);
+                    // Otherwise, put a value in the subtable specified by the input to this operation
+                    if (publish) {
+                        subtable.putValue(key, method.invoke(value));
+                    } else {
+                        subtable.delete(key);
+                    }
                 }
+
             }
         } catch (IllegalAccessException | InvocationTargetException e) {
             Throwables.propagate(e);

core/src/main/java/edu/wpi/grip/core/operations/networktables/NTValue.java

@@ -17,9 +17,15 @@
  * One potential way of having multiple values without using annotations might be to make the interface method return
  * a list, but this would prevent us from knowing how many values there are and what their names are without having
  * an instance of the object being published.
+ * <p>
+ * The weight of each accessor must be specified.  This determines order of the inputs to {@link NTPublishOperation}.
+ * It's important to specify weights if there are multiple keys because otherwise, different JVMs will return them in
+ * different orders, leading to projects that are interpreted differently on different machines.
  */
 @Retention(RetentionPolicy.RUNTIME)
 @Target(ElementType.METHOD)
 public @interface NTValue {
     String key() default "";
+
+    int weight();
 }

core/src/main/java/edu/wpi/grip/core/operations/networktables/NTVector2D.java

@@ -24,12 +24,12 @@ public NTVector2D(Size size) {
         this.y = size.height();
     }
 
-    @NTValue(key = "x")
+    @NTValue(key = "x", weight = 0)
     public double getX() {
         return x;
     }
 
-    @NTValue(key = "y")
+    @NTValue(key = "y", weight = 1)
     public double getY() {
         return y;
     }

core/src/test/java/edu/wpi/grip/core/operations/networktables/NTPublishOperationTest.java

@@ -0,0 +1,64 @@
+package edu.wpi.grip.core.operations.networktables;
+
+import com.google.common.eventbus.EventBus;
+import edu.wpi.grip.core.InputSocket;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Tests for error handling in the publish operations' reflection stuff.
+ */
+public class NTPublishOperationTest {
+
+    private class Report implements NTPublishable {
+        @NTValue(key = "foo", weight = 2)
+        public double getFoo() {
+            return 0.0;
+        }
+
+        @NTValue(key = "bar", weight = 1)
+        public double getBar() {
+            return 1.0;
+        }
+    }
+
+    private class ReportWithNonDistinctWeights implements NTPublishable {
+        @NTValue(key = "foo", weight = 1)
+        public double getFoo() {
+            return 0.0;
+        }
+
+        @NTValue(key = "bar", weight = 1)
+        public double getBar() {
+            return 0.0;
+        }
+    }
+
+    private class ReportWithParameters implements NTPublishable {
+        @NTValue(key = "foo", weight = 0)
+        public double getFoo(boolean b) {
+            return b ? 1.0 : 0.0;
+        }
+    }
+
+    @Test
+    public void testNTValueOrder() {
+        NTPublishOperation<Report, Report> ntPublishOperation = new NTPublishOperation<>(Report.class);
+        InputSocket<?>[] sockets = ntPublishOperation.createInputSockets(new EventBus());
+
+        assertEquals(4, sockets.length);
+        assertEquals("Publish bar", sockets[2].getSocketHint().getIdentifier());
+        assertEquals("Publish foo", sockets[3].getSocketHint().getIdentifier());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testNonDistinctWeights() {
+        new NTPublishOperation<>(ReportWithNonDistinctWeights.class);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testInvalidParameters() {
+        new NTPublishOperation<>(ReportWithParameters.class);
+    }
+}