From 349c9d9e4abf69e16336ba18eec67dec443af155 Mon Sep 17 00:00:00 2001 From: "PSp..om" Date: Wed, 13 Jul 2011 08:01:39 +0000 Subject: [PATCH] Modified to use the new SafeArrayList for children and controls. This means it is possible for a control to modify its own node hierarchy in a way that might have caused random skipping at best and index out of bounds exceptions at worse. Also, users can iterate over children and detach them at the same time using standard for each constructs. Performance is the same for me though I'd expected it to be at least slightly faster given that most inner loops now use direct array access. My scenes must not exploit this much. git-svn-id: https://jmonkeyengine.googlecode.com/svn/trunk@7857 75d07b2b-3a1a-0410-a2c5-0572b91ccdca --- .../core/com/jme3/scene/AssetLinkNode.java | 5 +- engine/src/core/com/jme3/scene/Node.java | 74 +++++++++---------- engine/src/core/com/jme3/scene/Spatial.java | 25 ++++--- 3 files changed, 54 insertions(+), 50 deletions(-) diff --git a/engine/src/core/com/jme3/scene/AssetLinkNode.java b/engine/src/core/com/jme3/scene/AssetLinkNode.java index 99764eeaa..ca0068228 100644 --- a/engine/src/core/com/jme3/scene/AssetLinkNode.java +++ b/engine/src/core/com/jme3/scene/AssetLinkNode.java @@ -40,6 +40,7 @@ import com.jme3.export.JmeExporter; import com.jme3.export.JmeImporter; import com.jme3.export.OutputCapsule; import com.jme3.export.binary.BinaryImporter; +import com.jme3.util.SafeArrayList; import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; @@ -177,8 +178,8 @@ public class AssetLinkNode extends Node { @Override public void write(JmeExporter e) throws IOException { - ArrayList childs = children; - children = new ArrayList(); + SafeArrayList childs = children; + children = new SafeArrayList(Spatial.class); super.write(e); OutputCapsule capsule = e.getCapsule(this); capsule.writeSavableArrayList(assetLoaderKeys, "assetLoaderKeyList", null); diff --git a/engine/src/core/com/jme3/scene/Node.java b/engine/src/core/com/jme3/scene/Node.java index e43a25a0f..bb7534a27 100644 --- a/engine/src/core/com/jme3/scene/Node.java +++ b/engine/src/core/com/jme3/scene/Node.java @@ -39,6 +39,7 @@ import com.jme3.export.JmeExporter; import com.jme3.export.JmeImporter; import com.jme3.export.Savable; import com.jme3.material.Material; +import com.jme3.util.SafeArrayList; import java.io.IOException; import java.util.ArrayList; import java.util.List; @@ -65,7 +66,7 @@ public class Node extends Spatial implements Savable { /** * This node's children. */ - protected ArrayList children = new ArrayList(1); + protected SafeArrayList children = new SafeArrayList(Spatial.class); /** * Serialization only. Do not use. @@ -99,7 +100,7 @@ public class Node extends Spatial implements Savable { @Override protected void setTransformRefresh(){ super.setTransformRefresh(); - for (Spatial child : children){ + for (Spatial child : children.getArray()){ if ((child.refreshFlags & RF_TRANSFORM) != 0) continue; @@ -110,7 +111,7 @@ public class Node extends Spatial implements Savable { @Override protected void setLightListRefresh(){ super.setLightListRefresh(); - for (Spatial child : children){ + for (Spatial child : children.getArray()){ if ((child.refreshFlags & RF_LIGHTLIST) != 0) continue; @@ -121,11 +122,11 @@ public class Node extends Spatial implements Savable { @Override protected void updateWorldBound(){ super.updateWorldBound(); + // for a node, the world bound is a combination of all it's children // bounds BoundingVolume resultBound = null; - for (int i = 0, cSize = children.size(); i < cSize; i++) { - Spatial child = children.get(i); + for (Spatial child : children.getArray()) { // child bound is assumed to be updated assert (child.refreshFlags & RF_BOUND) == 0; if (resultBound != null) { @@ -145,11 +146,11 @@ public class Node extends Spatial implements Savable { public void updateLogicalState(float tpf){ super.updateLogicalState(tpf); - // FIXME: Iterating through the children list backwards - // to avoid IndexOutOfBoundsException. This is sometimes unreliable, - // a more robust solution is needed. - for (int i = children.size()-1; i >= 0; i--){ - Spatial child = children.get(i); + if (children.isEmpty()) { + return; + } + + for (Spatial child : children.getArray()) { child.updateLogicalState(tpf); } } @@ -166,15 +167,16 @@ public class Node extends Spatial implements Savable { updateWorldTransforms(); } - // the important part- make sure child geometric state is refreshed - // first before updating own world bound. This saves - // a round-trip later on. - // NOTE 9/19/09 - // Although it does save a round trip, - for (int i = 0, cSize = children.size(); i < cSize; i++) { - Spatial child = children.get(i); - child.updateGeometricState(); - } + if (!children.isEmpty()) { + // the important part- make sure child geometric state is refreshed + // first before updating own world bound. This saves + // a round-trip later on. + // NOTE 9/19/09 + // Although it does save a round trip, + for (Spatial child : children.getArray()) { + child.updateGeometricState(); + } + } if ((refreshFlags & RF_BOUND) != 0){ updateWorldBound(); @@ -428,8 +430,7 @@ public class Node extends Spatial implements Savable { if (name == null) return null; - for (int x = 0, cSize = getQuantity(); x < cSize; x++) { - Spatial child = children.get(x); + for (Spatial child : children.getArray()) { if (name.equals(child.getName())) { return child; } else if(child instanceof Node) { @@ -454,8 +455,7 @@ public class Node extends Spatial implements Savable { if (children.contains(spat)) return true; - for (int i = 0, max = getQuantity(); i < max; i++) { - Spatial child = children.get(i); + for (Spatial child : children.getArray()) { if (child instanceof Node && ((Node) child).hasChild(spat)) return true; } @@ -483,14 +483,14 @@ public class Node extends Spatial implements Savable { @Override public void setLodLevel(int lod){ super.setLodLevel(lod); - for (int i = 0; i < children.size(); i++){ - children.get(i).setLodLevel(lod); + for (Spatial child : children.getArray()) { + child.setLodLevel(lod); } } public int collideWith(Collidable other, CollisionResults results){ int total = 0; - for (Spatial child : children){ + for (Spatial child : children.getArray()){ total += child.collideWith(other, results); } return total; @@ -575,7 +575,7 @@ public class Node extends Spatial implements Savable { @Override public Spatial deepClone(){ Node nodeClone = (Node) super.clone(); - nodeClone.children = new ArrayList(); + nodeClone.children = new SafeArrayList(Spatial.class); for (Spatial child : children){ Spatial childClone = child.deepClone(); childClone.parent = nodeClone; @@ -587,7 +587,7 @@ public class Node extends Spatial implements Savable { @Override public void write(JmeExporter e) throws IOException { super.write(e); - e.getCapsule(this).writeSavableArrayList(children, "children", null); + e.getCapsule(this).writeSavableArrayList(new ArrayList(children), "children", null); } @Override @@ -596,12 +596,12 @@ public class Node extends Spatial implements Savable { // This prevents empty children list if controls query // it in Control.setSpatial(). - children = e.getCapsule(this).readSavableArrayList("children", null); + children = new SafeArrayList( Spatial.class, + e.getCapsule(this).readSavableArrayList("children", null) ); // go through children and set parent to this node if (children != null) { - for (int x = 0, cSize = children.size(); x < cSize; x++) { - Spatial child = children.get(x); + for (Spatial child : children.getArray()) { child.parent = this; } } @@ -612,8 +612,8 @@ public class Node extends Spatial implements Savable { @Override public void setModelBound(BoundingVolume modelBound) { if(children != null) { - for(int i = 0, max = children.size(); i < max; i++) { - children.get(i).setModelBound(modelBound != null ? modelBound.clone(null) : null); + for (Spatial child : children.getArray()) { + child.setModelBound(modelBound != null ? modelBound.clone(null) : null); } } } @@ -621,16 +621,16 @@ public class Node extends Spatial implements Savable { @Override public void updateModelBound() { if(children != null) { - for(int i = 0, max = children.size(); i < max; i++) { - children.get(i).updateModelBound(); + for (Spatial child : children.getArray()) { + child.updateModelBound(); } } } @Override public void depthFirstTraversal(SceneGraphVisitor visitor) { - for(int i = 0, max = children.size(); i < max; i++) { - children.get(i).depthFirstTraversal(visitor); + for (Spatial child : children.getArray()) { + child.depthFirstTraversal(visitor); } visitor.visit(this); } diff --git a/engine/src/core/com/jme3/scene/Spatial.java b/engine/src/core/com/jme3/scene/Spatial.java index f67984bb5..4f2de2d03 100644 --- a/engine/src/core/com/jme3/scene/Spatial.java +++ b/engine/src/core/com/jme3/scene/Spatial.java @@ -53,6 +53,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.util.SafeArrayList; import com.jme3.util.TempVars; import java.io.IOException; import java.util.ArrayList; @@ -131,7 +132,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { public transient float queueDistance = Float.NEGATIVE_INFINITY; protected Transform localTransform; protected Transform worldTransform; - protected ArrayList controls = new ArrayList(1); + protected SafeArrayList controls = new SafeArrayList(Control.class); protected HashMap userData = null; /** * Spatial's parent, or null if it has none. @@ -512,8 +513,8 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { return; } - for (int i = 0; i < controls.size(); i++) { - controls.get(i).update(tpf); + for (Control c : controls.getArray()) { + c.update(tpf); } } @@ -532,8 +533,8 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { return; } - for (int i = 0; i < controls.size(); i++) { - controls.get(i).render(rm, vp); + for (Control c : controls.getArray() ) { + c.render(rm, vp); } } @@ -590,9 +591,9 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { * @see Spatial#addControl(com.jme3.scene.control.Control) */ public T getControl(Class controlType) { - for (int i = 0; i < controls.size(); i++) { - if (controlType.isAssignableFrom(controls.get(i).getClass())) { - return (T) controls.get(i); + for (Control c : controls.getArray()) { + if (controlType.isAssignableFrom(c.getClass())) { + return (T)c; } } return null; @@ -1114,7 +1115,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { if (clone instanceof Node) { Node node = (Node) this; Node nodeClone = (Node) clone; - nodeClone.children = new ArrayList(); + nodeClone.children = new SafeArrayList(Spatial.class); for (Spatial child : node.children) { Spatial childClone = child.clone(cloneMaterial); childClone.parent = nodeClone; @@ -1127,7 +1128,7 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { clone.setTransformRefresh(); clone.setLightListRefresh(); - clone.controls = new ArrayList(); + clone.controls = new SafeArrayList(Control.class); for (int i = 0; i < controls.size(); i++) { clone.controls.add(controls.get(i).cloneForSpatial(clone)); } @@ -1240,7 +1241,9 @@ public abstract class Spatial implements Savable, Cloneable, Collidable { capsule.write(shadowMode, "shadow_mode", ShadowMode.Inherit); capsule.write(localTransform, "transform", Transform.IDENTITY); capsule.write(localLights, "lights", null); - capsule.writeSavableArrayList(controls, "controlsList", null); + + // Shallow clone the controls array to convert its type. + capsule.writeSavableArrayList( new ArrayList(controls), "controlsList", null); capsule.writeStringSavableMap(userData, "user_data", null); }