Hi all,
While looking at the JME2 sources, I found a possible improvement in the class Spatial, in the method updateWorldData(). Starting at line 534:
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;
}
}
}
updateWorldVectors();
}
The list with controllers gets updated here, but this happens in a 'first in the list first, last in the list last' (by lack of a better description) way. Now, since controllers can remove themselves from the list, an IndexOutOfBoundsException might occur. To fix this, a try-catch block was added. Like mentioned in the source, this can cause the skipping of the update of one controller.
Is there a reason to update the controllers in this order? If not, it would probably be better to update 'last in the list first, first in the list last'. saves some overhead from the try-catch block, and makes sure everything gets updated.
Rene
Controllers should always be iterated: first come, first served. Not the other way around. Furthermore, I rose a similar issue “back in the days” 
and we came to another solution:
http://www.jmonkeyengine.com/forum/index.php?topic=9675.0
Ah, I see. Well, it was worth the try 
Rene Z said:
Ah, I see. Well, it was worth the try :P
I thought so too! (and still kinda do) ;)
Creating a copy of the spatial list then iterating it might work, but would create lots of garbage data during updates. Maybe a thread local "spatial list" should be available for this purpose.
Momoko_Fan said:
Creating a copy of the spatial list then iterating it might work, but would create lots of garbage data during updates. Maybe a thread local "spatial list" should be available for this purpose.
how about that then (no garbage and unmeasurable runtime effect):
Index: src/com/jme/scene/Spatial.java
===================================================================
--- src/com/jme/scene/Spatial.java (revision 4561)
+++ src/com/jme/scene/Spatial.java (working copy)
@@ -270,6 +270,8 @@
/** ArrayList of controllers for this spatial. */
protected ArrayList<Controller> geometricalControllers;
+
+ protected ArrayList<Controller> geometricalControllersIter;
private static final Vector3f compVecA = new Vector3f();
private static final Quaternion compQuat = new Quaternion();
@@ -309,6 +311,7 @@
public void addController(Controller controller) {
if (geometricalControllers == null) {
geometricalControllers = new ArrayList<Controller>(1);
+ geometricalControllersIter = new ArrayList<Controller>(1);
}
geometricalControllers.add(controller);
}
@@ -366,6 +369,7 @@
public Controller getController(int i) {
if (geometricalControllers == null) {
geometricalControllers = new ArrayList<Controller>(1);
+ geometricalControllersIter = new ArrayList<Controller>(1);
}
return geometricalControllers.get(i);
}
@@ -378,6 +382,7 @@
public ArrayList<Controller> getControllers() {
if (geometricalControllers == null) {
geometricalControllers = new ArrayList<Controller>(1);
+ geometricalControllersIter = new ArrayList<Controller>(1);
}
return geometricalControllers;
}
@@ -533,10 +538,12 @@
*/
public void updateWorldData(float time) {
// update spatial state via controllers
- if (geometricalControllers != null) {
- for (int i = 0, gSize = geometricalControllers.size(); i < gSize; i++) {
+ if (geometricalControllers != null && geometricalControllersIter != null) {
+ geometricalControllersIter.clear();
+ geometricalControllersIter.addAll(geometricalControllers);
+ for (int i = 0, gSize = geometricalControllersIter.size(); i < gSize; i++) {
try {
- Controller controller = geometricalControllers.get(i);
+ Controller controller = geometricalControllersIter.get(i);
if (controller != null) {
if (controller.isActive()) {
controller.update(time);
i'm sure its pretty fast since only references are copied, but still it gets called every update cycle and wont make things faster 
Just reversing the order of the iteration would be more performant, but might introduce some problems if controllers depend on the current order.
Could maybe the removing of a controller automaticly be queued instead of removed directly?
IMHO, the performance is not the main reason to change this part of code. I think it isn't good that there is a possibility a Controller gets skipped during an update.
Queuing removal might be a possibility, but maybe it is more practical to use some kind of LinkedList approach: During the update, the 'current' Controller can remove itself, but the Iterator will still point to the correct 'next' Spatial. This can't be done using the standard LinkedList, as it will throw a ConcurrentModificationException in case a Controller removes itself, and for every update a new Iterator is created so it means a lot of garbage.
I'm not sure about the Vector class, I have no experience with it but as far as I know it allows modifications during iteration.
BUMP
This issue crossed my mind again. What about this solution?
public void updateWorldData(float time) {
// update spatial state via controllers
if (geometricalControllers != null) {
for (int i = 0, lastSize = geometricalControllers.size(); i < lastSize; ) { // Do NOT increment i each iteration
Controller controller = geometricalControllers.get(i);
if (controller != null) {
if (controller.isActive()) {
controller.update(time);
}
}
if(geometricalControllers.size() < lastSize){ // If controller removed itself
lastSize = geometricalControllers.size() // store new lastSize. No need to increment i as it is already correct.
} else { // If controller didn't remove itself
i++ // We should increment i so it points to the next controller
}
}
}
updateWorldVectors();
}