phetsims / pendulum-lab

"Pendulum Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 10 forks source link

Acceleration Arrows in PendulumsNode #66

Closed aaronsamuel137 closed 9 years ago

aaronsamuel137 commented 9 years ago

During code review #56, I noticed this code:

        options.isAccelerationVisibleProperty.linkAttribute( accelerationArrow, 'visible' );

        // add visibility observer
        options.isAccelerationVisibleProperty.link( function( isAccelerationVisible ) {
          accelerationArrow.visible = isAccelerationVisible;
          pendulum.updateAccelerationVector();
        } );

It looks to me like the call to link is totally unnecessary after linkAttribute has already been called. The call to pendulum.updateAccelerationVector() also looks unnecessary, and it seems like it is getting called every frame in any case.

Is this correct, or is there some reason for having this code that I am not seeing? Please either document why it needs to be here or remove.

Also, it might be nice to change the link call in the velocity vector code to linkAttribute as well to make the two consistent.

        options.isVelocityVisibleProperty.link( function( isVelocityVisible ) {
          velocityArrow.visible = isVelocityVisible;
        } );
andrey-zelenkov commented 9 years ago

Replace 'link' to 'lazyLink' for accelerationVector, added comments, fixed bug for velocityVector. Reassign to @aaronsamuel137 for verification.

aaronsamuel137 commented 9 years ago

Looks better, thanks for the comments. Closing