/**
* <code>setLocalTranslation</code> sets the local translation of this
* node.
*
* @param localTranslation
* the local translation of this node.
*/
public void setLocalTranslation(Vector3f localTranslation) {
this.localTranslation = localTranslation;
this.worldTranslation.set(this.localTranslation);
}
(in Spatial.java)
Is it intended to use the parameter by reference? Since the class already has its own localTranslation field, initialized in the constructor. I can't think of a reason why the set() method should not be used here.
By many people this is considered a feature, so we did not change that yet. It's the same for some other things (rotation etc.). It was intended, but not all devs like it that way (makes change detection impossible etc.).
True, but it also offers plenty of chances for unintended behavior. If it didn't break so many existing uses I would definitely vote for changing this to copy only. Maybe once we hit a 1.0 version we can shift afterwards.
But if that's really the intended functionality you want, why not just use getLocalTranslation().set(myVector) instead? Granted that this can definitely cause unknowing developers to break something and not have any clue why, but it really does provide both capabilities based on desired functionality.
I understand where you are coming from, but I really think the default behavior should cause the least amount of hard-to-track bugs. It would be best if setXXX() was setting a value rather than a reference and did not cause object sharing but at the same time we offered another less obvious means to share references. This way you really know what you are doing if you go that way.
I agree in premise, but only because it is non-typical to have mutable objects like Vector3f. I think the way we have it with replacing reference would be perfectly correct if Vector3f were immutable. I also think it is perfectly correct even in the current scenario, but the fact that Vector3f IS mutable makes everything a little more confusing for the "un-initiated". Ideally we could have methods that support mutable and immutable styles but that would be a lot of clutter added to every Object in jME and I think would draw away from the power that jME ultimately provides.
I believe we could even gain better performance (and less garbage) if we were to do the following:
Provide ImmutableVector3f that extends Vector3f that throws an Exception if you try to change anything on it
Create a public static final ImmutableVector3f in Spatial set to 0.0f, 0.0f, 0.0f
Make Spatial (and anything else that needs default Vector3fs) utilize that static ImmutableVector3f instead of creating a Vector3f for every single Spatial (particularly when the majority of times it is immediately changed a new Vector3f anyway).
That would reduce the number of objects created in a scene significantly. The only place this would cause a problem is in the circumstance (that I believe is not very typical anyway) I mentioned above by calling getLocalTranslation().set(myVector3f()), but it would then throw a clear exception that would explain that you are using an immutable Vector3f and then you could change it.
One problem with that is that it leads to a lot more setting of Vectors, everything with default vectors will need to have them set to actual mutable default vectors just after construction, which would make it a bit of a nightmare to update old code. I don’t know if this is the “recommended” technique, but I always try to avoid using (for example) setLocalTranslation at all, so I know that each Node keeps it’s own separate original Vector3f. Then I ALWAYS use getLocalTranslation().set to move things. If setLocalTranslation didn’t exist, then there would be no problem since it’s not possible to get a “shared” Vector3f into a Node, as renanse was saying.
I posted this a while ago : http://www.jmonkeyengine.com/jmeforum/index.php?topic=3532.0 on roughly the same subject, but it killed the thread stone dead I hope that wasn’t because it is a horrible idea I still think that the FastNode approach there could work for change monitoring and would also enforce a “neater” way of handling vectors as described above, for people who use the new Node, without breaking old code. Maybe I should implement FastNode (or at least some of it) and try it out to see if it is actually ok to work with.
What if we supported some sort of option in the constructor during instantiation instead? I see some good optimizations this could have on game development personally. Particularly when you're considering things a lot of times that will never be changed in the first place.
you cannot make a subclass of Vector3f that is immutable - it has public fields! Additionally you will lose quite a lot of performance with polymorphisms.
I think we will have to come up with a better solution once we decide to change the current behavior.
I understand that the public variables have to stay in Vector3f. I just opt for factoring out an interface from Vector3f and using that interface where possible in JME (of course, in some places with heavy calculations Vector3f would be still needed). This won't break compatibility, but would allow us to have different implementations of that interface, including an ImmutableVector3f.
E.g. lets say we have
Vector3f implements Vector3fInterface
and
ImmutableVector3f implements Vector3fInterface
then we could rewrite in GeometryBatch without any problems:
public void addGeometry(TriMesh mesh, Vector3fInterface translation)
Thank you for your input. As we move towards the 1.0 release, these are the sorts of things we will be taking care of. So, if you (or anyone else) wishes to start a thread where fundamental OO design was broken without a good reason, feel free to start that thread and we can work to remedy it. As the JVM gets better (and it's a remarkable improvement from 1.4 to 1.6) these sorts of shortcuts start losing the benefits. Please, say where you feel mistakes have been made and the other developers and I will discuss whether it needs to be changed.
I hope that wasn’t because it is a horrible idea 
