search

phetsims / tambo

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

Memory leak reviews #176

Closed jonathanolson closed 1 year ago

jonathanolson commented 1 year ago

In https://github.com/phetsims/solar-system-common/commit/d9d7b51d13650fd169de2b65c43ef1ac4054e353, I noticed that enableControlProperties created from addSoundGenerator stick around after removeSoundGenerator. This seems a bit wrong. I added disposal of the soundClips themselves, since that clears the array and handles the memory leak.

However then I noticed another leak of DisplayedProperty in soundManager (we create it on addSoundGenerator, and don't dispose it). I'm working on patching that.

@jbphet can you review the associated commits, and let me know if the asymmetry for enableControlProperties is warranted?

jonathanolson commented 1 year ago

Added a map so I can clean up the leak in soundManager above.

jbphet commented 1 year ago

I looked this over and the functionality looks good. I made some minor editorial changes. Good to go, closing.