vear said:
I think all this should be rewritten, so that code should check for removed controller explicitly, and not rely on the exception happening. The exception was not reported, cause it is considered normal working if a controller is removed during update, its not an error. It should not be reported, but the code should be rewritten, so that the exception can never happen, and the try-catch should be then removed. If you want to make it right...
partly I agree. But I don't think controller removal during update is normal situation.
I suggest another patch that removeController()/detachChild queues it
and remove at the end of updateGeometricState() method.
But this raises problem to current users who gets the children size by getChildren().size() not by getQuantity()
It seems to be too difficult to make compatible to current user.
I think this kind of patch should be applied to jme3. but not for now.
So I prefer the first patch.
Thanks for the attention. Let's make jme better :)
Index: src/com/jme/scene/Spatial.java
===================================================================
--- src/com/jme/scene/Spatial.java (revision 4097)
+++ src/com/jme/scene/Spatial.java (working copy)
@@ -36,7 +36,10 @@
import java.io.Serializable;
import java.util.ArrayList;
import java.util.HashMap;
+import java.util.LinkedList;
import java.util.Stack;
+import java.util.logging.Level;
+import java.util.logging.Logger;
import com.jme.bounding.BoundingVolume;
import com.jme.intersection.CollisionResults;
@@ -269,6 +272,7 @@
/** ArrayList of controllers for this spatial. */
protected ArrayList<Controller> geometricalControllers;
+ protected LinkedList<Controller> controllersToRemove;
private static final Vector3f compVecA = new Vector3f();
private static final Quaternion compQuat = new Quaternion();
@@ -325,7 +329,12 @@
if (geometricalControllers == null) {
return false;
}
- return geometricalControllers.remove(controller);
+ int index = geometricalControllers.indexOf(controller);
+ if (index >= 0) {
+ removeController(index);
+ return true;
+ }
+ return false;
}
/**
@@ -340,7 +349,12 @@
if (geometricalControllers == null) {
return null;
}
- return geometricalControllers.remove(index);
+ if (controllersToRemove == null) {
+ controllersToRemove = new LinkedList<Controller>();
+ }
+ Controller removed = geometricalControllers.get(index);
+ controllersToRemove.add(removed);
+ return removed;
}
/**
@@ -521,6 +535,11 @@
propagateBoundToRoot();
}
}
+
+ if (controllersToRemove != null) {
+ geometricalControllers.removeAll(controllersToRemove);
+ controllersToRemove = null;
+ }
}
/**
@@ -533,20 +552,14 @@
public void updateWorldData(float time) {
// update spatial state via controllers
if (geometricalControllers != null) {
- for (int i = 0, gSize = geometricalControllers.size(); i < gSize; i++) {
- try {
- Controller controller = geometricalControllers.get(i);
- if (controller != null) {
- if (controller.isActive()) {
- controller.update(time);
- }
- }
- } catch (IndexOutOfBoundsException e) {
- // a controller was removed in Controller.update (note: this
- // may skip one controller)
- break;
- }
- }
+ for (int i = 0, gSize = geometricalControllers.size(); i < gSize; i++) {
+ Controller controller = geometricalControllers.get(i);
+ if (controller != null) {
+ if (controller.isActive()) {
+ controller.update(time);
+ }
+ }
+ }
}
updateWorldVectors();
Index: src/com/jme/scene/Node.java
===================================================================
--- src/com/jme/scene/Node.java (revision 4097)
+++ src/com/jme/scene/Node.java (working copy)
@@ -36,6 +36,7 @@
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.LinkedList;
import java.util.List;
import java.util.Stack;
import java.util.logging.Level;
@@ -68,6 +69,7 @@
/** This node's children. */
protected List<Spatial> children;
+ protected LinkedList<Spatial> childrenToRemove;
/**
* Default constructor.
@@ -99,7 +101,10 @@
public int getQuantity() {
if(children == null) {
return 0;
- }
+ }
+ if (childrenToRemove != null) {
+ return children.size() - childrenToRemove.size();
+ }
return children.size();
}
@@ -220,17 +225,11 @@
if(children == null) {
return -1;
}
- if (child == null)
- return -1;
- if (child.getParent() == this) {
- int index = children.indexOf(child);
- if (index != -1) {
- detachChildAt(index);
- }
- return index;
- }
-
- return -1;
+ int index = children.indexOf(child);
+ if (index < 0)
+ return -1;
+ detachChildAt(index);
+ return index;
}
/**
@@ -271,12 +270,12 @@
if(children == null) {
return null;
}
- Spatial child = children.remove(index);
- if ( child != null ) {
- child.setParent( null );
- logger.info("Child removed.");
+ if (childrenToRemove == null) {
+ childrenToRemove = new LinkedList<Spatial>();
}
- return child;
+ Spatial removed = children.get(index);
+ childrenToRemove.add(removed);
+ return removed;
}
/**
@@ -286,10 +285,10 @@
*/
public void detachAllChildren() {
if(children != null) {
- for ( int i = children.size() - 1; i >= 0; i-- ) {
- detachChildAt( i );
+ if (childrenToRemove == null) {
+ childrenToRemove = new LinkedList<Spatial>();
}
- logger.info("All children removed.");
+ childrenToRemove.addAll(children);
}
}
@@ -384,13 +383,7 @@
Spatial child;
for (int i = 0, n = getQuantity(); i < n; i++) {
- try {
- child = children.get(i);
- } catch (IndexOutOfBoundsException e) {
- // a child was removed in updateGeometricState (note: this may
- // skip one child)
- break;
- }
+ child = children.get(i);
if (child != null) {
child.updateGeometricState(time, false);
}
@@ -398,6 +391,17 @@
}
@Override
+ public void updateGeometricState(float time, boolean initiator) {
+ super.updateGeometricState(time, initiator);
+
+ if (childrenToRemove != null) {
+ children.removeAll(childrenToRemove);
+ logger.info("Children removed.");
+ childrenToRemove = null;
+ }
+ }
+
+ @Override
public void updateWorldVectors(boolean recurse) {
if (((lockedMode & Spatial.LOCKED_TRANSFORMS) == 0)) {
updateWorldScale();
