phetsims / tambo

library containing code to support sonification of PhET simulations
MIT License
2 stars 4 forks source link

Provide `stepTimer` option for sounds that need to be stepped. #193

Closed pixelzoom closed 4 months ago

pixelzoom commented 5 months ago

ContinuousPropertySoundClip is an example of a sound that needs to be stepped. The step method is "used for fading out the sound in the absence of change."

Having to call step can be very inconvenient, and problematic if sound needs to continue while the sim is paused.

The stepTimer option found in twixt.Animation provides an alternative (albeit with problems of its own) to having to manually call step:

  // One of the following config:
  // The Emitter (which provides a dt {number} value on emit) which drives the animation, or null if the client
  // will drive the animation by calling `step(dt)` manually.  Defaults to the joist Timer which runs automatically
  // as part of the Sim time step.
  // TODO #3: {ScreenView} - animates only when the ScreenView is the active one.
  // TODO #3: {Node} - animates only when the node's trail is visible on a Display
  stepEmitter?: TReadOnlyEmitter<[ number ]> | null;

... and FELSonifier:

  // The Emitter (which provides a dt {number} value on emit) which drives the animation, or null if the client
  // will drive the animation by calling step(dt) manually. Defaults to stepTimer (the joist Timer) which fires
  // automatically when Sim is stepped.
  stepEmitter?: TEmitter<[ number ]> | null;

Assigning to @jbphet so we can discuss/triage today.

jbphet commented 4 months ago

I've implemented this change for ContinuousPropertySoundClip, and changed the usages in sims such that they all use the default stepEmitter and thus don't have to step the ContinuousPropertySoundClip explicitly in the client code. Seems like a nice improvement.

This is the only sound generator with a step function like this in tambo, so I think this is done. I'll try to make sure to follow this pattern in the future.

@pixelzoom - can you please review the changes? You can ignore the commit to WaterBalanceSoundGenerator - that shouldn't have been included as part of this.

pixelzoom commented 4 months ago

Changes for stepEmitter option look good. Back to @jbphet in case there's more work to do, feel free to close.