From 61361014a5bfe0c6cf911ca9610882d813dbf605 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Thu, 25 Sep 2025 17:50:17 +0100 Subject: [PATCH 01/12] #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 --- .../java/com/jme3/app/SimpleApplication.java | 4 + .../src/main/java/com/jme3/scene/Node.java | 9 ++ .../src/main/java/com/jme3/scene/Spatial.java | 13 ++ .../IllegalThreadSceneGraphMutation.java | 7 + .../threadwarden/SceneGraphThreadWarden.java | 127 ++++++++++++++++++ 5 files changed, 160 insertions(+) create mode 100644 jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java create mode 100644 jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java diff --git a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java index f1f21b58cc..7c27007e4d 100644 --- a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java +++ b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java @@ -45,6 +45,7 @@ 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 @@ protected BitmapFont loadGuiFont() { public void initialize() { super.initialize(); + SceneGraphThreadWarden.setup(rootNode); + SceneGraphThreadWarden.setup(guiNode); + // Several things rely on having this guiFont = loadGuiFont(); diff --git a/jme3-core/src/main/java/com/jme3/scene/Node.java b/jme3-core/src/main/java/com/jme3/scene/Node.java index 0424cea053..46bb0829e8 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Node.java +++ b/jme3-core/src/main/java/com/jme3/scene/Node.java @@ -38,6 +38,7 @@ 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 @@ private void addUpdateChildren(SafeArrayList results) { * that would change state. */ void invalidateUpdateList() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); updateListValid = false; if (parent != null) { parent.invalidateUpdateList(); @@ -344,6 +346,7 @@ public int attachChild(Spatial child) { * @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 int detachChildNamed(String childName) { * @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 Spatial detachChildAt(int index) { * 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 int getChildIndex(Spatial sp) { * @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 List getChildren() { @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 void read(JmeImporter importer) throws IOException { @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 void setModelBound(BoundingVolume modelBound) { @Override public void updateModelBound() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (children != null) { for (Spatial child : children.getArray()) { child.updateModelBound(); diff --git a/jme3-core/src/main/java/com/jme3/scene/Spatial.java b/jme3-core/src/main/java/com/jme3/scene/Spatial.java index 2fe1775d3e..790cb13f58 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Spatial.java +++ b/jme3-core/src/main/java/com/jme3/scene/Spatial.java @@ -49,6 +49,7 @@ 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 @@ protected void setRequiresUpdates(boolean f) { * 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 @@ protected void setLightListRefresh() { } protected void setMatParamOverrideRefresh() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); refreshFlags |= RF_MATPARAM_OVERRIDE; Spatial p = parent; while (p != null) { @@ -316,6 +320,7 @@ protected void setMatParamOverrideRefresh() { * a refresh is required. */ protected void setBoundRefresh() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); refreshFlags |= RF_BOUND; Spatial p = parent; @@ -612,6 +617,7 @@ protected void updateMatParamOverrides() { * @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 void addMatParamOverride(MatParamOverride override) { * @see MatParamOverride */ public void removeMatParamOverride(MatParamOverride override) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (localOverrides.remove(override)) { setMatParamOverrideRefresh(); } @@ -637,6 +644,7 @@ public void removeMatParamOverride(MatParamOverride override) { * @see #addMatParamOverride(com.jme3.material.MatParamOverride) */ public void clearMatParamOverrides() { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (!localOverrides.isEmpty()) { setMatParamOverrideRefresh(); } @@ -772,6 +780,7 @@ public void runControlRender(RenderManager rm, ViewPort vp) { * @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 void addControlAt(int index, Control control) { * @see Spatial#addControl(com.jme3.scene.control.Control) */ public void removeControl(Class 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 void removeControl(Class controlType) { * @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 Node getParent() { * the parent of this node. */ protected void setParent(Node parent) { + assert SceneGraphThreadWarden.updateRequirement(this, parent); this.parent = parent; } @@ -1369,6 +1381,7 @@ public RenderQueue.ShadowMode getShadowMode() { * @param lod The lod level to set. */ public void setLodLevel(int lod) { + assert SceneGraphThreadWarden.assertOnCorrectThread(this); } /** diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/IllegalThreadSceneGraphMutation.java new file mode 100644 index 0000000000..64fe4bee8a --- /dev/null +++ b/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); + } +} diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java new file mode 100644 index 0000000000..4c111f71d5 --- /dev/null +++ b/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) + *

+ * Only has an effect if asserts are on + *

+ */ +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 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 + } + +} + From fca8209af8ca34e77a51f7d1cb87ce57724a096e Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Thu, 25 Sep 2025 17:57:43 +0100 Subject: [PATCH 02/12] #2562 Add tests for SceneGraphThreadWarden and fix bug where all nodes were restricted --- .../threadwarden/SceneGraphThreadWarden.java | 7 +- .../SceneGraphThreadWardenTest.java | 206 ++++++++++++++++++ 2 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index 4c111f71d5..27c56bf12f 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -116,9 +116,10 @@ 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()); + if(nodesThatAreMainThreadReserved.contains(spatial)){ + 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 } diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java new file mode 100644 index 0000000000..568119bd7b --- /dev/null +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -0,0 +1,206 @@ +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.assertFalse; +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; + + @BeforeClass + public static void setupClass() { + // Make sure assertions are enabled + boolean assertsEnabled = false; + assert assertsEnabled = true; + if (!assertsEnabled) { + System.err.println("WARNING: Assertions are not enabled! Tests may not work correctly."); + } + + // Ensure thread warden is enabled + SceneGraphThreadWarden.THREAD_WARDEN_ENABLED = true; + } + + @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. + */ + @Test + public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws ExecutionException, InterruptedException { + Node rootNode = new Node("root"); + SceneGraphThreadWarden.setup(rootNode); + + Future 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 isn't fine on a non main thread. + */ + @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 illegalMutationFuture = executorService.submit(() -> { + // This should fail because we're trying to add a node to a node that's connected to the root node + 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 isn't fine on a non main thread. + */ + @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(child1); + rootNode.attachChild(child2); + + Future 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 legalMutationFuture = executorService.submit(() -> { + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + return null; + }); + + // This should complete without exceptions + legalMutationFuture.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; + }; + } +} \ No newline at end of file From 2fcec028b410fec4c6d765bc5088a1679de19d3d Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Thu, 25 Sep 2025 18:07:16 +0100 Subject: [PATCH 03/12] #2562 Tidy up the test --- .../SceneGraphThreadWardenTest.java | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java index 568119bd7b..bde94435e5 100644 --- a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -28,17 +28,15 @@ 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; if (!assertsEnabled) { - System.err.println("WARNING: Assertions are not enabled! Tests may not work correctly."); + throw new RuntimeException("WARNING: Assertions are not enabled! Tests may not work correctly."); } - - // Ensure thread warden is enabled - SceneGraphThreadWarden.THREAD_WARDEN_ENABLED = true; } @Before @@ -51,7 +49,7 @@ public void tearDown() { executorService.shutdown(); SceneGraphThreadWarden.reset(); } - + /** * Test that normal node mutation is fine on the main thread. */ @@ -75,6 +73,9 @@ public void testNormalNodeMutationOnMainThread() { /** * Test that node mutation on nodes not connected to the root node is fine even on a non main thread. + *

+ * This is a use case where a thread is preparing things for later attachment to the scene graph. + *

*/ @Test public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws ExecutionException, InterruptedException { @@ -102,7 +103,8 @@ public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws Executio } /** - * Test that adding a node to the scene graph connected to the root node isn't fine on a non main thread. + * 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 { @@ -114,7 +116,7 @@ public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedExcept rootNode.attachChild(child); Future illegalMutationFuture = executorService.submit(() -> { - // This should fail because we're trying to add a node to a node that's connected to the root node + // 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; @@ -131,7 +133,12 @@ public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedExcept } /** - * Test that adding a node currently attached to a root node to a different node isn't fine on a non main thread. + * Test that adding a node currently attached to a root node to a different node leads to an exception. + *

+ * 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). + *

*/ @Test public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedException { @@ -141,7 +148,7 @@ public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedExce // Create two child nodes and attach them to the root node Node child1 = new Node("child1"); Node child2 = new Node("child2"); - rootNode.attachChild(child1); + rootNode.attachChild(child2); Future illegalMutationFuture = executorService.submit(() -> { @@ -185,7 +192,9 @@ public void testDetachmentReleasesProtection() throws ExecutionException, Interr // This should complete without exceptions legalMutationFuture.get(); } - + + + /** * Creates a single-threaded executor service with daemon threads. */ From 82466eda58bd67f07a1c2ea26e46c9f119f8e408 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Thu, 25 Sep 2025 18:40:32 +0100 Subject: [PATCH 04/12] #2562 Add parameterized tests for SceneGraphThreadWarden Introduce tests to verify thread safety of scene graph mutations under various conditions. Includes checks for both main-thread and non-main-thread scenarios with attached and detached objects. Ensures compliance with scene graph thread safety guarantees. --- .../SceneGraphThreadWardenExtendedTest.java | 203 ++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.java diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.java new file mode 100644 index 0000000000..c527ea11c9 --- /dev/null +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.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 SceneGraphThreadWardenExtendedTest { + + private static ExecutorService executorService; + + private final String testName; + private final Consumer 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 SceneGraphThreadWardenExtendedTest(String testName, Consumer 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 data() { + Material mockMaterial = Mockito.mock(Material.class); + MatParamOverride override = new MatParamOverride(VarType.Float, "TestParam", 1.0f); + + return Arrays.asList(new Object[][] { + { + "setMaterial", + (Consumer) spatial -> spatial.setMaterial(mockMaterial) + }, + { + "setLodLevel", + (Consumer) spatial -> spatial.setLodLevel(1) + }, + { + "addMatParamOverride", + (Consumer) spatial -> spatial.addMatParamOverride(override) + }, + { + "removeMatParamOverride", + (Consumer) spatial -> spatial.removeMatParamOverride(override) + }, + { + "clearMatParamOverrides", + (Consumer) 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 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 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; + }; + } +} From 43d8d44e99b3ed6b905043788492825ee6095864 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Thu, 25 Sep 2025 18:48:40 +0100 Subject: [PATCH 05/12] #2562 Add tests for SceneGraphThreadWarden's nested node handling Added tests to validate thread safety in scenarios involving nested nodes, such as attaching/detaching children and grandchildren. Ensures proper restriction and release of thread protections for hierarchical structures. This strengthens coverage for complex scene graph operations. --- .../SceneGraphThreadWardenTest.java | 127 ++++++++++++++---- 1 file changed, 100 insertions(+), 27 deletions(-) diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java index bde94435e5..1ac36c2ebf 100644 --- a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -27,7 +27,7 @@ public class SceneGraphThreadWardenTest { private static ExecutorService executorService; - + @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"}) @BeforeClass public static void setupClass() { @@ -38,12 +38,12 @@ public static void setupClass() { 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(); @@ -57,20 +57,20 @@ public void tearDown() { 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. *

@@ -81,27 +81,27 @@ public void testNormalNodeMutationOnMainThread() { public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws ExecutionException, InterruptedException { Node rootNode = new Node("root"); SceneGraphThreadWarden.setup(rootNode); - + Future 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. @@ -110,18 +110,18 @@ public void testNodeMutationOnNonConnectedNodesOnNonMainThread() throws Executio 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 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"); @@ -131,7 +131,7 @@ public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedExcept e.getCause() instanceof IllegalThreadSceneGraphMutation); } } - + /** * Test that adding a node currently attached to a root node to a different node leads to an exception. *

@@ -144,19 +144,19 @@ public void testAddingNodeToSceneGraphOnNonMainThread() throws InterruptedExcept 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 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"); @@ -166,7 +166,7 @@ public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedExce e.getCause() instanceof IllegalThreadSceneGraphMutation); } } - + /** * Test that detaching a node releases it from thread protection. */ @@ -174,21 +174,94 @@ public void testMovingNodeAttachedToRootOnNonMainThread() throws InterruptedExce 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 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 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 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(); } @@ -201,7 +274,7 @@ public void testDetachmentReleasesProtection() throws ExecutionException, Interr private static ExecutorService newSingleThreadDaemonExecutor() { return Executors.newSingleThreadExecutor(daemonThreadFactory()); } - + /** * Creates a thread factory that produces daemon threads. */ @@ -212,4 +285,4 @@ private static ThreadFactory daemonThreadFactory() { return t; }; } -} \ No newline at end of file +} From f4569c105dd4ecb450d28b8aeb5baa27ee247818 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Fri, 26 Sep 2025 12:41:09 +0100 Subject: [PATCH 06/12] #2562 Add geometry scene graph thread asserts --- .../main/java/com/jme3/scene/Geometry.java | 4 + ...GraphThreadWardenGeometryExtendedTest.java | 207 ++++++++++++++++++ ...eneGraphThreadWardenNodeExtendedTest.java} | 16 +- 3 files changed, 219 insertions(+), 8 deletions(-) create mode 100644 jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java rename jme3-core/src/test/java/com/jme3/scene/threadwarden/{SceneGraphThreadWardenExtendedTest.java => SceneGraphThreadWardenNodeExtendedTest.java} (91%) diff --git a/jme3-core/src/main/java/com/jme3/scene/Geometry.java b/jme3-core/src/main/java/com/jme3/scene/Geometry.java index 008e987f1d..e673823380 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Geometry.java +++ b/jme3-core/src/main/java/com/jme3/scene/Geometry.java @@ -44,6 +44,7 @@ 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 void setIgnoreTransform(boolean ignoreTransform) { */ @Override public void setLodLevel(int lod) { + SceneGraphThreadWarden.assertOnCorrectThread(this); if (mesh.getNumLodLevels() == 0) { throw new IllegalStateException("LOD levels are not set on this mesh"); } @@ -239,6 +241,7 @@ public int getTriangleCount() { * @throws IllegalArgumentException If mesh is null */ public void setMesh(Mesh mesh) { + SceneGraphThreadWarden.assertOnCorrectThread(this); if (mesh == null) { throw new IllegalArgumentException(); } @@ -269,6 +272,7 @@ public Mesh getMesh() { */ @Override public void setMaterial(Material material) { + SceneGraphThreadWarden.assertOnCorrectThread(this); this.material = material; nbSimultaneousGPUMorph = -1; if (isGrouped()) { diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenGeometryExtendedTest.java new file mode 100644 index 0000000000..3b264a4177 --- /dev/null +++ b/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 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 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 data() { + Material mockMaterial = Mockito.mock(Material.class); + Box box = new Box(1, 1, 1); + + return Arrays.asList(new Object[][] { + { + "setMaterial", + (Consumer) spatial -> spatial.setMaterial(mockMaterial) + }, + { + "setMesh", + (Consumer) spatial -> spatial.setMesh(box) + }, + { + "setLodLevel", + (Consumer) 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::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 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 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; + }; + } +} \ No newline at end of file diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java similarity index 91% rename from jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.java rename to jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java index c527ea11c9..7c36f07bcd 100644 --- a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenExtendedTest.java +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenNodeExtendedTest.java @@ -30,12 +30,12 @@ * These tests verify that various scene graph mutations are properly checked for thread safety. */ @RunWith(Parameterized.class) -public class SceneGraphThreadWardenExtendedTest { +public class SceneGraphThreadWardenNodeExtendedTest { private static ExecutorService executorService; private final String testName; - private final Consumer action; + private final Consumer action; @SuppressWarnings({"ReassignedVariable", "AssertWithSideEffects"}) @BeforeClass @@ -65,7 +65,7 @@ public void tearDown() { * @param testName A descriptive name for the test * @param action The action to perform on the spatial */ - public SceneGraphThreadWardenExtendedTest(String testName, Consumer action) { + public SceneGraphThreadWardenNodeExtendedTest(String testName, Consumer action) { this.testName = testName; this.action = action; } @@ -82,23 +82,23 @@ public static Collection data() { return Arrays.asList(new Object[][] { { "setMaterial", - (Consumer) spatial -> spatial.setMaterial(mockMaterial) + (Consumer) spatial -> spatial.setMaterial(mockMaterial) }, { "setLodLevel", - (Consumer) spatial -> spatial.setLodLevel(1) + (Consumer) spatial -> spatial.setLodLevel(1) }, { "addMatParamOverride", - (Consumer) spatial -> spatial.addMatParamOverride(override) + (Consumer) spatial -> spatial.addMatParamOverride(override) }, { "removeMatParamOverride", - (Consumer) spatial -> spatial.removeMatParamOverride(override) + (Consumer) spatial -> spatial.removeMatParamOverride(override) }, { "clearMatParamOverrides", - (Consumer) Spatial::clearMatParamOverrides + (Consumer) Spatial::clearMatParamOverrides } }); } From b0b24771b812ccf089a3e37caf6c56b81f4c3ade Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Fri, 26 Sep 2025 12:45:42 +0100 Subject: [PATCH 07/12] #2562 Introduced a `disableChecks` method and added support for disabling the thread warden via a system property. --- .../threadwarden/SceneGraphThreadWarden.java | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index 27c56bf12f..f5b7e118ca 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -21,7 +21,7 @@ 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 THREAD_WARDEN_ENABLED = !Boolean.getBoolean("nothreadwarden"); public static boolean ASSERTS_ENABLED = false; @@ -31,7 +31,7 @@ public class SceneGraphThreadWarden { } public static Thread mainThread; - public static final Set nodesThatAreMainThreadReserved = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>())); + public static final Set spatialsThatAreMainThreadReserved = Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<>())); /** * Marks the given node as being reserved for the main thread. @@ -51,12 +51,22 @@ public static void setup(Node rootNode){ setTreeRestricted(rootNode); } + /** + * Disables the thread warden checks (even when other asserts are on). + *

+ * Alternatively can be disabled by adding the -Dnothreadwarden=true parameter + *

+ */ + 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){ - nodesThatAreMainThreadReserved.add(spatial); + spatialsThatAreMainThreadReserved.add(spatial); if(spatial instanceof Node){ for(Spatial child : ((Node) spatial).getChildren()){ setTreeRestricted(child); @@ -69,7 +79,7 @@ private static void setTreeRestricted(Spatial spatial){ * @param spatial the node (and children) to release the restriction state of. */ private static void setTreeNotRestricted(Spatial spatial){ - nodesThatAreMainThreadReserved.remove(spatial); + spatialsThatAreMainThreadReserved.remove(spatial); if(spatial instanceof Node){ for(Spatial child : ((Node) spatial).getChildren()){ setTreeNotRestricted(child); @@ -83,8 +93,8 @@ public static boolean updateRequirement(Spatial spatial, Node newParent){ return true; } - boolean shouldNowBeRestricted = newParent !=null && nodesThatAreMainThreadReserved.contains(newParent); - boolean wasPreviouslyRestricted = nodesThatAreMainThreadReserved.contains(spatial); + boolean shouldNowBeRestricted = newParent !=null && spatialsThatAreMainThreadReserved.contains(newParent); + boolean wasPreviouslyRestricted = spatialsThatAreMainThreadReserved.contains(spatial); if(shouldNowBeRestricted || wasPreviouslyRestricted ){ assertOnCorrectThread(spatial); @@ -103,7 +113,7 @@ public static boolean updateRequirement(Spatial spatial, Node newParent){ } public static void reset(){ - nodesThatAreMainThreadReserved.clear(); + spatialsThatAreMainThreadReserved.clear(); mainThread = null; } @@ -116,7 +126,7 @@ public static boolean assertOnCorrectThread(Spatial spatial){ if(checksDisabled()){ return true; } - if(nodesThatAreMainThreadReserved.contains(spatial)){ + if(spatialsThatAreMainThreadReserved.contains(spatial)){ 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()); } From 2a0a746c90a92c04d31d97cb5fdda8e4ee4e4a02 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Fri, 26 Sep 2025 12:51:50 +0100 Subject: [PATCH 08/12] #2562 IAdd tes for disabled thread checks --- .../threadwarden/SceneGraphThreadWarden.java | 1 + .../SceneGraphThreadWardenTest.java | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index f5b7e118ca..73a955a6be 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -115,6 +115,7 @@ public static boolean updateRequirement(Spatial spatial, Node newParent){ public static void reset(){ spatialsThatAreMainThreadReserved.clear(); mainThread = null; + THREAD_WARDEN_ENABLED = !Boolean.getBoolean("nothreadwarden"); } private static boolean checksDisabled(){ diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java index 1ac36c2ebf..76c6d2658c 100644 --- a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -12,7 +12,6 @@ import java.util.concurrent.Future; import java.util.concurrent.ThreadFactory; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -266,6 +265,34 @@ public void testRemovingAChildFromTheRootNodeAlsoUnrestrictsTheGrandChild() thro 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 mutationFuture = executorService.submit(() -> { + Node grandchild = new Node("grandchild"); + child.attachChild(grandchild); + return null; + }); + + // This should complete without exceptions + mutationFuture.get(); + } + /** From a7643e7bc32ac56c5fbbf4a91fc882ecad11eda3 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Sat, 27 Sep 2025 10:48:44 +0100 Subject: [PATCH 09/12] #2562 Prompt users to turn on asserts (if they haven't already) --- jme3-core/src/main/java/com/jme3/scene/Spatial.java | 3 ++- .../jme3/scene/threadwarden/SceneGraphThreadWarden.java | 8 ++++++++ .../scene/threadwarden/SceneGraphThreadWardenTest.java | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/jme3-core/src/main/java/com/jme3/scene/Spatial.java b/jme3-core/src/main/java/com/jme3/scene/Spatial.java index 790cb13f58..7da3efaa5e 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Spatial.java +++ b/jme3-core/src/main/java/com/jme3/scene/Spatial.java @@ -369,7 +369,8 @@ public boolean checkCulling(Camera cam) { 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(); diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index 73a955a6be..8a68db54b4 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -135,5 +135,13 @@ public static boolean assertOnCorrectThread(Spatial spatial){ 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."; + } + } + } diff --git a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java index 76c6d2658c..7c92b2edd6 100644 --- a/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java +++ b/jme3-core/src/test/java/com/jme3/scene/threadwarden/SceneGraphThreadWardenTest.java @@ -33,6 +33,7 @@ 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."); } From 2f7cf65288378c7fd079863b49afed5efca3fd51 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Sat, 27 Sep 2025 11:07:46 +0100 Subject: [PATCH 10/12] #2562 Reset thread warden on shutdown --- jme3-core/src/main/java/com/jme3/app/SimpleApplication.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java index 7c27007e4d..e073391f96 100644 --- a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java +++ b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java @@ -244,6 +244,12 @@ public void initialize() { simpleInitApp(); } + @Override + public void stop(boolean waitFor) { + SceneGraphThreadWarden.reset(); + super.stop(waitFor); + } + @Override public void update() { if (prof != null) { From 87c94e6304649a68ee2970328015404708742912 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Sat, 27 Sep 2025 11:55:34 +0100 Subject: [PATCH 11/12] #2562 Log as well as exception (in case an executor service is being used where futures aren't promptly collected) --- .../scene/threadwarden/SceneGraphThreadWarden.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index 8a68db54b4..99a1a1d24e 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -6,6 +6,8 @@ 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. @@ -17,6 +19,8 @@ */ 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) @@ -129,7 +133,13 @@ public static boolean assertOnCorrectThread(Spatial spatial){ } if(spatialsThatAreMainThreadReserved.contains(spatial)){ 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()); + // 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 From 7b0fe87623a22adf3aa45f33e8cff4bf87a8a9e6 Mon Sep 17 00:00:00 2001 From: Richard Tingle <6330028+richardTingle@users.noreply.github.com> Date: Sat, 27 Sep 2025 12:54:07 +0100 Subject: [PATCH 12/12] #2562 Ensure all thread warden calls are asserts so can all be optised out --- .../src/main/java/com/jme3/app/SimpleApplication.java | 9 ++++++--- jme3-core/src/main/java/com/jme3/scene/Geometry.java | 6 +++--- .../jme3/scene/threadwarden/SceneGraphThreadWarden.java | 9 ++++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java index e073391f96..7fe97af210 100644 --- a/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java +++ b/jme3-core/src/main/java/com/jme3/app/SimpleApplication.java @@ -198,8 +198,10 @@ protected BitmapFont loadGuiFont() { public void initialize() { super.initialize(); - SceneGraphThreadWarden.setup(rootNode); - SceneGraphThreadWarden.setup(guiNode); + //noinspection AssertWithSideEffects + assert SceneGraphThreadWarden.setup(rootNode); + //noinspection AssertWithSideEffects + assert SceneGraphThreadWarden.setup(guiNode); // Several things rely on having this guiFont = loadGuiFont(); @@ -246,7 +248,8 @@ public void initialize() { @Override public void stop(boolean waitFor) { - SceneGraphThreadWarden.reset(); + //noinspection AssertWithSideEffects + assert SceneGraphThreadWarden.reset(); super.stop(waitFor); } diff --git a/jme3-core/src/main/java/com/jme3/scene/Geometry.java b/jme3-core/src/main/java/com/jme3/scene/Geometry.java index e673823380..dd3da84c9e 100644 --- a/jme3-core/src/main/java/com/jme3/scene/Geometry.java +++ b/jme3-core/src/main/java/com/jme3/scene/Geometry.java @@ -184,7 +184,7 @@ public void setIgnoreTransform(boolean ignoreTransform) { */ @Override public void setLodLevel(int lod) { - SceneGraphThreadWarden.assertOnCorrectThread(this); + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (mesh.getNumLodLevels() == 0) { throw new IllegalStateException("LOD levels are not set on this mesh"); } @@ -241,7 +241,7 @@ public int getTriangleCount() { * @throws IllegalArgumentException If mesh is null */ public void setMesh(Mesh mesh) { - SceneGraphThreadWarden.assertOnCorrectThread(this); + assert SceneGraphThreadWarden.assertOnCorrectThread(this); if (mesh == null) { throw new IllegalArgumentException(); } @@ -272,7 +272,7 @@ public Mesh getMesh() { */ @Override public void setMaterial(Material material) { - SceneGraphThreadWarden.assertOnCorrectThread(this); + assert SceneGraphThreadWarden.assertOnCorrectThread(this); this.material = material; nbSimultaneousGPUMorph = -1; if (isGrouped()) { diff --git a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java index 99a1a1d24e..72724c380d 100644 --- a/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java +++ b/jme3-core/src/main/java/com/jme3/scene/threadwarden/SceneGraphThreadWarden.java @@ -43,9 +43,9 @@ public class SceneGraphThreadWarden { * @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){ + public static boolean setup(Node rootNode){ if(checksDisabled()){ - return; + return true; } Thread thisThread = Thread.currentThread(); if(mainThread != null && mainThread != thisThread ){ @@ -53,6 +53,8 @@ public static void setup(Node rootNode){ } mainThread = thisThread; setTreeRestricted(rootNode); + + return true; // return true so can be a "side effect" of an assert } /** @@ -116,10 +118,11 @@ public static boolean updateRequirement(Spatial spatial, Node newParent){ return true; // return true so can be a "side effect" of an assert } - public static void reset(){ + 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(){