phetsims / neuron

"Neuron" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 3 forks source link

TravelingActionPotentialNode sets computeShapeBounds on a shape, which seems incorrect #44

Closed jbphet closed 9 years ago

jbphet commented 9 years ago

Noticed the following code in TravelingActionPotentialNode.js during the code review:

    function updateShape() {
      var transformedShape = mvt.modelToViewShape( travelingActionPotential.getShape() );
      transformedShape.computeShapeBounds = computeShapeBounds;
      foreground.setShape( transformedShape );
      background.setShape( transformedShape );
    }

Setting the computeShapeBounds function to be essentially stubbed out is a trick that we sometimes use to make a shape render quickly if we know that it isn't going to be manipulated by the user. However, this is generally done on a node, and not a shape. I'm thinking that the code that sets this method for the transformedShape is not needed, and that the other code in the same file overrides the method for the background and foreground nodes is sufficient.

Assigning to @AshrafSharf for discussion if needed, or just changing the code if he agrees.

AshrafSharf commented 9 years ago

computeShapeBounds function assignment to Shape instance removed

jbphet commented 9 years ago

Looks good, closing.