search

phetsims / tambo

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

`SoundGenerator` should probably extend `Disposable` #182

Closed jbphet closed 1 year ago

jbphet commented 1 year ago

Under https://github.com/phetsims/scenery/issues/1494 a type was created to standardize how we do and don't support disposing of object instances in order to release memory references and prevent leaks. The type (or class) is called Disposable, and the main type of Scenery - Node - extends this type by virtue of extending PhetioObject. I'm thinking that I should probably make SoundGenerator extend this type also, since it's meant to (sort of) be the audio version of Node.

jbphet commented 1 year ago

Assigning to @zepumph to see if he has any input or insights before I move forward with this. Also labeling it as high priority, since it would be good to work this out before finalizing the disposal behavior for the Greenhouse Effect code review (see https://github.com/phetsims/greenhouse-effect/issues/336).

jbphet commented 1 year ago

Oh bummer - according to the calendar, @zepumph is out for the next week.

I'm a bit uncomfortable doing this without talking with some other devs first, since it is in widely used common code, so I think I'll not change SoundGenerator at this time and will add assertions in the sound generators that currently exist in Greenhouse Effect, and @zepumph can discuss when he gets back.

zepumph commented 1 year ago

Disposable is pretty light weight and seems like a great supertype for this case. Happy to talk more next week. Good luck.

zepumph commented 1 year ago

I poked around SoundGenerator a bit more and it seems excellent for Disposable. You should be able to remove the disposeSoundGenerator function and just use the disposeEmitter.

jbphet commented 1 year ago

This is done. I regression tested on the tambo demo, greenhouse-effect, john-travoltage, and a handful of other sims that have sound. I'll also keep an eye out for CT failures that could potentially be related. Closing.