phetsims / hookes-law

"Hooke's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

reduce coil radius as spring is stretched #13

Closed pixelzoom closed 6 years ago

pixelzoom commented 9 years ago

I'm creating this issue in case anyone wants to restore this feature in the future, perhaps when performance of baseline platforms or PhET APIs improves. No action or assignment is required at this time.

As a real-world spring is stretched, the radius of its coil decreases. This feature was implemented in HookesLawSpringNode (a subtype of ParametricSpringNode), but it has a performance penalty: gradients are recomputed as the displacement changes. Since the effect is subtle and the performance penalty is significant on iPad, the feature was removed.

Here's the code that was removed from HookesLawSpringNode's constructor:

    // shrink the coil radius (y dimension) a bit when stretching the spring, to simulate radius change
    spring.displacementProperty.link( function( displacement ) {
      if ( displacement <= 0 ) {
        // compressed
        propertySet.aspectRatioProperty.set( options.aspectRatio );
      }
      else {
        // stretched
        propertySet.aspectRatioProperty.set( options.aspectRatio * ( 1 - 0.05 * displacement / spring.displacementRange.max ) );
      }
    } );
pixelzoom commented 9 years ago

The decision to drop this feature was made in https://github.com/phetsims/hookes-law/issues/3#issuecomment-118965909, where @arouinfar wrote:

@pixelzoom, I would be fine with dropping this behavior, especially since it has a noticeable impact on performance. While it's a nice touch, I doubt that many users will notice such a subtle change in coil radius. I don't think it's worth sacrificing performance for it.

pixelzoom commented 9 years ago

Note that if this feature in added in the future, it will negate the performance optimizations made in https://github.com/phetsims/hookes-law/issues/3. Changing propertySet.aspectRatioProperty changes the number of points (Vector2) used in the spring geometry. When the number of points changes, we can't take advantage of the "mutate points" optimization. And that optimization was responsible for the majority of the observed performance gains.

pixelzoom commented 6 years ago

This feature was taken out ~3 years ago, and no one has ever asked for it. So I'll close this issue.