Changes from constants to accessible variables (AbstractPhysicsBody & AbstractCollisionBody)

[JNightRide]

Hello everyone.

This PR modifies the properties of type final so that those who inherit from class AbstractCollisionBody | AbstractPhysicsBody can modify it (have full access to it).

Having the lists as final greatly limits the classes that inherit from these objects since we cannot instantiate new objects for those properties... I think it is better to have them accessible so that users can modify them if they consider it necessary.

What do you think of this proposal?

[wnbittle]

@JNightRide would you be able to provide a few concrete scenarios where this proposal would help you? For example, "I want to be able to assign these final members so I can perform a copy" or "I want to do this so that I can serialize/deserialize the object" etc. Knowing the use-case(s) for this change would help me play the pros/cons game.

That said, at a high level, marking things final provide the following benefits:

1. I can guarantee the underlying object that is assigned to the member has the correct performance characteristics (think about the differences between an ArrayList, LinkedList, etc.)
2. I can guarantee the object is always non-null
3. The JVM can reorganize that memory more efficiently if it knows its address can't be changed by user code
4. The JVM can optimize methods that use that memory if it knows its address can't be changed by user code

[JNightRide]

Hi @wnbittle.

Thanks for reviewing this PR.

What I'm trying to do is clone physical bodies, make an exact copy of them; However, currently properties cannot be cloned correctly (cloning is only partially done), which is bad. I'm trying to serialize dyn4j objects for the JME engine (it's a personal project).

Releasing these properties can give access to object cloning support (clone() method), since cloning an object; In itself, its attributes are not cloned, so it must be done manually, which involves making new references to the objects; For example:

import java.util.ArrayList;
import java.util.List;

public class TestBody extends AbstractPhysicsBody implements Cloneable {

    public TestBody() {
    }
    
    @Override
    public TestBody clone() {
        try {
            TestBody clon = (TestBody) super.clone();
            clon.fixtures = (List<BodyFixture>) ((ArrayList<BodyFixture>) fixtures).clone();
            clon.forces = (List<Force>) ((ArrayList<Force>) forces).clone();
            
            clon.transform = transform.copy();
            // For all attributes to make a new copy by cloning...
            return clon;
        } catch (CloneNotSupportedException e) {
            throw new InternalError(e);
        }
    }
}

This results in a new object identical to the original; Of course, the BodyFixture should be cloned anyway, but as far as I know, those objects are not a problem.

If you want, you can visit this repository to see what I intend to do (repo)

[wnbittle]

Imagine dyn4j had a .clone() method on every object today (in other words, you don't have to define the clone method or extend from these classes at all) would you still need the members non-final? Is there another use-case outside of cloning that you need the members non-final?

For the cloning use-case, I'm proposing every (within reason) class implement the custom interface Copyable<T> that exists in dyn4j today.

[JNightRide]

Imagine dyn4j had a .clone() method on every object today (in other words, you don't have to define the clone method or extend from these classes at all) would you still need the members non-final?

I think in this case the question would be; the properties would be protected or private since if the clone() method exists (along with the Cloneable interface) dyn4j had to do something to perform the full clone of all its attributes, so it is possible that these properties are not final

Is there another use-case outside of cloning that you need the members non-final?

Another way (possibly) is serialization, since we have to rebuild the object in its initial (original) state.

For the cloning use-case, I'm proposing every (within reason) class implement the custom interface Copyable that exists in dyn4j today.

The Copyable<T> interface can be used when the class has only primitive type attributes. For more complex objects, I would recommend implementing the Cloneable interface and overriding the clone() method.

[JNightRide]

I just realized that there is an open issue about cloning support, it is possible that this PR could contribute to this issue since porting the Cloneable interface involves releasing several (non-final) properties...

[wnbittle]

Thank you for the additional detail. I was doing some testing with using the Copyable<T> interface and found a few interesting things:

• The annoying thing with Copyable<T> is that I can't implement it as Copyable<CollisionBody<T>> on the CollisionBody interface AND implement it as Copyable<PhysicsBody> on the PhysicsBody interface. What this means is that everything that extends CollisionBody will always report returning a CollisionBody instead of the current type. The contract of the Copyable<T> interface is that every subclass should override the copy method and return an instance of the subclass, but there's no enforcement of that rule. This poses a problem for those who extend from any of these classes today because they may not know that they need to override the copy method since it's implemented in super classes.
• There are some protected members on the AbstractCollisionBody that really shouldn't be copied. The owner and fixtureModificationHandler for example. These properties are only set when the body is added to a World and I don't think copy/clone should automatically add the copy to the World. There's also the userData member - would a developer want that deep copied, reference copied, not copied at all, or something between? This issue applies to native Java Clonable as well.
• The Copyable<T> interface can't be fully implemented on abstract classes, namely AbstractCollisionBody and AbstractPhysicsBody since the contract is a copy method that should return an instance of the class itself, but you can't create an instance of an abstract class. This issue applies to native Java Clonable as well.

The serialization/deserialization scenario is a tough one. There are some members you don't want serialized (or simply can't be) and some members that don't have anything to serialize except for what class is being used. I messed around with serialization/deserialization in the Sandbox application that was built long ago that could be used as a reference, but I wouldn't do it that way again. The point is, that you should be able to rebuild the simulation without access to the protected/final methods (I think there are some issues with this in 5.x though), but there isn't a native dyn4j way to do that.

So to summarize, I don't think Copyable<T> will work and Clonable has similar problems. Serialization has some of the same issues as cloning and a few additional issues. I also don't think final is the problem here - in your example above:

// don't do this
// clon.transform = transform.copy();
// do this
clon.transform.set(transform);

// don't do this
// clon.fixtures = (List<BodyFixture>) ((ArrayList<BodyFixture>) fixtures).clone();
// do something like this
for (T fixture : this.fixtures) {
  BodyFixture bf = new ...;
  bf.setXXX(...)
  bf.setYYY(...)
  clon.fixtures.add(bf);
}

Sure, it's more code, but it solves some key issues with dyn4j supporting natively. I'd love to support what I call "Copy Constructors", but there's an issue there as well when copying fixtures.

Next steps: I want to try a few more things, but at this point, I don't think dyn4j will be able to support a native Copy feature or Clonable. It may come down to users having to implement their own way of cloning.

[JNightRide]

For now I will leave this PR (possibly close it)...

The Copyable interface can't be fully implemented on abstract classes, namely AbstractCollisionBody and AbstractPhysicsBody since the contract is a copy method that should return an instance of the class itself, but you can't create an instance of an abstract class. This issue applies to native Java Clonable as well.

This part is where 'Cloneable' does the magic, the JVM is responsible for making an exact copy of the object (it doesn't matter if it is abstract) you will have a copy of said object.

What this code snippet does is add the same object back to the list; when you clone an object, you only clone the object itself; Its properties don't, so you have to be careful not to affect the original object.

for (T fixture : this.fixtures) {
  BodyFixture bf = new ...;
  bf.setXXX(...)
  bf.setYYY(...)
  clon.fixtures.add(bf);
}

Next steps: I want to try a few more things, but at this point, I don't think dyn4j will be able to support a native Copy feature or Clonable. It may come down to users having to implement their own way of cloning.

I am very interested in this topic, I would like to help; I'm trying things out on my end and hope to present a cloning possibility.