Significantly changed the way updateLogicalState

works internally to avoid full-scene graph traversals
once per frame.
Random unrelated things first:

Spatial constructors were modified to be protected since it
is an abstract class and it's good practice.  I also flipped
them to be this(default values) instead of this() to delegate
constructors.

Geometry and Node constructors were modified in similar ways
to at least call the this(name) version of the constructor
everywhere.

removeControl(Class) was modified to do what it's javadoc
says and only remove the first encountered control.  Prior
to this change, it was removing all matching controls.

The meaty stuff:
Node.updateLogicalState() no longer traverses all children
and never traverses any children at all unless it is the root.
If it is the root node, it keeps a list of all Spatials that
require updates and only iterates that list.

Spatial was modified to have a package-private requiresUpdates()
method that returns true if the requiresUpdates flag is set
or the Spatial has controls.

The requiresUpdates flag can be set by subclasses that require
updateLogicalState() to be called even if they have no controls.
(BitmapText is such an example.)  By default, subclasses will
_always_ have this flag set to true, ie: default to the OLD behavior
and avoid silently breaking.

Subclasses that wish to have the new-hotness optimal behavior
must call setRequiresUpdates(false) in their constructors.
They can opt in to better performance rather than silently
breaking with an upgrade.

The only negative side effect of this change (other than
slightly increased complexity) is that for large scene
graphs with lots and lots of controls or children requiring
updates, a large list of spatials will be kept in the root
node.  This will never exceed "number of spatials in the
scene graph" and so already had a fairly small upper bound
with the old code due to performance.

I think I've captured all of the edge cases... but we'll
see. :)

Stress test example to follow.

jme3-core/src/main/java/com/jme3/scene/Geometry.java

@@ -77,15 +77,18 @@ public class Geometry extends Spatial {
      * is managed by.
      */
     protected GeometryGroupNode groupNode;
+    
     /**
      * The start index of this <code>Geometry's</code> inside 
      * the {@link GeometryGroupNode}.
      */
-    protected int startIndex = -1;    
+    protected int startIndex = -1;
+        
     /**
      * Serialization only. Do not use.
      */
     public Geometry() {
+        this(null);
     }
 
     /**
@@ -97,6 +100,12 @@ public class Geometry extends Spatial {
      */
     public Geometry(String name) {
         super(name);
+        
+        // For backwards compatibility, only clear the "requires
+        // update" flag if we are not a subclass of Geometry.
+        // This prevents subclass from silently failing to receive
+        // updates when they upgrade.
+        setRequiresUpdates(Geometry.class != getClass()); 
     }
 
     /**

jme3-core/src/main/java/com/jme3/scene/Node.java

@@ -66,10 +66,27 @@ public class Node extends Spatial implements Savable {
      */
     protected SafeArrayList<Spatial> children = new SafeArrayList<Spatial>(Spatial.class);
 
+    /**
+     * If this node is a root, this list will contain the current
+     * set of children (and children of children) that require 
+     * updateLogicalState() to be called as indicated by their
+     * requiresUpdate() method.
+     */
+    private SafeArrayList<Spatial> updateList = null;
+    
+    /**
+     * False if the update list requires rebuilding.  This is Node.class
+     * specific and therefore not included as part of the Spatial update flags.
+     * A flag is used instead of nulling the updateList to avoid reallocating
+     * a whole list every time the scene graph changes.
+     */     
+    private boolean updateListValid = false;    
+
     /**
      * Serialization only. Do not use.
      */
     public Node() {
+        this(null);
     }
 
     /**
@@ -81,6 +98,12 @@ public class Node extends Spatial implements Savable {
      */
     public Node(String name) {
         super(name);
+        
+        // For backwards compatibility, only clear the "requires
+        // update" flag if we are not a subclass of Node.
+        // This prevents subclass from silently failing to receive
+        // updates when they upgrade.
+        setRequiresUpdates(Node.class != getClass()); 
     }
 
     /**
@@ -152,16 +175,72 @@ public class Node extends Spatial implements Savable {
         this.worldBound = resultBound;
     }
 
+    @Override
+    protected void setParent(Node parent) {
+        if( this.parent == null && parent != null ) {
+            // We were a root before and now we aren't... make sure if
+            // we had an updateList then we clear it completely to 
+            // avoid holding the dead array.
+            updateList = null;
+            updateListValid = false;
+        }
+        super.setParent(parent);
+    }
+
+    private void addUpdateChildren( SafeArrayList<Spatial> results ) {
+        for( Spatial child : children.getArray() ) {
+            if( child.requiresUpdates() ) {
+                results.add(child);
+            }
+            if( child instanceof Node ) {
+                ((Node)child).addUpdateChildren(results);
+            }
+        }
+    }
+
+    /**
+     *  Called to invalide the root node's update list.  This is
+     *  called whenever a spatial is attached/detached as well as
+     *  when a control is added/removed from a Spatial in a way
+     *  that would change state.
+     */
+    void invalidateUpdateList() {
+        updateListValid = false;
+        for( Node n = parent; n != null; n = n.getParent() ) {
+            n.invalidateUpdateList();
+        }
+    }
+
+    private SafeArrayList<Spatial> getUpdateList() {
+        if( updateListValid ) {
+            return updateList;
+        }
+        if( updateList == null ) {
+            updateList = new SafeArrayList<Spatial>(Spatial.class);            
+        } else {
+            updateList.clear();
+        }
+
+        // Build the list
+        addUpdateChildren(updateList);
+        updateListValid = true;       
+        return updateList;   
+    }
+
     @Override
     public void updateLogicalState(float tpf){
         super.updateLogicalState(tpf);
 
-        if (children.isEmpty()) {
+        // Only perform updates on children if we are the
+        // root and then only peform updates on children we
+        // know to require updates.
+        // So if this isn't the root, abort.
+        if( parent != null ) {
             return;
         }
-        
-        for (Spatial child : children.getArray()) {
-            child.updateLogicalState(tpf);
+
+        for( Spatial s : getUpdateList().getArray() ) {
+            s.updateLogicalState(tpf);
         }
     }
 
@@ -251,28 +330,7 @@ public class Node extends Spatial implements Savable {
      * @throws IllegalArgumentException if child is null.
      */
     public int attachChild(Spatial child) {
-        if (child == null)
-            throw new IllegalArgumentException("child cannot be null");
-
-        if (child.getParent() != this && child != this) {
-            if (child.getParent() != null) {
-                child.getParent().detachChild(child);
-            }
-            child.setParent(this);
-            children.add(child);
-
-            // XXX: Not entirely correct? Forces bound update up the
-            // tree stemming from the attached child. Also forces
-            // transform update down the tree-
-            child.setTransformRefresh();
-            child.setLightListRefresh();
-            if (logger.isLoggable(Level.FINE)) {
-                logger.log(Level.FINE,"Child ({0}) attached to this node ({1})",
-                        new Object[]{child.getName(), getName()});
-            }
-        }
-        
-        return children.size();
+        return attachChildAt(child, children.size());
     }
     
     /**
@@ -298,12 +356,18 @@ public class Node extends Spatial implements Savable {
             }
             child.setParent(this);
             children.add(index, child);
+            
+            // XXX: Not entirely correct? Forces bound update up the
+            // tree stemming from the attached child. Also forces
+            // transform update down the tree-
             child.setTransformRefresh();
             child.setLightListRefresh();
             if (logger.isLoggable(Level.FINE)) {
                 logger.log(Level.FINE,"Child ({0}) attached to this node ({1})",
                         new Object[]{child.getName(), getName()});
             }
+            
+            invalidateUpdateList();
         }
         
         return children.size();
@@ -380,6 +444,8 @@ public class Node extends Spatial implements Savable {
             child.setTransformRefresh();
             // lights are also inherited from parent
             child.setLightListRefresh();
+            
+            invalidateUpdateList();
         }
         return child;
     }
@@ -390,6 +456,9 @@ public class Node extends Spatial implements Savable {
      * node.
      */
     public void detachAllChildren() {
+        // 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.
         for ( int i = children.size() - 1; i >= 0; i-- ) {
             detachChildAt(i);
         }

jme3-core/src/main/java/com/jme3/scene/Spatial.java

@@ -162,16 +162,24 @@ public abstract class Spatial implements Savable, Cloneable, Collidable, Cloneab
     protected transient int refreshFlags = 0;
 
     /**
-     * Serialization only. Do not use.
+     * Set to true if a subclass requires updateLogicalState() even
+     * if it doesn't have any controls.  Defaults to true thus implementing
+     * the legacy behavior for any subclasses not specifically turning it
+     * off.
+     * This flag should be set during construction and never changed
+     * as it's supposed to be class-specific and not runtime state.
      */
-    public Spatial() {
-        localTransform = new Transform();
-        worldTransform = new Transform();
-
-        localLights = new LightList(this);
-        worldLights = new LightList(this);
+    private boolean requiresUpdates = true;
 
-        refreshFlags |= RF_BOUND;
+    /**
+     * Serialization only. Do not use.
+     * Not really. This class is never instantiated directly but the
+     * subclasses like to use the no-arg constructor for their own 
+     * no-arg constructor... which is technically weaker than
+     * forward supplying defaults.
+     */
+    protected Spatial() {
+        this(null);
     }
 
     /**
@@ -182,9 +190,16 @@ public abstract class Spatial implements Savable, Cloneable, Collidable, Cloneab
      *            the name of the scene element. This is required for
      *            identification and comparison purposes.
      */
-    public Spatial(String name) {
-        this();
+    protected Spatial(String name) {
         this.name = name;
+        
+        localTransform = new Transform();
+        worldTransform = new Transform();
+
+        localLights = new LightList(this);
+        worldLights = new LightList(this);
+
+        refreshFlags |= RF_BOUND;
     }
 
     public void setKey(AssetKey key) {
@@ -195,6 +210,54 @@ public abstract class Spatial implements Savable, Cloneable, Collidable, Cloneab
         return key;
     }
 
+    /**
+     * Returns true if this spatial requires updateLogicalState() to
+     * be called, either because setRequiresUpdate(true) has been called
+     * or because the spatial has controls.  This is package private to
+     * avoid exposing it to the public API since it is only used by Node.
+     */
+    boolean requiresUpdates() {
+        return requiresUpdates | !controls.isEmpty();
+    }
+    
+    /**
+     * Subclasses can call this with true to denote that they require 
+     * updateLogicalState() to be called even if they contain no controls.
+     * Setting this to false reverts to the default behavior of only
+     * updating if the spatial has controls.  This is not meant to
+     * indicate dynamic state in any way and must be called while 
+     * unattached or an IllegalStateException is thrown.  It is designed
+     * to be called during object construction and then never changed, ie:
+     * it's meant to be subclass specific state and not runtime state.
+     * Subclasses of Node or Geometry that do not set this will get the
+     * old default behavior as if this was set to true.  Subclasses should
+     * call setRequiresUpdate(false) in their constructors to receive
+     * optimal behavior if they don't require updateLogicalState() to be
+     * called even if there are no controls.
+     */
+    protected void setRequiresUpdates( boolean f ) {
+        // Note to explorers, the reason this was done as a protected setter
+        // instead of passed on construction is because it frees all subclasses
+        // from having to make sure to always pass the value up in case they
+        // are subclassed.
+        // The reason that requiresUpdates() isn't just a protected method to
+        // override (which would be more correct) is because the flag provides
+        // some flexibility in how we break subclasses.  A protected method
+        // would require that all subclasses that required updates need implement
+        // this method or they would silently stop processing updates.  A flag
+        // lets us set a default when a subclass is detected that is different
+        // than the internal "more efficient" default.
+        // Spatial's default is 'true' for this flag requiring subclasses to
+        // override it for more optimal behavior.  Node and Geometry will override
+        // it to false if the class is Node.class or Geometry.class.
+        // This means that all subclasses will default to the old behavior
+        // unless they opt in. 
+        if( parent != null ) {
+            throw new IllegalStateException("setRequiresUpdates() cannot be called once attached."); 
+        }
+        this.requiresUpdates = f;
+    } 
+
     /**
      * Indicate that the transform of this spatial has changed and that
      * a refresh is required.
@@ -599,8 +662,17 @@ public abstract class Spatial implements Savable, Cloneable, Collidable, Cloneab
      * @see Spatial#removeControl(java.lang.Class) 
      */
     public void addControl(Control control) {
+        boolean before = requiresUpdates();
         controls.add(control);
         control.setSpatial(this);
+        boolean after = requiresUpdates();
+        
+        // If the requirement to be updated has changed
+        // then we need to let the parent node know so it
+        // can rebuild its update list.
+        if( parent != null && before != after ) {
+            parent.invalidateUpdateList();   
+        }
     }
 
     /**
@@ -609,12 +681,22 @@ public abstract class Spatial implements Savable, Cloneable, Collidable, Cloneab
      * @see Spatial#addControl(com.jme3.scene.control.Control) 
      */
     public void removeControl(Class<? extends Control> controlType) {
+        boolean before = requiresUpdates();
         for (int i = 0; i < controls.size(); i++) {
             if (controlType.isAssignableFrom(controls.get(i).getClass())) {
                 Control control = controls.remove(i);
                 control.setSpatial(null);
+                break; // added to match the javadoc  -pspeed
             }
         }
+        boolean after = requiresUpdates();
+        
+        // If the requirement to be updated has changed
+        // then we need to let the parent node know so it
+        // can rebuild its update list.
+        if( parent != null && before != after ) {
+            parent.invalidateUpdateList();   
+        }
     }
 
     /**
@@ -627,11 +709,21 @@ public abstract class Spatial implements Savable, Cloneable, Collidable, Cloneab
      * @see Spatial#addControl(com.jme3.scene.control.Control) 
      */
     public boolean removeControl(Control control) {
+        boolean before = requiresUpdates();
         boolean result = controls.remove(control);
         if (result) {
             control.setSpatial(null);
         }
 
+        boolean after = requiresUpdates();
+        
+        // If the requirement to be updated has changed
+        // then we need to let the parent node know so it
+        // can rebuild its update list.
+        if( parent != null && before != after ) {
+            parent.invalidateUpdateList();   
+        }
+        
         return result;
     }