search

phetsims / sound-waves

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

This sim uses a lot of code from wave interference #6

Closed samreid closed 1 year ago

samreid commented 1 year ago

There are currently 52 imports from wave-interference, including images, sound effects, UI components, etc. Should we treat "wave-interference" like a common code dependency (like circuit-construction-kit-common, or density-buoyancy-common), or should we take time to move things to tambo, scenery-phet, etc?

@kathy-phet and @pixelzoom what do you recommend?

pixelzoom commented 1 year ago

Should we treat "wave-interference" like a common code dependency

Definitely not, unless you rename it to have a "-common" suffix. And a "-common" repo would be the approach when dealing with a "suite" of sims (like CCK) where there's a "common" repo that's specific to that suite (like circuit-construction-kit-common).

or should we take time to move things to tambo, scenery-phet, etc?

Unless wave-intererence and sound are part of the same "suite"... Resources (code, images, etc.) should be moved from a sim-specific repo to a common-code repo when they are need by a second sim. In the case of code, the code should be generalized -- that means (for example) default options that are not sim-specific, documentation that is general, etc.

And if it's time that's the real issue, versus which approach is correct... Take the time to use the correct approach.

samreid commented 1 year ago

At today's quarterly planning meeting, I indicated it might take 4-6 hours to move the reusable code to the correct repos. I don't recall being allocated time for that this quarter. I'll unassign for now, but since I'm the responsible dev for this repo, I may return to it if all my other issues are completed.

samreid commented 1 year ago

From discussion with @jbphet on slack:

Hi John. We want to use a sound sine wave oscillator in the new “sound” sim. Right now it is borrowing from wave-interference https://github.com/phetsims/wave-interference/blob/0908c5bc1f6be2242a4db72336a3debd41da5153/js/waves/view/SineWaveGenerator.ts. Should I move that file SineWaveGenerator to tambo?

@jbphet said:

Sure. In Web Audio, it's fairly easy to specify the waveform (from a limited set of options). Can you consider making it just a WaveGenerator with an option for the wave type?

@samreid said:

So… move the file to tambo. Rename it to WaveGenerator. Leave the default as oscillatorType: 'sine' ?

@jbphet said:

Sounds good to me!

@samreid said:

I’ll do that, thanks!

@jbphet said:

Thank you for doing it!

samreid commented 1 year ago

I moved Lattice and ImageDataRenderer to scenery-phet. There are no more dependencies on the wave-interference repo, closing.