Browse Source

Merge pull request #2563 from richardTingle/#2562-thread-warden

#2562 thread warden
Ryan McDonough 2 weeks ago
parent
commit
e20665ec28

+ 13 - 0
jme3-core/src/main/java/com/jme3/app/SimpleApplication.java

@@ -45,6 +45,7 @@ import com.jme3.renderer.RenderManager;
 import com.jme3.renderer.queue.RenderQueue.Bucket;
 import com.jme3.scene.Node;
 import com.jme3.scene.Spatial.CullHint;
+import com.jme3.scene.threadwarden.SceneGraphThreadWarden;
 import com.jme3.system.AppSettings;
 import com.jme3.system.JmeContext.Type;
 import com.jme3.system.JmeSystem;
@@ -197,6 +198,11 @@ public abstract class SimpleApplication extends LegacyApplication {
     public void initialize() {
         super.initialize();
 
+        //noinspection AssertWithSideEffects
+        assert SceneGraphThreadWarden.setup(rootNode);
+        //noinspection AssertWithSideEffects
+        assert SceneGraphThreadWarden.setup(guiNode);
+
         // Several things rely on having this
         guiFont = loadGuiFont();
 
@@ -240,6 +246,13 @@ public abstract class SimpleApplication extends LegacyApplication {
         simpleInitApp();
     }
 
+    @Override
+    public void stop(boolean waitFor) {
+        //noinspection AssertWithSideEffects
+        assert SceneGraphThreadWarden.reset();
+        super.stop(waitFor);
+    }
+
     @Override
     public void update() {
         if (prof != null) {

+ 4 - 0
jme3-core/src/main/java/com/jme3/scene/Geometry.java

@@ -44,6 +44,7 @@ import com.jme3.math.Matrix4f;
 import com.jme3.renderer.Camera;
 import com.jme3.scene.VertexBuffer.Type;
 import com.jme3.scene.mesh.MorphTarget;
+import com.jme3.scene.threadwarden.SceneGraphThreadWarden;
 import com.jme3.util.TempVars;
 import com.jme3.util.clone.Cloner;
 import com.jme3.util.clone.IdentityCloneFunction;
@@ -183,6 +184,7 @@ public class Geometry extends Spatial {
      */
     @Override
     public void setLodLevel(int lod) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (mesh.getNumLodLevels() == 0) {
             throw new IllegalStateException("LOD levels are not set on this mesh");
         }
@@ -239,6 +241,7 @@ public class Geometry extends Spatial {
      * @throws IllegalArgumentException If mesh is null
      */
     public void setMesh(Mesh mesh) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (mesh == null) {
             throw new IllegalArgumentException();
         }
@@ -269,6 +272,7 @@ public class Geometry extends Spatial {
      */
     @Override
     public void setMaterial(Material material) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         this.material = material;
         nbSimultaneousGPUMorph = -1;
         if (isGrouped()) {

+ 9 - 0
jme3-core/src/main/java/com/jme3/scene/Node.java

@@ -38,6 +38,7 @@ import com.jme3.collision.CollisionResults;
 import com.jme3.export.JmeExporter;
 import com.jme3.export.JmeImporter;
 import com.jme3.material.Material;
+import com.jme3.scene.threadwarden.SceneGraphThreadWarden;
 import com.jme3.util.SafeArrayList;
 import com.jme3.util.clone.Cloner;
 import java.io.IOException;
@@ -201,6 +202,7 @@ public class Node extends Spatial {
      *  that would change state.
      */
     void invalidateUpdateList() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         updateListValid = false;
         if (parent != null) {
             parent.invalidateUpdateList();
@@ -344,6 +346,7 @@ public class Node extends Spatial {
      * @throws IllegalArgumentException if child is null or this
      */
     public int attachChildAt(Spatial child, int index) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (child == null) {
             throw new IllegalArgumentException("child cannot be null");
         }
@@ -428,6 +431,7 @@ public class Node extends Spatial {
      * @return the child at the supplied index.
      */
     public Spatial detachChildAt(int index) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         Spatial child = children.remove(index);
         if (child != null) {
             child.setParent(null);
@@ -455,6 +459,7 @@ public class Node extends Spatial {
      * node.
      */
     public void detachAllChildren() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         // Note: this could be a bit more efficient if it delegated
         // to a private method that avoided setBoundRefresh(), etc.
         // for every child and instead did one in here at the end.
@@ -483,6 +488,7 @@ public class Node extends Spatial {
      * @param index2 The index of the second child to swap
      */
     public void swapChildren(int index1, int index2) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         Spatial c2 = children.get(index2);
         Spatial c1 = children.remove(index1);
         children.add(index1, c2);
@@ -562,6 +568,7 @@ public class Node extends Spatial {
 
     @Override
     public void setMaterial(Material mat) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         for (int i = 0; i < children.size(); i++) {
             children.get(i).setMaterial(mat);
         }
@@ -778,6 +785,7 @@ public class Node extends Spatial {
 
     @Override
     public void setModelBound(BoundingVolume modelBound) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (children != null) {
             for (Spatial child : children.getArray()) {
                 child.setModelBound(modelBound != null ? modelBound.clone(null) : null);
@@ -787,6 +795,7 @@ public class Node extends Spatial {
 
     @Override
     public void updateModelBound() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (children != null) {
             for (Spatial child : children.getArray()) {
                 child.updateModelBound();

+ 15 - 1
jme3-core/src/main/java/com/jme3/scene/Spatial.java

@@ -49,6 +49,7 @@ import com.jme3.renderer.queue.RenderQueue;
 import com.jme3.renderer.queue.RenderQueue.Bucket;
 import com.jme3.renderer.queue.RenderQueue.ShadowMode;
 import com.jme3.scene.control.Control;
+import com.jme3.scene.threadwarden.SceneGraphThreadWarden;
 import com.jme3.util.SafeArrayList;
 import com.jme3.util.TempVars;
 import com.jme3.util.clone.Cloner;
@@ -278,11 +279,13 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * a refresh is required.
      */
     protected void setTransformRefresh() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         refreshFlags |= RF_TRANSFORM;
         setBoundRefresh();
     }
 
     protected void setLightListRefresh() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         refreshFlags |= RF_LIGHTLIST;
         // Make sure next updateGeometricState() visits this branch
         // to update lights.
@@ -299,6 +302,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
     }
 
     protected void setMatParamOverrideRefresh() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         refreshFlags |= RF_MATPARAM_OVERRIDE;
         Spatial p = parent;
         while (p != null) {
@@ -316,6 +320,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * a refresh is required.
      */
     protected void setBoundRefresh() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         refreshFlags |= RF_BOUND;
 
         Spatial p = parent;
@@ -364,7 +369,8 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
             throw new IllegalStateException("Scene graph is not properly updated for rendering.\n"
                     + "State was changed after rootNode.updateGeometricState() call. \n"
                     + "Make sure you do not modify the scene from another thread!\n"
-                    + "Problem spatial name: " + getName());
+                    + "Problem spatial name: " + getName() + "\n" +
+                    SceneGraphThreadWarden.getTurnOnAssertsPrompt());
         }
 
         CullHint cm = getCullHint();
@@ -612,6 +618,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @see MatParamOverride
      */
     public void addMatParamOverride(MatParamOverride override) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (override == null) {
             throw new IllegalArgumentException("override cannot be null");
         }
@@ -626,6 +633,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @see MatParamOverride
      */
     public void removeMatParamOverride(MatParamOverride override) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (localOverrides.remove(override)) {
             setMatParamOverrideRefresh();
         }
@@ -637,6 +645,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @see #addMatParamOverride(com.jme3.material.MatParamOverride)
      */
     public void clearMatParamOverrides() {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         if (!localOverrides.isEmpty()) {
             setMatParamOverrideRefresh();
         }
@@ -772,6 +781,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @see Spatial#removeControl(java.lang.Class)
      */
     public void addControl(Control control) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         boolean before = requiresUpdates();
         controls.add(control);
         control.setSpatial(this);
@@ -823,6 +833,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @see Spatial#addControl(com.jme3.scene.control.Control)
      */
     public void removeControl(Class<? extends Control> controlType) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         boolean before = requiresUpdates();
         for (int i = 0; i < controls.size(); i++) {
             if (controlType.isAssignableFrom(controls.get(i).getClass())) {
@@ -850,6 +861,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @see Spatial#addControl(com.jme3.scene.control.Control)
      */
     public boolean removeControl(Control control) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
         boolean before = requiresUpdates();
         boolean result = controls.remove(control);
         if (result) {
@@ -1005,6 +1017,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      *            the parent of this node.
      */
     protected void setParent(Node parent) {
+        assert SceneGraphThreadWarden.updateRequirement(this, parent);
         this.parent = parent;
     }
 
@@ -1369,6 +1382,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable,
      * @param lod The lod level to set.
      */
     public void setLodLevel(int lod) {
+        assert SceneGraphThreadWarden.assertOnCorrectThread(this);
     }
 
     /**

+ 7 - 0
jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java

@@ -0,0 +1,7 @@
+package com.jme3.scene.threadwarden;
+
+public class IllegalThreadSceneGraphMutation extends IllegalStateException{
+    public IllegalThreadSceneGraphMutation(String message){
+        super(message);
+    }
+}

+ 160 - 0
jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java

@@ -0,0 +1,160 @@
+package com.jme3.scene.threadwarden;
+
+import com.jme3.scene.Node;
+import com.jme3.scene.Spatial;
+
+import java.util.Collections;
+import java.util.Set;
+import java.util.WeakHashMap;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+/**
+ * Thread warden keeps track of mutations to the scene graph and ensures that they are only done on the main thread.
+ * IF the parent node is marked as being reserved for the main thread (which basically means it's connected to the
+ * root node)
+ * <p>
+ *     Only has an effect if asserts are on
+ * </p>
+ */
+public class SceneGraphThreadWarden {
+
+    private static final Logger logger = Logger.getLogger(SceneGraphThreadWarden.class.getName());
+    
+    /**
+     * If THREAD_WARDEN_ENABLED is true AND asserts are on the checks are made.
+     * This parameter is here to allow asserts to run without thread warden checks (by setting this parameter to false)
+     */
+    public static boolean THREAD_WARDEN_ENABLED = !Boolean.getBoolean("nothreadwarden");
+
+    public static boolean ASSERTS_ENABLED = false;
+
+    static{
+        //noinspection AssertWithSideEffects
+        assert ASSERTS_ENABLED = true;
+    }
+
+    public static Thread mainThread;
+    public static final Set<Spatial> spatialsThatAreMainThreadReserved = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>()));
+
+    /**
+     * Marks the given node as being reserved for the main thread.
+     * Additionally, sets the current thread as the main thread (if it hasn't already been set)
+     * @param rootNode the root node of the scene graph. This is used to determine if a spatial is a child of the root node.
+     *                 (Can add multiple "root" nodes, e.g. gui nodes or overlay nodes)
+     */
+    public static boolean setup(Node rootNode){
+        if(checksDisabled()){
+            return true;
+        }
+        Thread thisThread = Thread.currentThread();
+        if(mainThread != null && mainThread != thisThread ){
+            throw new IllegalStateException("The main thread has already been set to " + mainThread.getName() + " but now it's being set to " + Thread.currentThread().getName());
+        }
+        mainThread = thisThread;
+        setTreeRestricted(rootNode);
+
+        return true; // return true so can be a "side effect" of an assert
+    }
+
+    /**
+     * Disables the thread warden checks (even when other asserts are on).
+     * <p>
+     * Alternatively can be disabled by adding the -Dnothreadwarden=true parameter
+     * </p>
+     */
+    public static void disableChecks(){
+        THREAD_WARDEN_ENABLED = false;
+    }
+
+    /**
+     * Runs through the entire tree and sets the restriction state of all nodes below the given node
+     * @param spatial the node (and children) to set the restriction state of
+     */
+    private static void setTreeRestricted(Spatial spatial){
+        spatialsThatAreMainThreadReserved.add(spatial);
+        if(spatial instanceof Node){
+            for(Spatial child : ((Node) spatial).getChildren()){
+                setTreeRestricted(child);
+            }
+        }
+    }
+
+    /**
+     * Releases this tree from being only allowed to be mutated on the main thread
+     * @param spatial the node (and children) to release the restriction state of.
+     */
+    private static void setTreeNotRestricted(Spatial spatial){
+        spatialsThatAreMainThreadReserved.remove(spatial);
+        if(spatial instanceof Node){
+            for(Spatial child : ((Node) spatial).getChildren()){
+                setTreeNotRestricted(child);
+            }
+        }
+    }
+
+    @SuppressWarnings("SameReturnValue")
+    public static boolean updateRequirement(Spatial spatial, Node newParent){
+        if(checksDisabled()){
+            return true;
+        }
+
+        boolean shouldNowBeRestricted = newParent !=null && spatialsThatAreMainThreadReserved.contains(newParent);
+        boolean wasPreviouslyRestricted = spatialsThatAreMainThreadReserved.contains(spatial);
+
+        if(shouldNowBeRestricted || wasPreviouslyRestricted ){
+            assertOnCorrectThread(spatial);
+        }
+
+        if(shouldNowBeRestricted == wasPreviouslyRestricted){
+            return true;
+        }
+        if(shouldNowBeRestricted){
+            setTreeRestricted(spatial);
+        }else{
+            setTreeNotRestricted(spatial);
+        }
+
+        return true; // return true so can be a "side effect" of an assert
+    }
+
+    public static boolean reset(){
+        spatialsThatAreMainThreadReserved.clear();
+        mainThread = null;
+        THREAD_WARDEN_ENABLED = !Boolean.getBoolean("nothreadwarden");
+        return true; // return true so can be a "side effect" of an assert
+    }
+
+    private static boolean checksDisabled(){
+       return !THREAD_WARDEN_ENABLED || !ASSERTS_ENABLED;
+    }
+
+    @SuppressWarnings("SameReturnValue")
+    public static boolean assertOnCorrectThread(Spatial spatial){
+        if(checksDisabled()){
+            return true;
+        }
+        if(spatialsThatAreMainThreadReserved.contains(spatial)){
+            if(Thread.currentThread() != mainThread){
+                // log as well as throw an exception because we are running in a thread, if we are in an executor service the exception
+                // might not make itself known until `get` is called on the future (and JME might crash before that happens).
+                String message = "The spatial " + spatial + " was mutated on a thread other than the main thread, was mutated on " + Thread.currentThread().getName();
+                IllegalThreadSceneGraphMutation ex = new IllegalThreadSceneGraphMutation(message);
+                logger.log(Level.WARNING, message, ex);
+
+                throw ex;
+            }
+        }
+        return true; // return true so can be a "side effect" of an assert
+    }
+
+    public static String getTurnOnAssertsPrompt(){
+        if(ASSERTS_ENABLED){
+            return "";
+        } else{
+            return "To get more accurate debug consider turning on asserts. This will allow JME to do additional checks which *may* find the source of the problem. To do so, add -ea to the JVM arguments.";
+        }
+    }
+
+}
+

+ 207 - 0
jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java

@@ -0,0 +1,207 @@
+package com.jme3.scene.threadwarden;
+
+import com.jme3.material.Material;
+import com.jme3.scene.Geometry;
+import com.jme3.scene.Mesh;
+import com.jme3.scene.Node;
+import com.jme3.scene.shape.Box;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadFactory;
+import java.util.function.Consumer;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * Parameterized tests for SceneGraphThreadWarden class with Geometry objects.
+ * These tests verify that various scene graph mutations are properly checked for thread safety.
+ */
+@RunWith(Parameterized.class)
+public class SceneGraphThreadWardenGeometryExtendedTest {
+
+    private static ExecutorService executorService;
+
+    private final String testName;
+    private final Consumer<Geometry> action;
+
+    @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"})
+    @BeforeClass
+    public static void setupClass() {
+        // Make sure assertions are enabled
+        boolean assertsEnabled = false;
+        assert assertsEnabled = true;
+        if (!assertsEnabled) {
+            throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly.");
+        }
+    }
+
+    @Before
+    public void setup() {
+        executorService = newSingleThreadDaemonExecutor();
+    }
+
+    @After
+    public void tearDown() {
+        executorService.shutdown();
+        SceneGraphThreadWarden.reset();
+    }
+
+    /**
+     * Constructor for the parameterized test.
+     * 
+     * @param testName A descriptive name for the test
+     * @param action The action to perform on the spatial
+     */
+    public SceneGraphThreadWardenGeometryExtendedTest(String testName, Consumer<Geometry> action) {
+        this.testName = testName;
+        this.action = action;
+    }
+
+    /**
+     * Define the parameters for the test.
+     * Each parameter is a pair of (test name, action to perform on spatial).
+     */
+    @Parameterized.Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+        Material mockMaterial = Mockito.mock(Material.class);
+        Box box = new Box(1, 1, 1);
+
+        return Arrays.asList(new Object[][] {
+            { 
+                "setMaterial", 
+                (Consumer<Geometry>) spatial -> spatial.setMaterial(mockMaterial)
+            },
+            { 
+                "setMesh", 
+                (Consumer<Geometry>) spatial -> spatial.setMesh(box)
+            },
+            { 
+                "setLodLevel", 
+                (Consumer<Geometry>) spatial -> {
+                    // Need to set a mesh with LOD levels first
+                    Mesh mesh = new Box(1, 1, 1);
+                    mesh.setLodLevels(new com.jme3.scene.VertexBuffer[]{
+                        mesh.getBuffer(com.jme3.scene.VertexBuffer.Type.Index)
+                    });
+                    spatial.setMesh(mesh);
+                    spatial.setLodLevel(0);
+                }
+            },
+            { 
+                "removeFromParent", 
+                (Consumer<Geometry>) Geometry::removeFromParent
+            }
+        });
+    }
+
+    /**
+     * Test that scene graph mutation is fine on the main thread when the object is attached to the root.
+     */
+    @Test
+    public void testMutationOnMainThreadOnAttachedObject() {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a geometry and attach it to the root node
+        Geometry geometry = new Geometry("geometry", new Box(1, 1, 1));
+        rootNode.attachChild(geometry);
+
+        // This should work fine since we're on the main thread
+        action.accept(geometry);
+    }
+
+    /**
+     * Test that scene graph mutation is fine on the main thread when the object is not attached to the root.
+     */
+    @Test
+    public void testMutationOnMainThreadOnDetachedObject() {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a geometry but don't attach it to the root node
+        Geometry geometry = new Geometry("geometry", new Box(1, 1, 1));
+
+        // This should work fine since we're on the main thread
+        action.accept(geometry);
+    }
+
+    /**
+     * Test that scene graph mutation is fine on a non-main thread when the object is not attached to the root.
+     */
+    @Test
+    public void testMutationOnNonMainThreadOnDetachedObject() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a geometry but don't attach it to the root node
+        Geometry geometry = new Geometry("geometry", new Box(1, 1, 1));
+
+        Future<Void> future = executorService.submit(() -> {
+            // This should work fine since the geometry is not connected to the root node
+            action.accept(geometry);
+            return null;
+        });
+
+        // This should complete without exceptions
+        future.get();
+    }
+
+    /**
+     * Test that scene graph mutation is not allowed on a non-main thread when the object is attached to the root.
+     */
+    @Test
+    public void testMutationOnNonMainThreadOnAttachedObject() throws InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a geometry and attach it to the root node
+        Geometry geometry = new Geometry("geometry", new Box(1, 1, 1));
+        rootNode.attachChild(geometry);
+
+        Future<Void> future = executorService.submit(() -> {
+            // This should fail because we're trying to modify a geometry that's connected to the scene graph
+            action.accept(geometry);
+            return null;
+        });
+
+        try {
+            future.get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
+        }
+    }
+
+    /**
+     * Creates a single-threaded executor service with daemon threads.
+     */
+    private static ExecutorService newSingleThreadDaemonExecutor() {
+        return Executors.newSingleThreadExecutor(daemonThreadFactory());
+    }
+
+    /**
+     * Creates a thread factory that produces daemon threads.
+     */
+    private static ThreadFactory daemonThreadFactory() {
+        return r -> {
+            Thread t = Executors.defaultThreadFactory().newThread(r);
+            t.setDaemon(true);
+            return t;
+        };
+    }
+}

+ 203 - 0
jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java

@@ -0,0 +1,203 @@
+package com.jme3.scene.threadwarden;
+
+import com.jme3.material.Material;
+import com.jme3.material.MatParamOverride;
+import com.jme3.scene.Node;
+import com.jme3.scene.Spatial;
+import com.jme3.shader.VarType;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadFactory;
+import java.util.function.Consumer;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * Parameterized tests for SceneGraphThreadWarden class.
+ * These tests verify that various scene graph mutations are properly checked for thread safety.
+ */
+@RunWith(Parameterized.class)
+public class SceneGraphThreadWardenNodeExtendedTest {
+
+    private static ExecutorService executorService;
+
+    private final String testName;
+    private final Consumer<Node> action;
+
+    @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"})
+    @BeforeClass
+    public static void setupClass() {
+        // Make sure assertions are enabled
+        boolean assertsEnabled = false;
+        assert assertsEnabled = true;
+        if (!assertsEnabled) {
+            throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly.");
+        }
+    }
+
+    @Before
+    public void setup() {
+        executorService = newSingleThreadDaemonExecutor();
+    }
+
+    @After
+    public void tearDown() {
+        executorService.shutdown();
+        SceneGraphThreadWarden.reset();
+    }
+
+    /**
+     * Constructor for the parameterized test.
+     * 
+     * @param testName A descriptive name for the test
+     * @param action The action to perform on the spatial
+     */
+    public SceneGraphThreadWardenNodeExtendedTest(String testName, Consumer<Node> action) {
+        this.testName = testName;
+        this.action = action;
+    }
+
+    /**
+     * Define the parameters for the test.
+     * Each parameter is a pair of (test name, action to perform on spatial).
+     */
+    @Parameterized.Parameters(name = "{0}")
+    public static Collection<Object[]> data() {
+        Material mockMaterial = Mockito.mock(Material.class);
+        MatParamOverride override = new MatParamOverride(VarType.Float, "TestParam", 1.0f);
+
+        return Arrays.asList(new Object[][] {
+            { 
+                "setMaterial", 
+                (Consumer<Node>) spatial -> spatial.setMaterial(mockMaterial)
+            },
+            { 
+                "setLodLevel", 
+                (Consumer<Node>) spatial -> spatial.setLodLevel(1)
+            },
+            { 
+                "addMatParamOverride", 
+                (Consumer<Node>) spatial -> spatial.addMatParamOverride(override)
+            },
+            { 
+                "removeMatParamOverride", 
+                (Consumer<Node>) spatial -> spatial.removeMatParamOverride(override)
+            },
+            { 
+                "clearMatParamOverrides", 
+                (Consumer<Node>) Spatial::clearMatParamOverrides
+            }
+        });
+    }
+
+    /**
+     * Test that scene graph mutation is fine on the main thread when the object is attached to the root.
+     */
+    @Test
+    public void testMutationOnMainThreadOnAttachedObject() {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and attach it to the root node
+        Node child = new Node("child");
+        rootNode.attachChild(child);
+
+        // This should work fine since we're on the main thread
+        action.accept(child);
+    }
+
+    /**
+     * Test that scene graph mutation is fine on the main thread when the object is not attached to the root.
+     */
+    @Test
+    public void testMutationOnMainThreadOnDetachedObject() {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node but don't attach it to the root node
+        Node child = new Node("child");
+
+        // This should work fine since we're on the main thread
+        action.accept(child);
+    }
+
+    /**
+     * Test that scene graph mutation is fine on a non-main thread when the object is not attached to the root.
+     */
+    @Test
+    public void testMutationOnNonMainThreadOnDetachedObject() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node but don't attach it to the root node
+        Node child = new Node("child");
+
+        Future<Void> future = executorService.submit(() -> {
+            // This should work fine since the node is not connected to the root node
+            action.accept(child);
+            return null;
+        });
+
+        // This should complete without exceptions
+        future.get();
+    }
+
+    /**
+     * Test that scene graph mutation is not allowed on a non-main thread when the object is attached to the root.
+     */
+    @Test
+    public void testMutationOnNonMainThreadOnAttachedObject() throws InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and attach it to the root node
+        Node child = new Node("child");
+        rootNode.attachChild(child);
+
+        Future<Void> future = executorService.submit(() -> {
+            // This should fail because we're trying to modify a node that's connected to the scene graph
+            action.accept(child);
+            return null;
+        });
+
+        try {
+            future.get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
+        }
+    }
+
+    /**
+     * Creates a single-threaded executor service with daemon threads.
+     */
+    private static ExecutorService newSingleThreadDaemonExecutor() {
+        return Executors.newSingleThreadExecutor(daemonThreadFactory());
+    }
+
+    /**
+     * Creates a thread factory that produces daemon threads.
+     */
+    private static ThreadFactory daemonThreadFactory() {
+        return r -> {
+            Thread t = Executors.defaultThreadFactory().newThread(r);
+            t.setDaemon(true);
+            return t;
+        };
+    }
+}

+ 316 - 0
jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java

@@ -0,0 +1,316 @@
+package com.jme3.scene.threadwarden;
+
+import com.jme3.scene.Node;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadFactory;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+/**
+ * Tests for SceneGraphThreadWarden class.
+ * These tests verify that:
+ * - Normal node mutation is fine on the main thread
+ * - Node mutation on nodes not connected to the root node is fine even on a non main thread
+ * - Adding a node to the scene graph (indirectly) connected to the root node isn't fine on a non main thread
+ * - Adding a node currently attached to a root node to a different node isn't fine on a non main thread
+ */
+public class SceneGraphThreadWardenTest {
+
+    private static ExecutorService executorService;
+
+    @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"})
+    @BeforeClass
+    public static void setupClass() {
+        // Make sure assertions are enabled
+        boolean assertsEnabled = false;
+        assert assertsEnabled = true;
+        //noinspection ConstantValue
+        if (!assertsEnabled) {
+            throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly.");
+        }
+    }
+
+    @Before
+    public void setup() {
+        executorService = newSingleThreadDaemonExecutor();
+    }
+
+    @After
+    public void tearDown() {
+        executorService.shutdown();
+        SceneGraphThreadWarden.reset();
+    }
+
+    /**
+     * Test that normal node mutation is fine on the main thread.
+     */
+    @Test
+    public void testNormalNodeMutationOnMainThread() {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // This should work fine since we're on the main thread
+        Node child = new Node("child");
+        rootNode.attachChild(child);
+
+        // Add another level of children
+        Node grandchild = new Node("grandchild");
+        child.attachChild(grandchild);
+
+        // Detach should also work fine
+        child.detachChild(grandchild);
+        rootNode.detachChild(child);
+    }
+
+    /**
+     * Test that node mutation on nodes not connected to the root node is fine even on a non main thread.
+     * <p>
+     *     This is a use case where a thread is preparing things for later attachment to the scene graph.
+     * </p>
+     */
+    @Test
+    public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        Future<Node> nonConnectedNodeFuture = executorService.submit(() -> {
+            // This should work fine since these nodes are not connected to the root node
+            Node parent = new Node("parent");
+            Node child = new Node("child");
+            parent.attachChild(child);
+
+            // Add another level of children
+            Node grandchild = new Node("grandchild");
+            child.attachChild(grandchild);
+
+            return parent;
+        });
+
+        // Get the result to ensure the task completed without exceptions
+        Node nonConnectedNode = nonConnectedNodeFuture.get();
+
+        // Now we can attach it to the root node on the main thread
+        rootNode.attachChild(nonConnectedNode);
+    }
+
+    /**
+     * Test that adding a node to the scene graph connected to the root node in a non main thread leads to an
+     * exception.
+     */
+    @Test
+    public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and attach it to the root node
+        Node child = new Node("child");
+        rootNode.attachChild(child);
+
+        Future<Void> illegalMutationFuture = executorService.submit(() -> {
+            // This should fail because we're trying to add a node to a node that's connected to the scene graph
+            Node grandchild = new Node("grandchild");
+            child.attachChild(grandchild);
+            return null;
+        });
+
+        try {
+            illegalMutationFuture.get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
+        }
+    }
+
+    /**
+     * Test that adding a node currently attached to a root node to a different node leads to an exception.
+     * <p>
+     *     This is testing an edge case where you think you'd working with non-connected nodes, but in reality
+     *     one of your nodes is already attached to the scene graph (and you're attaching it to a different node which will
+     *     detach it from the scene graph).
+     * </p>
+     */
+    @Test
+    public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create two child nodes and attach them to the root node
+        Node child1 = new Node("child1");
+        Node child2 = new Node("child2");
+
+        rootNode.attachChild(child2);
+
+        Future<Void> illegalMutationFuture = executorService.submit(() -> {
+            // This should fail because we're trying to move a node that's connected to the root node
+            child1.attachChild(child2); // This implicitly detaches child2 from rootNode
+            return null;
+        });
+
+        try {
+            illegalMutationFuture.get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
+        }
+    }
+
+    /**
+     * Test that detaching a node releases it from thread protection.
+     */
+    @Test
+    public void testDetachmentReleasesProtection() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and attach it to the root node
+        Node child = new Node("child");
+        rootNode.attachChild(child);
+
+        // Now detach it from the root node
+        child.removeFromParent();
+
+        // Now we should be able to modify it on another thread
+        Future<Void> legalMutationFuture = executorService.submit(() -> {
+            Node grandchild = new Node("grandchild");
+            child.attachChild(grandchild);
+            return null;
+        });
+
+        // This should complete without exceptions
+        legalMutationFuture.get();
+    }
+
+    /**
+     * Test that adding a child to the root node also restricts the grandchild.
+     * This test will add a grandchild to a child BEFORE adding the child to the root,
+     * then try (and fail) to make an illegal on-thread change to the grandchild.
+     */
+    @Test
+    public void testAddingAChildToTheRootNodeAlsoRestrictsTheGrandChild() throws InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and a grandchild node
+        Node child = new Node("child");
+        Node grandchild = new Node("grandchild");
+
+        // Attach the grandchild to the child BEFORE adding the child to the root
+        child.attachChild(grandchild);
+
+        // Now attach the child to the root node
+        rootNode.attachChild(child);
+
+        // Try to make an illegal on-thread change to the grandchild
+        Future<Void> illegalMutationFuture = executorService.submit(() -> {
+            // This should fail because the grandchild is now restricted
+            Node greatGrandchild = new Node("greatGrandchild");
+            grandchild.attachChild(greatGrandchild);
+            return null;
+        });
+
+        try {
+            illegalMutationFuture.get();
+            fail("Expected an IllegalThreadSceneGraphMutation exception");
+        } catch (ExecutionException e) {
+            // This is expected - verify it's the right exception type
+            assertTrue("Expected IllegalThreadSceneGraphMutation, got: " + e.getCause().getClass().getName(),
+                    e.getCause() instanceof IllegalThreadSceneGraphMutation);
+        }
+    }
+
+    /**
+     * Test that removing a child from the root node also unrestricts the grandchild.
+     * This test will add a child with a grandchild to the root node, then remove the child
+     * and verify that the grandchild can be modified on a non-main thread.
+     */
+    @Test
+    public void testRemovingAChildFromTheRootNodeAlsoUnrestrictsTheGrandChild() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and a grandchild node
+        Node child = new Node("child");
+        Node grandchild = new Node("grandchild");
+
+        // Attach the grandchild to the child
+        child.attachChild(grandchild);
+
+        // Attach the child to the root node
+        rootNode.attachChild(child);
+
+        // Now remove the child from the root node
+        child.removeFromParent();
+
+        // Try to make a change to the grandchild on a non-main thread
+        Future<Void> legalMutationFuture = executorService.submit(() -> {
+            // This should succeed because the grandchild is no longer restricted
+            Node greatGrandchild = new Node("greatGrandchild");
+            grandchild.attachChild(greatGrandchild);
+            return null;
+        });
+
+        // This should complete without exceptions
+        legalMutationFuture.get();
+    }
+
+    /**
+     * Test that an otherwise illegal scene graph mutation won't throw an exception
+     * if the checks have been disabled by calling disableChecks().
+     */
+    @Test
+    public void testDisableChecksAllowsIllegalMutation() throws ExecutionException, InterruptedException {
+        Node rootNode = new Node("root");
+        SceneGraphThreadWarden.setup(rootNode);
+
+        // Create a child node and attach it to the root node
+        Node child = new Node("child");
+        rootNode.attachChild(child);
+
+        // Disable the thread warden checks
+        SceneGraphThreadWarden.disableChecks();
+
+        // Try to make a change to the child on a non-main thread
+        // This would normally be illegal, but should succeed because checks are disabled
+        Future<Void> mutationFuture = executorService.submit(() -> {
+            Node grandchild = new Node("grandchild");
+            child.attachChild(grandchild);
+            return null;
+        });
+
+        // This should complete without exceptions
+        mutationFuture.get();
+    }
+
+
+
+    /**
+     * Creates a single-threaded executor service with daemon threads.
+     */
+    private static ExecutorService newSingleThreadDaemonExecutor() {
+        return Executors.newSingleThreadExecutor(daemonThreadFactory());
+    }
+
+    /**
+     * Creates a thread factory that produces daemon threads.
+     */
+    private static ThreadFactory daemonThreadFactory() {
+        return r -> {
+            Thread t = Executors.defaultThreadFactory().newThread(r);
+            t.setDaemon(true);
+            return t;
+        };
+    }
+}