Since some days, I'm beginning to study jME. This evening, I've checked-out the java code of jME (and I'm happy to see maven as one of the build tool).
So I'm a newbie (as mentionned by this forum regarding the number of my posts) however, I'm not so newbie concerning Java :roll:
So, I've started to read the class Spatial, I've then jumped to see what a Controller is and now I'm back to Spatial.
In 5 minutes I've:
learned what a Controller is (though nothing is detailled about Controllerin the wiki :’( but now I know I can add such documentation),
seen maybe some code that could be optimized or that could lead to problems
So is this forum the right place to discuss such things (from cosmetics to maybe important ones)?
Well, I don't understand the contract for returning objects like ArrayList instead of say List, UnmodifiableList or even Collection
for list of controllers in the class Spatial and Node.
Moreover, as mentionned by the note, line 340, the lack of synchronization on this object may lead to strange results.
So is this done to ensure memory gain (because of lazy initialization)? :?
Or because this method is mostly overriden, this is not a problem (though I can see the same behaviour in Node line 394 for children)?
However, the point is that testing that the lazy initialized objects (Spatial.geometricalControllers, Node.children, Geometry.batchList) are differrent from null in a lot of place tends to obfusc the code (if not slowing the execution).
What do you think of it?
And what about the lack of synchronization?
I didn't dig in the history of these classes to discover what were the reasons but maybe you could help me understand a little more?
About the second constructor of the class Spatial, it could be replaced by:
public Spatial(final String name) {
this(); <--- it will call the default constructor
this.name = name;
}
instead of
public Spatial(String name) {
this.name = name;
localRotation = new Quaternion();
worldRotation = new Quaternion();
localTranslation = new Vector3f();
worldTranslation = new Vector3f();
localScale = new Vector3f(1.0f, 1.0f, 1.0f);
worldScale = new Vector3f(1.0f, 1.0f, 1.0f);
}
About the remark for the first contructor:
/**
* Empty Constructor to be used internally only.
*/
public Spatial() {
localRotation = new Quaternion();
worldRotation = new Quaternion();
localTranslation = new Vector3f();
worldTranslation = new Vector3f();
localScale = new Vector3f(1.0f, 1.0f, 1.0f);
worldScale = new Vector3f(1.0f, 1.0f, 1.0f);
}
Wouldn't it be safer to set its visibility to package (as following)?
/**
* Empty Constructor to be used internally only.
*/
Spatial() {
localRotation = new Quaternion();
...
worldScale = new Vector3f(1.0f, 1.0f, 1.0f);
}
My guess is ArrayList is used because it doesn't make any difference, really. An ArrayList is used, so why not return one? Eg. the List interface lacks elementAt() which we use a lot (for faster itteration).
About synchronization, it's slow. Escpecially synchronized collections. You should only use it if there's a point to it…
My guess is ArrayList is used because it doesn't make any difference, really. An ArrayList is used, so why not return one?
Simply because I can then manipulate directly the data inside it }:-@ thus possibly changing the behviour of the enclosing object.
llama said:
Eg. the List interface lacks elementAt() which we use a lot (for faster itteration).
Well, the class I know that has elementAt() method is Vector, are you telling me that you use Vector instead of ArrayList sometimes :wink:
Well it appears, that the Vector implements List as well as ArrayList and though List doesn't have the elementAt() method, it has the get() which has the same behaviour.
Moreover, if List was returned you could simply change the implementation (even org.apache.commons.collection.FastArrayList).
llama said:
About synchronization, it's slow. Escpecially synchronized collections. You should only use it if there's a point to it.. if you make all modifications from the same thread, that is never. Synchronized collections are useless anyway.. because it could still lead to inpredicatable results. With jME, if you choose to, you can have fine grained control over when you update or render an object, so you can do your own synchronization around that.
Hmmm, tell me if I'm wrong, but imagine, I want to create a cool MMORPG with a server side module that handles the physics and a client that just render the world.
Wouldn't problem bein to occurs on the server concerning multi-threaded update of the tree? :| (just some thoughts)
But indeed I guess synchronization must be used where needed only! :wink:
Then I continue, in the Node class, maybe children should be instanciated to ArrayList<Spatial>(1) because as I understand, a Node maintains the scene tree and thus has always at least one child (a leaf: Geometry or an other node: Node) :?
Node.swapChildren (Line 305), the method Collections.swap(children, index1, index2); could be used, see javadoc, it performs some more checks and provides one optimization when index1 == index2. And here, no test about children being different from null is done :-o
In the Quad.resize method, maybe some tests could be done to modify the list of vertices only when width and/or height are really changed (by adding transient width and height attributs to the class). It would allow the coding of getWidth and getHeight methods which could be useful for 2D games (think of Quad used for sprites with animation frames that could have different sizes). :roll:
Simply because I can then manipulate directly the data inside it }:-@ thus possibly changing the behviour of the enclosing object.
Yes, I don't like that, too. It was done that way to access the list quickly from outside. The UnmodifiableList is not used to save memory...
But it's not that critical for the children of Nodes, as I don't think anybody does modify the list. A bigger problem is returning a reference to localTranslation/Rotation/Scale: when these are modified directly the scenegraph cannot detect when it need to update transforms and thus does this every frame... a good solution for this is still pending (though Mojomonk said they would already have one at NCSoft).
...
A bigger problem is returning a reference to localTranslation/Rotation/Scale: when these are modified directly the scenegraph cannot detect when it need to update transforms and thus does this every frame... a good solution for this is still pending (though Mojomonk said they would already have one at NCSoft).
yes i also had problems whith localTranslation/Rotation/Scale. i wanted to do some additional operations when an object is scaled. the problem is that you can change the scale of a Node in 2 different ways: getLocalScale().set(...) or by calling setLocalScale(...), so overriding setLocalScale() does not help. For the same reason if you need a Quad.getHieght() and Quad.getWidth(), you'll have to compute the values each time because you can't really trace size changes.
With the set and getXXX().set thing, it seems like this flows from Vectors being mutable. Since mutable Vectors let you avoid lots of object creation this is definitely the best way to do it, but the syntax is slightly awkward, and as you say it's hard to detect changes, so I've thought before that it might be nice to have "alter" methods, alterTranslation, etc. These would specifically do the same thing as getTranslation().set, but since the Node would "notice", it could mark itself "dirty", and the code would be a bit neater too. The problem would be all the huge amounts of code that do getXXX().set Of course it would still be possible to have external references to the node's Vectors being changed, but IMHO that is a bit of a bug to have in any code anyway. If you have single vectors being shared between different objects it gets really hard to follow
One way to deal with this completely would be to create a Node subclass (FastNode?) that specifically did not implement set, only "get", "alter" and "store". Using set would give a runtime exception (and be deprecated at compile time), using "alter" would copy a provided vector's values to the node's Vector, and "store" would set a provided vector to the node's values. At no point would FastNode ever accept a new instance of Vector, it would always keep its own internal vectors, and never hand out references to them. "get" would return not the internal Vector3f, but an unmodifiable wrapper Vector3f, always reflecting the internal values but not allowing them to be set (more runtime exceptions I'm afraid). Oh… oops, you can't do that since Vector3f has public fields. Well, you could deprecate/runtime exception FastNode.get as well. That would cover pretty much everything that people actually want to do, and they could voluntarily move to using the new FastNode and the new methods. Any FastNodes in a scenegraph would only be updated as necessary since they would always know when their Vectors had altered (only possible by calling alter methods). Node could gain a field to check quickly for a FastNode without instanceof. I can't think of any way that could fail, since a FastNode would always retain the same Vector3f it was constructed with (it would have to have no Vectors provided on construction), so there is no way of putting an externally-referenced Vector into it, or modifying the internal Vector without calling a FastNode method. Everything above obviously goes for Quaternions and rotation the same way as Vectors and translation/scale. If this has already been solved at NCSoft then sorry for the rambling
Of course it would still be possible to have external references to the node's Vectors being changed, but IMHO that is a bit of a bug to have in any code anyway. If you have single vectors being shared between different objects it gets really hard to follow