ShapeChangingModelElement is maintaining separate bounds

[jbphet]

There is some odd looking code in ShapeChangingModelElement that has to do with how the position and shape relate to each other. It appears that this model element retains a separate bounds variable and uses this for keeping track of where the model element is located.

In the constructor there is this:

    // @public {Bounds2}
    this.bounds = this.shapeProperty.get().bounds.copy();

Position is set via this:

    translate: function( x, y ) {
      this.bounds.shift( x, y );
      this.centerPosition.addXY( x, y );
      this.shapeProperty.notifyListenersStatic();
    },

As a result, the bounds property is updated, then notifications are sent out for a shape change even though the shape itself hasn't changed, only its position. This is fairly confusing to read and thus a maintenance problem. Also, this may be causing some performance degradation, since notifications are going out that the shape has changed when in fact just the position has been updated.

This likely came about because of a misread of the original Java code, where the Shape is explicitly transformed instead of a separate bounds, or as an attempt to improve performance by not causing shape changes at every movement of the model element. At any rate, it should be cleaned up.

My plan to address this is to first try to have separate shape and position properties. If that can be done in a reasonable time frame, I'll call it good. If not, I'll look into transforming the shape with each movement of the model element and see if this works without noticeable performance degradation.

[jbphet]

I looked at the Java code, and in ShapeChangingModelElement.java, the translate method looks like this:

    public void translate( Vector2D translationVector ) {
        AffineTransform translationTransform = AffineTransform.getTranslateInstance( translationVector.getX(), translationVector.getY() );
        shapeProperty.set( translationTransform.createTransformedShape( shapeProperty.get() ) );
    }

This shows that that are no separate bounds - it's just the shape itself that is being transformed. I'm not sure why the separate bounds were added in the JavaScript version. Maybe an attempt to improve performance?

[jbphet]

The way that the bounds were maintained was significantly revised while working on improving performance, and the bounds are now being updated if the shape or position change. They are still being maintained so that they can be quickly accessed (this is mostly used to test overlap with other molecule or to check that it is in bounds), but the methods for positioning the ShapeChangingModelElement are now more efficient and understandable. Closing.