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?
Of course they share the same Color when cloned.
You need to .setDiffuseColor(ColorRGBA newColor) after cloning.
I think it’s for when you want very similar Lights, example:
Clone point lights, but only change the position ( setLocalTranslation() ) or Color (see above).
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.
I guess this might be related to JME3.0, where there was the Light Interface and no real derivation structure (iirc).
Anyway: Should we enable setColor() to change the instance?
It would be more efficient in such a situation:
ColorRGBA blue = new ColorRGBA(...);
for (int i = 0; i < 10; i++)
rootNode.addLight(new PointLight(new Vector3f(0f, 1f, 1f * i), blue));
So to say when someone is creating the street light, he will most likely use the same instance again.
That way you can keep the reference and change all lights’ Colors at once.
Note: What happens with constants like ColorRGBA.Blue if someone is using something like:
Node n;
n.getLight().getColor().multLocal(0.3f);
This would change the Color with the same effect as changing Vector3f.ZERO for example…
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).
Definately.
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.
That’s true, but I meant the following problem (which rather generally comes with the Light-Sharing):
/* My Light flickering control */
public void controlUpdate(float tpf)
{
spatial.getLocalLightList()[0].getColor().multLocal(FastMath.random());
}
(Actually that code wouldn’t work at all, but you get the Idea: Instead of different flickering you would have multiple lights flickering with the last executed Control.)
This is why I like the idea of Vector3f, to print a warning when such a thing happens (a constant being changed).
So as conclusion:
We could break existing (bad) implementations
It requires being written down somewhere
Is there anything positive about a hard copy of the Color? Except that it works for ppl not knowing about OOP at all?
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 color
Material material.setColor(color)
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.)