changeset 10881:fbaafb899805

8180151: JavaFX incorrectly renders scenegraph with two 3D boxes with certain dimensions Reviewed-by: kcr
author nlisker
date Thu, 22 Mar 2018 14:11:26 -0700
parents 3e6c9b6db694
children 3bc097d85c79
files modules/javafx.graphics/src/main/java/javafx/scene/shape/Box.java modules/javafx.graphics/src/main/java/javafx/scene/shape/Cylinder.java modules/javafx.graphics/src/main/java/javafx/scene/shape/PredefinedMeshManager.java modules/javafx.graphics/src/main/java/javafx/scene/shape/Shape3D.java modules/javafx.graphics/src/main/java/javafx/scene/shape/Sphere.java modules/javafx.graphics/src/shims/java/javafx/scene/shape/PredefinedMeshManagerShim.java modules/javafx.graphics/src/test/java/test/javafx/scene/shape/PredefinedMeshManagerTest.java
diffstat 7 files changed, 354 insertions(+), 52 deletions(-) [+]
line wrap: on
line diff
--- a/modules/javafx.graphics/src/main/java/javafx/scene/shape/Box.java	Tue Mar 20 13:01:33 2018 -0700
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/shape/Box.java	Thu Mar 22 14:11:26 2018 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -92,7 +92,7 @@
     public static final double DEFAULT_SIZE = 2;
 
     {
-        // To initialize the class helper at the begining each constructor of this class
+        // To initialize the class helper at the beginning of each constructor of this class
         BoxHelper.initHelper(this);
     }
 
@@ -135,7 +135,7 @@
                 public void invalidated() {
                     NodeHelper.markDirty(Box.this, DirtyBits.MESH_GEOM);
                     manager.invalidateBoxMesh(key);
-                    key = 0;
+                    key = null;
                     NodeHelper.geomChanged(Box.this);
                 }
             };
@@ -165,7 +165,7 @@
                 public void invalidated() {
                     NodeHelper.markDirty(Box.this, DirtyBits.MESH_GEOM);
                     manager.invalidateBoxMesh(key);
-                    key = 0;
+                    key = null;
                     NodeHelper.geomChanged(Box.this);
                 }
             };
@@ -195,7 +195,7 @@
                 public void invalidated() {
                     NodeHelper.markDirty(Box.this, DirtyBits.MESH_GEOM);
                     manager.invalidateBoxMesh(key);
-                    key = 0;
+                    key = null;
                     NodeHelper.geomChanged(Box.this);
                 }
             };
@@ -222,8 +222,8 @@
             if (w < 0 || h < 0 || d < 0) {
                 peer.updateMesh(null);
             } else {
-                if (key == 0) {
-                    key = generateKey(w, h, d);
+                if (key == null) {
+                    key = new BoxKey(w, h, d);
                 }
                 mesh = manager.getBoxMesh(w, h, d, key);
                 mesh.updatePG();
@@ -469,11 +469,47 @@
         return mesh;
     }
 
-    private static int generateKey(float w, float h, float d) {
-        int hash = 3;
-        hash = 97 * hash + Float.floatToIntBits(w);
-        hash = 97 * hash + Float.floatToIntBits(h);
-        hash = 97 * hash + Float.floatToIntBits(d);
-        return hash;
+    private static class BoxKey extends Key {
+
+        final double width, height, depth;
+
+        private BoxKey(double width, double height, double depth) {
+            this.width = width;
+            this.height = height;
+            this.depth = depth;
+        }
+
+        @Override
+        public int hashCode() {
+            long bits = 7L;
+            bits = 31L * bits + Double.doubleToLongBits(depth);
+            bits = 31L * bits + Double.doubleToLongBits(height);
+            bits = 31L * bits + Double.doubleToLongBits(width);
+            return Long.hashCode(bits);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            }
+            if (obj == null) {
+                return false;
+            }
+            if (!(obj instanceof BoxKey)) {
+                return false;
+            }
+            BoxKey other = (BoxKey) obj;
+            if (Double.compare(depth, other.depth) != 0) {
+                return false;
+            }
+            if (Double.compare(height, other.height) != 0) {
+                return false;
+            }
+            if (Double.compare(width, other.width) != 0) {
+                return false;
+            }
+            return true;
+        }
     }
 }
--- a/modules/javafx.graphics/src/main/java/javafx/scene/shape/Cylinder.java	Tue Mar 20 13:01:33 2018 -0700
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/shape/Cylinder.java	Thu Mar 22 14:11:26 2018 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -153,7 +153,7 @@
                 public void invalidated() {
                     NodeHelper.markDirty(Cylinder.this, DirtyBits.MESH_GEOM);
                     manager.invalidateCylinderMesh(key);
-                    key = 0;
+                    key = null;
                     NodeHelper.geomChanged(Cylinder.this);
                 }
             };
@@ -183,7 +183,7 @@
                 public void invalidated() {
                     NodeHelper.markDirty(Cylinder.this, DirtyBits.MESH_GEOM);
                     manager.invalidateCylinderMesh(key);
-                    key = 0;
+                    key = null;
                     NodeHelper.geomChanged(Cylinder.this);
                 }
             };
@@ -211,8 +211,8 @@
             if (h < 0 || r < 0) {
                 peer.updateMesh(null);
             } else {
-                if (key == 0) {
-                    key = generateKey(h, r, divisions);
+                if (key == null) {
+                    key = new CylinderKey(h, r, divisions);
                 }
                 mesh = manager.getCylinderMesh(h, r, divisions, key);
                 mesh.updatePG();
@@ -563,11 +563,48 @@
         return m;
     }
 
-    private static int generateKey(float h, float r, int div) {
-        int hash = 7;
-        hash = 47 * hash + Float.floatToIntBits(h);
-        hash = 47 * hash + Float.floatToIntBits(r);
-        hash = 47 * hash + div;
-        return hash;
+    private static class CylinderKey extends Key {
+
+        final double radius, height;
+        final int divisions;
+
+        private CylinderKey(double radius, double height, int divisions) {
+            this.radius = radius;
+            this.height = height;
+            this.divisions = divisions;
+        }
+
+        @Override
+        public int hashCode() {
+            long bits = 7L;
+            bits = 31L * bits + Double.doubleToLongBits(radius);
+            bits = 31L * bits + Double.doubleToLongBits(height);
+            bits = 31L * bits + divisions;
+            return Long.hashCode(bits);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            }
+            if (obj == null) {
+                return false;
+            }
+            if (!(obj instanceof CylinderKey)) {
+                return false;
+            }
+            CylinderKey other = (CylinderKey) obj;
+            if (divisions != other.divisions) {
+                return false;
+            }
+            if (Double.compare(radius, other.radius) != 0) {
+                return false;
+            }
+            if (Double.compare(height, other.height) != 0) {
+                return false;
+            }
+            return true;
+        }
     }
 }
--- a/modules/javafx.graphics/src/main/java/javafx/scene/shape/PredefinedMeshManager.java	Tue Mar 20 13:01:33 2018 -0700
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/shape/PredefinedMeshManager.java	Thu Mar 22 14:11:26 2018 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,9 @@
 package javafx.scene.shape;
 
 import java.util.HashMap;
+import java.util.Map;
+
+import javafx.scene.shape.Shape3D.Key;
 
 final class PredefinedMeshManager {
 
@@ -33,9 +36,9 @@
     private static final int INITAL_CAPACITY = 17; // TODO
     private static final float LOAD_FACTOR = 0.75f;
 
-    private HashMap<Integer, TriangleMesh> boxCache = null;
-    private HashMap<Integer, TriangleMesh> sphereCache = null;
-    private HashMap<Integer, TriangleMesh> cylinderCache = null;
+    private Map<Key, TriangleMesh> boxCache = null;
+    private Map<Key, TriangleMesh> sphereCache = null;
+    private Map<Key, TriangleMesh> cylinderCache = null;
 
     private PredefinedMeshManager() {}
 
@@ -43,7 +46,7 @@
         return INSTANCE;
     }
 
-    synchronized TriangleMesh getBoxMesh(float w, float h, float d, int key) {
+    synchronized TriangleMesh getBoxMesh(float w, float h, float d, Key key) {
         if (boxCache == null) {
             boxCache = BoxCacheLoader.INSTANCE;
         }
@@ -58,7 +61,7 @@
         return mesh;
     }
 
-    synchronized TriangleMesh getSphereMesh(float r, int div, int key) {
+    synchronized TriangleMesh getSphereMesh(float r, int div, Key key) {
         if (sphereCache == null) {
             sphereCache = SphereCacheLoader.INSTANCE;
         }
@@ -73,7 +76,7 @@
         return mesh;
     }
 
-    synchronized TriangleMesh getCylinderMesh(float h, float r, int div, int key) {
+    synchronized TriangleMesh getCylinderMesh(float h, float r, int div, Key key) {
         if (cylinderCache == null) {
             cylinderCache = CylinderCacheLoader.INSTANCE;
         }
@@ -88,7 +91,7 @@
         return mesh;
     }
 
-    synchronized void invalidateBoxMesh(int key) {
+    synchronized void invalidateBoxMesh(Key key) {
         if (boxCache != null) {
             TriangleMesh mesh = boxCache.get(key);
             if (mesh != null) {
@@ -101,7 +104,7 @@
         }
     }
 
-    synchronized void invalidateSphereMesh(int key) {
+    synchronized void invalidateSphereMesh(Key key) {
         if (sphereCache != null) {
             TriangleMesh mesh = sphereCache.get(key);
             if (mesh != null) {
@@ -114,7 +117,7 @@
         }
     }
 
-    synchronized void invalidateCylinderMesh(int key) {
+    synchronized void invalidateCylinderMesh(Key key) {
         if (cylinderCache != null) {
             TriangleMesh mesh = cylinderCache.get(key);
             if (mesh != null) {
@@ -155,25 +158,50 @@
         }
     }
 
+    /**
+     * Note: The only user of this method is in unit test: PredefinedMeshManagerTest.
+     */
+    void test_clearCaches() {
+        INSTANCE.dispose();
+    }
+
+    /**
+     * Note: The only user of this method is in unit test: PredefinedMeshManagerTest.
+     */
+    int test_getBoxCacheSize() {
+        return INSTANCE.boxCache.size();
+    }
+
+    /**
+     * Note: The only user of this method is in unit test: PredefinedMeshManagerTest.
+     */
+    int test_getSphereCacheSize() {
+        return INSTANCE.sphereCache.size();
+    }
+
+    /**
+     * Note: The only user of this method is in unit test: PredefinedMeshManagerTest.
+     */
+    int test_getCylinderCacheSize() {
+        return INSTANCE.cylinderCache.size();
+    }
+
     private final static class BoxCacheLoader {
 
         // lazy & thread-safe instantiation
-        private static final HashMap<Integer, TriangleMesh>
-                INSTANCE = new HashMap<Integer, TriangleMesh>(INITAL_CAPACITY, LOAD_FACTOR);
+        private static final Map<Key, TriangleMesh> INSTANCE = new HashMap<>(INITAL_CAPACITY, LOAD_FACTOR);
     }
 
     private final static class SphereCacheLoader {
 
         // lazy & thread-safe instantiation
-        private static final HashMap<Integer, TriangleMesh>
-                INSTANCE = new HashMap<Integer, TriangleMesh>(INITAL_CAPACITY, LOAD_FACTOR);
+        private static final Map<Key, TriangleMesh> INSTANCE = new HashMap<>(INITAL_CAPACITY, LOAD_FACTOR);
     }
 
     private final static class CylinderCacheLoader {
 
         // lazy & thread-safe instantiation
-        private static final HashMap<Integer, TriangleMesh>
-                INSTANCE = new HashMap<Integer, TriangleMesh>(INITAL_CAPACITY, LOAD_FACTOR);
+        private static final Map<Key, TriangleMesh> INSTANCE = new HashMap<>(INITAL_CAPACITY, LOAD_FACTOR);
     }
 
 };
--- a/modules/javafx.graphics/src/main/java/javafx/scene/shape/Shape3D.java	Tue Mar 20 13:01:33 2018 -0700
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/shape/Shape3D.java	Thu Mar 22 14:11:26 2018 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -38,7 +38,6 @@
 import javafx.beans.property.ObjectProperty;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.beans.value.ChangeListener;
-import javafx.beans.value.ObservableValue;
 import javafx.beans.value.WeakChangeListener;
 import javafx.scene.Node;
 import javafx.scene.paint.Material;
@@ -107,7 +106,20 @@
     }
 
     PredefinedMeshManager manager = PredefinedMeshManager.getInstance();
-    int key = 0;
+    Key key;
+
+    /**
+     * Used by the caching mechanism to compare between instances of the same shape.
+     * Each shape implements equals and hashCode using its base parameters.
+     */
+    abstract static class Key {
+
+        @Override
+        public abstract boolean equals(Object obj);
+
+        @Override
+        public abstract int hashCode();
+    }
 
     /**
      * Defines the material this {@code Shape3D}.
--- a/modules/javafx.graphics/src/main/java/javafx/scene/shape/Sphere.java	Tue Mar 20 13:01:33 2018 -0700
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/shape/Sphere.java	Thu Mar 22 14:11:26 2018 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -148,7 +148,7 @@
                 public void invalidated() {
                     NodeHelper.markDirty(Sphere.this, DirtyBits.MESH_GEOM);
                     manager.invalidateSphereMesh(key);
-                    key = 0;
+                    key = null;
                     NodeHelper.geomChanged(Sphere.this);
                 }
             };
@@ -182,8 +182,8 @@
             if (r < 0) {
                 pgSphere.updateMesh(null);
             } else {
-                if (key == 0) {
-                    key = generateKey(r, divisions);
+                if (key == null) {
+                    key = new SphereKey(r, divisions);
                 }
                 mesh = manager.getSphereMesh(r, divisions, key);
                 mesh.updatePG();
@@ -443,10 +443,43 @@
         return m;
     }
 
-    private static int generateKey(float r, int div) {
-        int hash = 5;
-        hash = 23 * hash + Float.floatToIntBits(r);
-        hash = 23 * hash + div;
-        return hash;
+    private static class SphereKey extends Key {
+
+        final double radius;
+        final int divisions;
+
+        private SphereKey(double radius, int divisions) {
+            this.radius = radius;
+            this.divisions = divisions;
+        }
+
+        @Override
+        public int hashCode() {
+            long bits = 7L;
+            bits = 31L * bits + Double.doubleToLongBits(radius);
+            bits = 31L * bits + divisions;
+            return Long.hashCode(bits);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            }
+            if (obj == null) {
+                return false;
+            }
+            if (!(obj instanceof SphereKey)) {
+                return false;
+            }
+            SphereKey other = (SphereKey) obj;
+            if (divisions != other.divisions) {
+                return false;
+            }
+            if (Double.compare(radius, other.radius) != 0) {
+                return false;
+            }
+            return true;
+        }
     }
 }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/modules/javafx.graphics/src/shims/java/javafx/scene/shape/PredefinedMeshManagerShim.java	Thu Mar 22 14:11:26 2018 -0700
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package javafx.scene.shape;
+
+public class PredefinedMeshManagerShim {
+
+    public static void clearCaches() {
+        PredefinedMeshManager.getInstance().test_clearCaches();
+    }
+
+    public static int getBoxCacheSize() {
+        return PredefinedMeshManager.getInstance().test_getBoxCacheSize();
+    }
+
+    public static int getSphereCacheSize() {
+        return PredefinedMeshManager.getInstance().test_getSphereCacheSize();
+    }
+
+    public static int getCylinderCacheSize() {
+        return PredefinedMeshManager.getInstance().test_getCylinderCacheSize();
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/modules/javafx.graphics/src/test/java/test/javafx/scene/shape/PredefinedMeshManagerTest.java	Thu Mar 22 14:11:26 2018 -0700
@@ -0,0 +1,111 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package test.javafx.scene.shape;
+
+import static org.junit.Assert.*;
+
+import javafx.scene.shape.Box;
+import javafx.scene.shape.Cylinder;
+import javafx.scene.shape.PredefinedMeshManagerShim;
+import javafx.scene.shape.Shape3D;
+import javafx.scene.shape.Sphere;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import com.sun.javafx.scene.NodeHelper;
+
+public class PredefinedMeshManagerTest {
+
+    @Before
+    public void clearCaches() {
+        PredefinedMeshManagerShim.clearCaches();
+    }
+
+    private void testShapeAddition(Shape3D shape, int correctSize) {
+        NodeHelper.updatePeer(shape);
+        int size = -1;
+        String name = null;
+        if (shape instanceof Box) {
+            size = PredefinedMeshManagerShim.getBoxCacheSize();
+            name = "box";
+        }
+        else if (shape instanceof Sphere) {
+            size = PredefinedMeshManagerShim.getSphereCacheSize();
+            name = "sphere";
+        }
+        else if (shape instanceof Cylinder) {
+            size = PredefinedMeshManagerShim.getCylinderCacheSize();
+            name = "cylinder";
+        }
+        assertEquals("Added a " + name + " - cache should contain " + correctSize + " mesh.", correctSize, size);
+    }
+
+    @Test
+    public void boxCacheTest() {
+        Box box1 = new Box(9, 1, 12);
+        testShapeAddition(box1 ,1);
+
+        // JDK-8180151: size will stay 1 without the fix (due to hash collision)
+        // new dimensions to cause a collision are any w/2, h*4, d/2
+        Box box2 = new Box(4.5, 4, 6);
+        testShapeAddition(box2 ,2);
+
+        Box box1again = new Box(9, 1, 12);
+        testShapeAddition(box1again, 2);
+    }
+
+    @Test
+    public void sphereCacheTest() {
+        Sphere sphere1 = new Sphere(10, 1000);
+        testShapeAddition(sphere1, 1);
+
+        // JDK-8180151: size will stay 1 without the fix (due to hash collision)
+        // From the old hash function:
+        // div2 = 23 * (Float.floatToIntBits(r1) - Float.floatToIntBits(r2)) + div1
+        Sphere sphere2 = new Sphere(9.99997, 1713);
+        testShapeAddition(sphere2, 2);
+
+        Sphere sphere1again = new Sphere(10, 1000);
+        testShapeAddition(sphere1again, 2);
+    }
+
+    @Test
+    public void cylinderCacheTest() {
+        Cylinder cylinder1 = new Cylinder(10, 20, 1000);
+        testShapeAddition(cylinder1, 1);
+
+        // JDK-8180151: size will stay 1 without the fix (due to hash collision)
+        // From the old hash function:
+        // div2 = 47*47 * (Float.floatToIntBits(h1) - Float.floatToIntBits(h2)) +
+        //           47 * (Float.floatToIntBits(r1) - Float.floatToIntBits(r2)) + div1;
+        Cylinder cylinder2 = new Cylinder(9.9997, 19.9997, 362618);
+        testShapeAddition(cylinder2, 2);
+
+        Cylinder cylinder1again = new Cylinder(10, 20, 1000);
+        testShapeAddition(cylinder1again, 2);
+    }
+}