search

phetsims / fourier-making-waves

"Fourier: Making Waves" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

Ignore irrelevant harmonics? #224

Open pixelzoom opened 2 years ago

pixelzoom commented 2 years ago

This came up during discussion of https://github.com/phetsims/mean-share-and-balance/issues/60 (use of static vs dynamic PhET-iO elements).

Fourier has not been PhET-iO designed yet, and the PhET-iO implementation is incomplete. It uses a static set of 11 harmonics. When numberOfHarmonicsProperty is set to N, all harmonics > N have their amplitude set to 0, ensuring that they have no contribution to the series' waveform. Our decisions for using a "static elements" approach are captured in https://github.com/phetsims/fourier-making-waves/issues/6.

In a Zoom discussion with @samreid and @marlitas, @samreid pointed out that (via Studio) you can set amplitudes for harmonics > N, and they will incorrectly contribute to the waveform. I will investigate how this relates to the desired PhET-iO design for this sim, and whether anything needs to be done here.

@arouinfar FYI.

pixelzoom commented 2 years ago

@arouinfar In https://github.com/phetsims/fourier-making-waves/issues/123#issuecomment-1007805980, you said:

@pixelzoom I would consider the design review to be complete. Change requests are summarized in https://github.com/phetsims/fourier-making-waves/issues/123#issuecomment-924389379 with game-specific requests in #144.

So are you OK with the PhET-iO client being able to change harmonics that do not have a visible slider? For example:

  1. Run the sim in Studio
  2. Go to Discrete screen
  3. Change Harmonics spinner to 5
  4. In Studio, set fourierMakingWaves.discreteScreen.model.fourierSeries.harmonics.harmonic11.amplitudeProperty to 1. Harmonic 11 is now non-zero, so it's shown on the Harmonics chart, and contributes to the Sum. See screenshot below.

I vaguely recall discussing this long ago. Maybe we decided that it's OK as is, and that we'd not in the Client Guide "Do not set non-zero amplitudes for harmonics that are not visible." That would certainly be easier than preventing client from doing that (which does not seem doable, because phetioReadOnly is not dynamic) or changing the implementation to ignore those harmonics.

screenshot_1740
pixelzoom commented 2 years ago

@arouinfar I see in https://github.com/phetsims/fourier-making-waves/issues/123#issuecomment-924389379:

Remove Studio Control

  • [ ] discreteScreen.model.fourierSeries.harmonics.harmonic*.amplitudeProperty
  • [ ] waveGameScreen.model.level.answerSeries.harmonics.harmonic.amplitudeProperty
  • [ ] waveGameScreen.model.level*.numberOfAmplitudeControlsProperty

So we're not planning to allow amplitudes to be changed via Studio. If that's the case, then perhaps they should also be phetioReadOnly: true?

pixelzoom commented 2 years ago

If we want to prevent the PhET-iO client from sabatoging themselves by setting a non-zero amplitude for irrelevant harmonics... We could consider adding visibleProperty to Harmonic, which is derived from numberOfHarmonicsProperty. Harmonics that are invisible would effectively have zero amplitude, regardless of the value of their amplitudeProperty. For view components related to a Harmonic (amplitude sliders, measurement tools, waveforms, ...) we would wire up their visibleProperty to the Harmonic's visibleProperty.

pixelzoom commented 2 years ago

Since a PhET-iO version of this sim is currently not on the horizon, I'm going to label this issue as deferred, and unassign myself and @arouinfar. We'll come back to this when PhET-iO work resumes.

arouinfar commented 2 years ago

@arouinfar I see in https://github.com/phetsims/fourier-making-waves/issues/123#issuecomment-924389379:

Remove Studio Control

  • [ ] discreteScreen.model.fourierSeries.harmonics.harmonic*.amplitudeProperty
  • [ ] waveGameScreen.model.level.answerSeries.harmonics.harmonic.amplitudeProperty
  • [ ] waveGameScreen.model.level*.numberOfAmplitudeControlsProperty

So we're not planning to allow amplitudes to be changed via Studio. If that's the case, then perhaps they should also be phetioReadOnly: true?

These elements should not be phetioReadOnly: true. The reason for asking for phetioStudioControl: false was due to the slider being problematic. Now that we have a better data entry interface in Studio, this set of requests is no longer necessary.