search

phetsims / vegas

Reusable game components for PhET simulations.
MIT License
1 stars 4 forks source link

Rewrite GameAudioPlayer to use tambo #77

Closed jbphet closed 2 years ago

jbphet commented 5 years ago

The GameAudioPlayer type currently uses the older vibe library and should be rewritten to use tambo instead, since that library uses Web Audio in a more standard way and is the one we plan to use long term.

jbphet commented 5 years ago

Closely related to https://github.com/phetsims/vibe/issues/33.

jbphet commented 5 years ago

Here is a list of the sims that will need to have sound buttons removed and be tested for this change:

jbphet commented 5 years ago

This should be complete. I'll leave it open long enough to verify that continuous testing wasn't broken by these changes.

jbphet commented 5 years ago

CT is clear of issues that would be caused by these changes. Closing.

jbphet commented 5 years ago

I just ran a test while working on a different issue where I loaded the current master version of "Build an Atom" with the fuzz parameter enabled and then clicked on the screen so that the audio context could start working. The game audio was played occasionally as fuzz test ran, but after a while it start to get a bit scratchy and distorted sounding, and eventually stopped altogether. I took a couple of heap snapshots a few minutes apart, and they would seem to indicate that memory is being leaked. I should investigate and fix. Reopening.

SaurabhTotey commented 5 years ago

This issue is much more difficult than it would appear. Even after commenting out all of the insides of all of the methods in GameAudioPlayer, it seems like its usages still leak. However, if I comment out the usages of GameAudioPlayer, there is no apparent leak. I am unsure for why this behaviour would happen. I will continue working on this issue.

SaurabhTotey commented 5 years ago

Disregard my previous comment. I believe I didn't test for long enough previously. After longer testing, I don't think these memory leaks are actually related to GameAudioPlayer. Even after commenting out all usages of GameAudioPlayer, the game screens of BAA and Graphing Lines still leak. I will continue looking into what is causing these memory leaks.

SaurabhTotey commented 5 years ago

Just as a small note for myself, when unlinking all linked listeners for all disposable game view objects, it seems like the screen leaks much much faster. I am unsure of why this is happening, but I will continue to investigate.

SaurabhTotey commented 5 years ago

After https://github.com/phetsims/build-an-atom/commit/fb7cb74bdd18791346f2594943c479f57680226b, it seems like the leak is actually coming from GameAudio. Seemingly, having the listeners not getting unlinked was making the leak seem to appear to be from elsewhere. However, now the leak is clearly coming from something sound related, so I will continue looking into GameAudioPlayer. It might be possible that the screen was leaking due to both linked listeners and GameAudioPlayer, and now with one source of leaks gone, the GameAudioPlayer leaks are more apparent.

SaurabhTotey commented 5 years ago

By making the GameAudioPlayer sounds constants, I seem to have at least fixed this issue for BAA. I will need to test and see what happens for other uses of GameAudioPlayer, such as Graphing Lines.

SaurabhTotey commented 5 years ago

After also testing with Graphing Lines and Arithmetic, I believe this issue has been solved. I think earlier, I just got unlucky by using BAA to test, because it probably had another source of leaks that was obfuscating my debugging. I will close this issue now. Feel free to reopen if it actually isn't solved.

pixelzoom commented 2 years ago

Reopening.

As discovered in https://github.com/phetsims/equality-explorer/issues/177, the way that this issue was addressed had an undesirable consequence. All UI sounds (not just game sounds) are now enabled for all sims that use GameAudioPlayer. In the case of Equality Explorer, I've discovered this during RC tesitng for a new release, and we'll have to decided if it's OK to re-publish the sim with partial UI sound support.

@jbphet Please discuss this with @kathy-phet @amanda-phet @arouinfar. The options for sims that use GameAudioPlayer are:

High priority, since this affects an RC that is in progress, https://github.com/phetsims/qa/issues/735.

Here's the list of sims that use GameAudioPlayer, and now have UI sounds enabled:

screenshot_1439
pixelzoom commented 2 years ago

I recommend this option, to be applied to all sims that use GameAudioPlayer and have not been explicitly released with UI sounds enabled:

  • (D) Do something to enable game sounds, but not all UI sounds. (The correct way to address this issue, imo.)

This can be accomplished by setting the output level to zero for the 'ui-sounds' category:

soundManager.setOutputLevelForCategory( 'user-interface', 0 );

But this exposes another bug. Sounds that were recently added to vegas UI components (LevelSelectionButton, back button in scoreboard, ...). were not associated with the 'ui-sounds' category, so can't be turned off. This will also need to be fixed in #98.

kathy-phet commented 2 years ago

@jbphet and @markgammon will collaborate on a sim-specific issue, and put individual issues in the repos of the sims with effected games.

pixelzoom commented 2 years ago

11/18/21 design meeting: @amanda-phet @arouinfar @kathy-phet @markgammon @jbphet @chrisklus @ariel-phet

markgammon commented 2 years ago

Issues created indicating UI sounds have been enabled and how to turn these off:

markgammon commented 2 years ago

@pixelzoom - @jbphet and I created an issue for deciding whether to turn off UI sounds and have added it to each of the listed sims.

Should this issue remain open or ok to close?

pixelzoom commented 2 years ago

Sorry I missed the above question - this issue was not assigned back to me.

OK to close.