It seems that Light and its associated concrete subclasses (PointLight, SpotLight, …) rely on a simple shallow .clone() implementation. That means that if I import a Light and then clone it, all the clones share the same instance of Color, Position, Direction. Changing the Color, Position, Direction of any one Light, changes it for all.
So if I import a SpotLight, in order to leverage externally defined attributes like Color and range and angles, etc, and then clone() it and try to position the various SpotLights in different places, they all end up in the same place since they are all sharing the same instance of the Position vector.
Is this really how .clone() for a Light is supposed to work? Is this just a bug? What is the recommended approach for getting a copy of a Light that starts with the same attributes as another, but which can subsequently be customized without altering the original?
That’s definately a bug.
At least for Position and Direction we should have a different instance each clone.
For the Colour it would make sense aswell, since if you’d want to save that RAM you could do it manually after cloning.
Agreed, that the position of a PointLight should be cloned, not shared. It doesn’t make much sense.
My humble oppinion: Better this way than the other way around (shallow cloning is okay or even better here). Usually lights share the same color if they are of same type (e.g. street lights or lamps in hallways). And you can all deactivate or activate them at the same time (e.g. “street light - day or night” or “hallway - power of building on or off”).
But if it were the other way around, it should be clear from the API that lamps could share the same ColorRGBA - it must be clear if I can make them share the same ColorRGBA instances.
But I suggest to stay consistent - either clone all or share all.
It’s a bug… it has to be since there is no way to change the color instance of a light. You can call setColor() all day long but it’s only going to call color.set() on its internal value that’s now shared among all of the clones.
Funny thing if you have a j3o with a light in it and load it multiple times then they will also share the same position, color, etc…
In my SDK it shows only 4 sub classes. Okay, the PBR light is in a different fork (and I’m sure that @nehon has read this thread). These lights I see have very few and already cloneable attributes - e.g. PointLight has two float and one Vector3f. The base class is not that complicated too - just clone the color instance.
That’s why I use ColorRGBA.Blue.clone() even if not really needed. But for newbies we should make a tutorial that lists that as one of the many pitfalls of jME. We could also tell them to use myColor.set(ColorRGBA.Blue).
Though it would be better to simply don’t use Local on any light, vector whatever processing function because (as the first part of the chain), even if it wasn’t for the Blue you could also have other shared instances.
Using your clone approach would remove the improvement again (since you have different instances).
Additionally it occassionally happens that people wonder why their mult doesn’t work as expected, so there is the need to explain that.
I know, I shouldn’t, but I’m using that “feature” in the upcoming demo game: Just change the r,g,b,a attributes of a ColorRGBA instance used in a material and then the material changes color too. I found this to be pretty nice and concise. But it kind of brakes the rule to always use a setter - because it’s like using the getter and then setting some of the attributes directly (I know I could use a chain like: material.setColor(material.getColor()) but found this to be so overly verbose) So, this works for materials now:
color.r = 0.5f
Yes, I use this feature also… for vec3, vec4, and color. All the time.
There’s been talk of optimizing how material parameters are loaded to avoid the constant resetting of values that don’t change… but if/when that change makes it into core then I will follow up with one that lets us specifically mark some as dynamic. (At least until I add scene graph based material parameter overrides.)