This is an automated email from the ASF dual-hosted git repository.

desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git

commit 18b10ac7caf86b0401616b99fb38a7f0fc3c5a5c
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Wed Sep 16 19:53:59 2020 +0200

    Enable optimization in `TranslationTransform` which were missed.
---
 .../transform/AbstractLinearTransform.java         |  40 +++++---
 .../operation/transform/CopyTransform.java         | 112 +++++++++------------
 .../operation/transform/ProjectiveTransform.java   |   9 +-
 .../operation/transform/TranslationTransform.java  |  32 +++++-
 4 files changed, 112 insertions(+), 81 deletions(-)

diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/AbstractLinearTransform.java b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/AbstractLinearTransform.java
index 84335d7..1ca6a39 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/AbstractLinearTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/AbstractLinearTransform.java
@@ -44,7 +44,7 @@ import org.opengis.util.FactoryException;
  * </ul>
  *
  * @author  Martin Desruisseaux (Geomatys)
- * @version 1.0
+ * @version 1.1
  * @since   0.6
  * @module
  */
@@ -58,6 +58,8 @@ abstract class AbstractLinearTransform extends AbstractMathTransform implements
      * The inverse transform, or {@code null} if not yet created.
      * This field is part of the serialization form in order to avoid rounding errors if a user
      * asks for the inverse of the inverse (i.e. the original transform) after deserialization.
+     *
+     * @see #inverse()
      */
     volatile LinearTransform inverse;
 
@@ -126,7 +128,8 @@ abstract class AbstractLinearTransform extends AbstractMathTransform implements
     }
 
     /**
-     * Creates the inverse transform of this object.
+     * Returns the inverse transform of this object.
+     * This method invokes {@link #createInverse()} when first needed, then caches the result.
      */
     @Override
     @SuppressWarnings("DoubleCheckedLocking")                           // Okay since 'inverse' is volatile.
@@ -136,19 +139,7 @@ abstract class AbstractLinearTransform extends AbstractMathTransform implements
             synchronized (this) {
                 inv = inverse;
                 if (inv == null) {
-                    /*
-                     * Should never be the identity transform at this point (except during tests) because
-                     * MathTransforms.linear(…) should never instantiate this class in the identity case.
-                     * But we check anyway as a paranoiac safety.
-                     */
-                    if (isIdentity()) {
-                        inv = this;
-                    } else {
-                        inv = MathTransforms.linear(Matrices.inverse(this));
-                        if (inv instanceof AbstractLinearTransform) {
-                            ((AbstractLinearTransform) inv).inverse = this;
-                        }
-                    }
+                    inv = createInverse();
                     inverse = inv;
                 }
             }
@@ -157,6 +148,25 @@ abstract class AbstractLinearTransform extends AbstractMathTransform implements
     }
 
     /**
+     * Invoked by {@link #inverse()} the first time that the inverse transform needs to be computed.
+     */
+    LinearTransform createInverse() throws NoninvertibleTransformException {
+        /*
+         * Should never be the identity transform at this point (except during tests) because
+         * MathTransforms.linear(…) should never instantiate this class in the identity case.
+         * But we check anyway as a paranoiac safety.
+         */
+        if (isIdentity()) {
+            return this;
+        }
+        final LinearTransform inv = MathTransforms.linear(Matrices.inverse(this));
+        if (inv instanceof AbstractLinearTransform) {
+            ((AbstractLinearTransform) inv).inverse = this;
+        }
+        return inv;
+    }
+
+    /**
      * Returns the parameter descriptors for this math transform.
      *
      * @return {@inheritDoc}
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/CopyTransform.java b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/CopyTransform.java
index edf2ee7..c5c25e6 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/CopyTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/CopyTransform.java
@@ -19,7 +19,6 @@ package org.apache.sis.referencing.operation.transform;
 import java.util.Arrays;
 import org.opengis.geometry.DirectPosition;
 import org.opengis.referencing.operation.Matrix;
-import org.opengis.referencing.operation.NoninvertibleTransformException;
 import org.apache.sis.referencing.operation.matrix.MatrixSIS;
 import org.apache.sis.referencing.operation.matrix.Matrices;
 
@@ -36,7 +35,7 @@ import org.apache.sis.referencing.operation.matrix.Matrices;
  * an {@link java.awt.geom.AffineTransform} for every 2D affine conversions.</p>
  *
  * @author  Martin Desruisseaux (Geomatys)
- * @version 0.7
+ * @version 1.1
  * @since   0.5
  * @module
  */
@@ -326,72 +325,61 @@ final class CopyTransform extends AbstractLinearTransform {
     }
 
     /**
-     * Creates the inverse transform of this object.
+     * Invoked by {@link #inverse()} the first time that the inverse transform needs to be computed.
      */
     @Override
-    @SuppressWarnings("DoubleCheckedLocking")                                   // Okay since 'inverse' is volatile.
-    public LinearTransform inverse() throws NoninvertibleTransformException {
-        LinearTransform inv = inverse;
-        if (inv == null) {
-            synchronized (this) {
-                inv = inverse;
-                if (inv == null) {
-                    /*
-                     * Note: no need to perform the following check at this point because MathTransforms.linear(…)
-                     *       should never instantiate this class in the identity case and because we perform an
-                     *       equivalent check later anyway.
-                     *
-                     *       if (isIdentity()) {
-                     *           inverse = this;
-                     *       } else { ... }
-                     */
-                    final int srcDim = this.srcDim;
-                    final int dstDim = indices.length;
-                    final int[] reverse = new int[srcDim];
-                    Arrays.fill(reverse, -1);
-                    for (int i=dstDim; --i>=0;) {
-                        reverse[indices[i]] = i;
-                    }
-                    /*
-                     * Check if there is any unassigned dimension. In such case,
-                     * delegates to the generic ProjectiveTransform with a matrix
-                     * which set the missing values to NaN.
-                     */
-                    for (int j=srcDim; --j>=0;) {
-                        if (reverse[j] < 0) {
-                            final MatrixSIS matrix = Matrices.createZero(srcDim + 1, dstDim + 1);
-                            for (j=0; j<srcDim; j++) {      // Okay to reuse 'j' since the outer loop will not continue.
-                                final int i = reverse[j];
-                                if (i >= 0) {
-                                    matrix.setElement(j, i, 1);
-                                } else {
-                                    matrix.setElement(j, dstDim, Double.NaN);
-                                }
-                            }
-                            matrix.setElement(srcDim, dstDim, 1);
-                            inv = MathTransforms.linear(matrix);
-                            if (inv instanceof AbstractLinearTransform) {
-                                ((AbstractLinearTransform) inv).inverse = this;
-                            }
-                            inverse = inv;
-                            return inv;
-                        }
-                    }
-                    /*
-                     * At this point, we know that we can create the inverse transform.
-                     * If this transform is the identity transform or an anti-diagonal matrix except last row
-                     * (e.g. matrix used for swapping axis order), then the old and new arrays would be equal.
-                     */
-                    CopyTransform copyInverse = this;
-                    if (!Arrays.equals(reverse, indices)) {
-                        copyInverse = new CopyTransform(indices.length, reverse);
-                        copyInverse.inverse = this;
+    final LinearTransform createInverse() {
+        /*
+         * Note: no need to perform the following check at this point because MathTransforms.linear(…)
+         *       should never instantiate this class in the identity case and because we perform an
+         *       equivalent check later anyway.
+         *
+         *       if (isIdentity()) {
+         *           inverse = this;
+         *       } else { ... }
+         */
+        final int srcDim = this.srcDim;
+        final int dstDim = indices.length;
+        final int[] reverse = new int[srcDim];
+        Arrays.fill(reverse, -1);
+        for (int i=dstDim; --i>=0;) {
+            reverse[indices[i]] = i;
+        }
+        /*
+         * Check if there is any unassigned dimension. In such case,
+         * delegates to the generic ProjectiveTransform with a matrix
+         * which set the missing values to NaN.
+         */
+        for (int j=srcDim; --j>=0;) {
+            if (reverse[j] < 0) {
+                final MatrixSIS matrix = Matrices.createZero(srcDim + 1, dstDim + 1);
+                for (j=0; j<srcDim; j++) {      // Okay to reuse `j` since the outer loop will not continue.
+                    final int i = reverse[j];
+                    if (i >= 0) {
+                        matrix.setElement(j, i, 1);
+                    } else {
+                        matrix.setElement(j, dstDim, Double.NaN);
                     }
-                    inverse = inv = copyInverse;
                 }
+                matrix.setElement(srcDim, dstDim, 1);
+                final LinearTransform inv = MathTransforms.linear(matrix);
+                if (inv instanceof AbstractLinearTransform) {
+                    ((AbstractLinearTransform) inv).inverse = this;
+                }
+                return inv;
             }
         }
-        return inv;
+        /*
+         * At this point, we know that we can create the inverse transform.
+         * If this transform is the identity transform or an anti-diagonal matrix except last row
+         * (e.g. matrix used for swapping axis order), then the old and new arrays would be equal.
+         */
+        CopyTransform copyInverse = this;
+        if (!Arrays.equals(reverse, indices)) {
+            copyInverse = new CopyTransform(indices.length, reverse);
+            copyInverse.inverse = this;
+        }
+        return copyInverse;
     }
 
     /**
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java
index 7516f1f..5dc40ff 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java
@@ -113,11 +113,14 @@ class ProjectiveTransform extends AbstractLinearTransform implements ExtendedPre
          */
         boolean isScale       = true;                       // ScaleTransform accepts non-square matrix.
         boolean isTranslation = (numRow == numCol);         // TranslationTransform is restricted to square matrix.
+        final int lastColumn  = numCol - 1;
         for (int i=0; i<n; i++) {
-            if (elt[i] != 0) {
+            final double v = elt[i];
+            if (v != 0) {
+                final int row  = (i / numCol);
                 final int col  = (i % numCol);
-                isScale       &= (i / numCol) == col;
-                isTranslation &= (col == numCol - 1);
+                isScale       &= row == col;
+                isTranslation &= (col == lastColumn) || (isScale && v == 1);
                 if (!(isScale | isTranslation)) {
                     return this;
                 }
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/TranslationTransform.java b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/TranslationTransform.java
index 2071d61..b149a0d 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/TranslationTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/TranslationTransform.java
@@ -32,7 +32,7 @@ import org.apache.sis.util.ArraysExt;
  * {@link org.apache.sis.internal.referencing.j2d.AffineTransform2D} should be used in such case.</div>
  *
  * @author  Martin Desruisseaux (Geomatys)
- * @version 1.0
+ * @version 1.1
  *
  * @see <a href="http://issues.apache.org/jira/browse/SIS-176">SIS-176</a>
  *
@@ -75,6 +75,28 @@ final class TranslationTransform extends AbstractLinearTransform implements Exte
     }
 
     /**
+     * Creates a transform as the inverse of the given transform.
+     */
+    private TranslationTransform(final TranslationTransform other) {
+        offsets = negate(other.offsets);
+        errors  = negate(other.errors);
+        inverse = other;
+    }
+
+    /**
+     * Returns a new array with negative values of given array (can be {@code null}).
+     */
+    private static double[] negate(double[] array) {
+        if (array != null) {
+            array = array.clone();
+            for (int i=0; i<array.length; i++) {
+                array[i] = -array[i];
+            }
+        }
+        return array;
+    }
+
+    /**
      * Constructs a translation transform from a matrix having the given elements.
      * This constructors assumes that the matrix is square, affine and contains only
      * translation terms (this is not verified).
@@ -296,6 +318,14 @@ final class TranslationTransform extends AbstractLinearTransform implements Exte
     }
 
     /**
+     * Invoked by {@link #inverse()} the first time that the inverse transform needs to be computed.
+     */
+    @Override
+    final LinearTransform createInverse() {
+        return new TranslationTransform(this);
+    }
+
+    /**
      * {@inheritDoc}
      *
      * @return {@inheritDoc}