simplify track cloning

[stephengold]

This PR resolves issue #829 ("BoneTrack.clone() adds scales") and also cleans up a design issue that Paul raised in the wake of commit ce871d8.

[pspeed42]

Why doesn't SpatialTrack clone() also follow the pattern and instead do its own cloning?

[Nehon]

IMO Compact arrays shouldn't be cloned. They are pure immutable data and that's typically what you want to share between instances. Never got why they were cloned in the first place.

Also, not sure this PR worse the shot, these classes will be deprecated in 3.3, except for CompactArray.

[stephengold]

I don't want SpatialTrack.clone() to perform a deep clone of the track, because that would mean cloning the track spatial (plus all its controls and children). If the user wants a clone that deep, they should provide their own cloner.

As for CompactArray, I see it's immutable only after serialize() has been invoked. If invalid==false, jmeClone() could return an alias and cloneFields() could be a no-op. Better?

Perhaps we shouldn't even allow cloning a CompactArray before it's been serialized.

Even if animation tracks are deprecated in 3.3, I expect they'll continue to be used for some time.

[pspeed42]

re: "I don't want SpatialTrack.clone() to perform a deep clone of the track, because that would mean cloning the track spatial (plus all its controls and children). If the user wants a clone that deep, they should provide their own cloner."
...but then you could always just not clone that in the cloneFields() or alternately, in clone() null out the field and then set it back.

I guess the question is, what does it mean to not deep clone a spatial track that has a reference to a tracked spatial? Do you want both tracks to share the spatial or the new track to have an empty spatial? Either approach can be accommodated with a proper clone() method calling the JME Cloner. (If you want it to share a reference, you could even just index it in the cloner so the spatial won't be cloned.

[stephengold]

I see 2 circumstances in which one might want to clone a track. One is when a model or scene is being cloned and deep cloning is desired. For that one needs an external cloner. The other circumstance is when only the track is being cloned, and the goal is to have 2 tracks referencing the same spatial. The clone() method is the obvious interface to use in the latter situation.

Never cloning the track spatial in cloneFields() would break the deep cloning situation. Nulling and restoring the field in clone() would work great, but it would make the code less clear than in the current PR.

I'm not sure what indexing in the cloner means. Say more about that, please.

Also, Paul, I'd like your opinions on cloning compact arrays. Should they be implement cloning at all? And if so, should cloning be optimized away if invalid==false?

[pspeed42]

When you create a JME Cloner, you can tell it objects that you do not want to clone.

http://javadoc.jmonkeyengine.org/com/jme3/util/clone/Cloner.html#setClonedValue-T-T-

cloner.setClonedValue(trackedSpatial, trackedSpatial);

...you could do similar to what's done for other objects and have a clone() and a clone(boolean) method (the first which calls the second with default behavior). The boolean could control whether or not you stick that spatial in the cloner ahead of time as above.

I think that's really clear what's happening: "We are using the JME Cloner to clone this but we don't want to clone this one object."

Then cloneFields() can call JME cloner to clone it just like always.

[pspeed42]

I don't think I have a strong opinion on the arrays thing. We could make it so that it doesn't clone if it's already been "immuted". If the dependencies are right, I guess we could even build a clone function for that type that would do that... clone it or return it depending on its mutability state.

[stephengold]

Great tip, Paul. I rewrote SpatialTrack.clone() to use setClonedValue() as you suggested. Thank you!

After studying the CompactArray class again, I don't consider its instances immutable, not even when invalid==false. Nothing prevents CompactArray.add() being called when invalid==false. Even if JME treats CompactArray as pure immutable data, but there's nothing stopping a user from cloning an array and then adding to one instance or the other. So I think it's appropriate to clone the CompactArray fields when cloning a track.

Please re-review this PR, guys.