#2562 Introduce SceneGraphThreadWarden for thread safety in scene graph operations.

This update adds the SceneGraphThreadWarden to enforce that scene graph mutations occur only on the main thread IF those nodes are already part of the main scene graph

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,9 @@ public abstract class SimpleApplication extends LegacyApplication {
     public void initialize() {
         super.initialize();
 
+        SceneGraphThreadWarden.setup(rootNode);
+        SceneGraphThreadWarden.setup(guiNode);
+
         // Several things rely on having this
         guiFont = loadGuiFont();
 

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();

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;
@@ -612,6 +617,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 +632,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 +644,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 +780,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 +832,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 +860,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 +1016,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 +1381,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);
     }
 
     /**

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);
+    }
+}

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

@@ -0,0 +1,127 @@
+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;
+
+/**
+ * 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 {
+
+    /**
+     * 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 = true;
+
+    public static boolean ASSERTS_ENABLED = false;
+
+    static{
+        //noinspection AssertWithSideEffects
+        assert ASSERTS_ENABLED = true;
+    }
+
+    public static Thread mainThread;
+    public static final Set<Object> nodesThatAreMainThreadReserved = 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 void setup(Node rootNode){
+        if(checksDisabled()){
+            return;
+        }
+        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);
+    }
+
+    /**
+     * 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){
+        nodesThatAreMainThreadReserved.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){
+        nodesThatAreMainThreadReserved.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 && nodesThatAreMainThreadReserved.contains(newParent);
+        boolean wasPreviouslyRestricted = nodesThatAreMainThreadReserved.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 void reset(){
+        nodesThatAreMainThreadReserved.clear();
+        mainThread = null;
+    }
+
+    private static boolean checksDisabled(){
+       return !THREAD_WARDEN_ENABLED || !ASSERTS_ENABLED;
+    }
+
+    @SuppressWarnings("SameReturnValue")
+    public static boolean assertOnCorrectThread(Spatial spatial){
+        if(checksDisabled()){
+            return true;
+        }
+
+        if(Thread.currentThread() != mainThread){
+            throw new IllegalThreadSceneGraphMutation("The spatial " + spatial + " was mutated on a thread other than the main thread, was mutated on " + Thread.currentThread().getName());
+        }
+        return true; // return true so can be a "side effect" of an assert
+    }
+
+}
+