Hey all, I was wondering if it were possible to change Node to initialize the children list, removing the requirement to check for nulls when calling Node.getChildren().
I could see cons for this approach, such as the added time for object creation and the added memory requirement, but it might be worth it.
Patch:
diff --git a/src/com/jme/scene/Node.java b/src/com/jme/scene/Node.java
index c42c701…e541df1 100755
— a/src/com/jme/scene/Node.java
+++ b/src/com/jme/scene/Node.java
@@ -67,7 +67,7 @@ public class Node extends Spatial implements Serializable, Savable {
private static final long serialVersionUID = 1L;
/** This node's children. */
- protected List<Spatial> children;
+ protected List<Spatial> children = new ArrayList<Spatial>();
/**
* Default constructor.
–
Also, I'm not sure if this is the correct place to post these kind of requests, as I'm not a committer. Also, is there a standard patch format (this one's just a git dump)?
Thanks,
Zack
there is getQuantity() in Node which should be enough to check if there are any children attached. A lot of ballast has been removed from Node in the past for performance reasons, and it would be a shame to see all that ballast go back in over time.
Exactly. As there are many spatials/nodes initializing the list by default is a performance (memory size) issue.
I see, definitely understandable considering the large number of nodes created in a typical JME app.
protected List<Spatial> children = new ArrayList<Spatial>(0); might be reasonable? (I mean, who is going to create a Node with NO childeren anyway??)
I can already think of several situations off the top of my head. 
(Placeholders etc)
I agree with llama and I don't think that placeholders are a valid argument against since placeholders are going to be replaced by something that has children as well. :o
Ok, it's Node not Spatial. I agree it's not a real memory impact.
I still do not think it's necessary to initialize it on construction. But it'd be ok. Just be sure to make it final then and remove all the != null checks in that case to make it an actual improvement.
This is optimization without proving it's worth it. Unless there is a design requirement I would leave it as it is.
Honestly I don't see that there's any down-side to this suggestion. It will definitely not be a performance penalty (unless of course you've littered a bunch of empty Nodes in your scene for some strange reason) and despite an argument for performance improvement it would reduce the complexity of the code by removing all the null checks as Irrisor suggested.
Well, that’s an argument .
According to http://www.javapractices.com/topic/TopicAction.do?Id=83 a newly created ArrayList is around 80 bytes. If someone happens to have 1000 nodes in a scene that’s 80KB.
Not too much overhead, and the performance improvement could be a bigger benefit.
I don’t know. Up to you guys. A null check doesn’t seem like a big mess in the code to me. But, as llama said, who is going to create a Node with no children? :roll:
well i guess there could be people (like me) that use the children list being null in their game logic.
if (rootNode != null && rootNode.getChildren() == null) {
rootNode.attachChild(starRain);
} // if
but maybe thats just bad form anyway ;).
rootNode.getChildren().size() > 0
would fix that. anyhow, just wanted to let you know.
If you had a quadtree of nodes and then build a scene editor to populate it, you would expect some nodes to have no children.
I vote for it to be in
I have just realized that if we change this (of which I'm still not sure) we need to consider that Node is Serializable. The readObject() method now (maybe) should cope with old serialized objects for which "children" is null. Therefore we would need to modify the readObject to check for that and initialize children (we can also break compatibility with previously serialized objects, as JME2 is in development, but we should have a policy about whether we support previous serialized objects if in the future we apply changes like this).
Currently children is being initialized to: Collections.synchronizedList(new ArrayList<Spatial>(1)). The same should be done in readObject(). Remember, readObject() can be thought of as another constructor or as a factory method.
Now, I wonder whether we should be consistent and apply the same strategy to other fields like Spatial.geometricalControllers which is also null-based, and possibly other null-based structures in the scenegraph.
For consistency reasons, and because I fail to see any improvement initializing children, I would leave Node as it is.
Edit: I used "code" tags instead of "tt". I know, I should use "preview" more often…
jjmontes said:
Now, I wonder whether we should be consistent and apply the same strategy to other fields likewhich is also null-based, and possibly other null-based structures in the scenegraph.
Spatial.geometricalControllers
Obviously not a good idea, as that would definitely consume a lot of memory, while having no use at all.
jjmontes said:
For consistency reasons, and because I fail to see any improvement initializing, I would leave Node as it is.
children
That'd be my opinion as well.
I have another doubt: we can try to be up to date with this forum about contribution, but is there anyone checking the commits done to the repository, or could someone evil or unadvertedly commit unexpected changes without anyone noticing?
I say this because so far I was happily updating the project every once in a while, but now I feel a bit reluctant… 
Do now the traditional developers need to post changes to the contribution depot before commiting?
(Again, I don't think this open access to the repository is a good thing. I trust the current developers and their ability to let people in. No need to grant everyone commit rights. Especially now that this board exists, if current developers don't have time to commit changes in this board someone could take that role).
Well I felt I should check the commits… to get any idea at all of what is being done and how easy it is to check this stuff… I think we need to have a more strict way of commenting the commits as some comments are very good while others a bit lacking. Also many of the patches posted on this forum have not been committed yet. I did not do a real 1:1 comparison, but I guess that many of the threads with no [committed] tag are not in SVN.
Another idea is to have all the patch topics here as polls… and at least a certain number of 'yea'-s is needed for a commit, even if no comments are made. I have the distinct feeling that some of the patches are overlooked… ahh… I am just rambling i think…
Anyways we should continue to fine-tune the patch process, but I guess the discussion should be in a separate thread.