phetsims / vibe

Library for handling audio for PhET simulations. Provides cross-platform support and enables usage of base64 audio embedded in an HTML document.
MIT License
3 stars 3 forks source link

Global audio volume control #28

Closed jonathanolson closed 7 years ago

jonathanolson commented 7 years ago

May be generally useful, but desired for automated sim testing.

jonathanolson commented 7 years ago

Added an intermediate GainNode between the sound source and the context's destination. @jbphet, does this look like a reasonable approach?

Also, is that an acceptable query parameter Name (audioVolume)?

jbphet commented 7 years ago

Two comments/questions:

jonathanolson commented 7 years ago

I'm curious as to why you went for a volume parameter instead of a simple enable/disable.

For test-server testing:

Also, users would have not indication of what this setting is after startup since there is no volume control on our sims

It's more of an internal volume control, maybe a different name would be better, as end-users would generally not use this. An equivalent statement would be "Users would not be able to use a sim with stringTest=x".

Has this been integrated with sim code so that if this parameter is set to zero the auto control buttons will show that audio is disabled, and pressing the audio toggle button would turn it back on?

If a user-controllable volume setting is desired, presumably we'd want to work on design of how it works (which could be handled in a separate issue).

@jbphet, thoughts on a better name for this parameter, e.g. 'testAudioVolume'? I didn't immediately make it fully developer-only since the ability to set volume at 0.5x or 2x could be valuable sometimes, but I didn't mean to open Pandora's box by recommending a user-available volume control.

jbphet commented 7 years ago

Thanks, that helps to clarify the intent. Calling it 'testAudioVolume' would probably be better, since the intent is apparently only internal use and testing.

I'm assigning to @ariel-phet to comment. I know @jonathanolson just wanted to do something quick and easy to solve an immediate problem, and should move forward with that, but I think that this is something that our users might be very interested in. The use case I'm envisioning is a teacher setting up a URL with the audio disabled so that his/her students could use a sim with a game and not have all the dings and boings happening. So the question to @ariel-phet is whether it's okay to make this an internal-only feature with no intention of ever supporting it outside of PhET, or should we give it some more thought and implement it as a feature that will one day be available to our users.

ariel-phet commented 7 years ago

I think it would be good to have @jessegreenberg weigh in on this question as well as @emily-phet and @terracoda

One potential future use case I see is for sonification, we may need the ability for a user to customize the audio levels of different sounds in the sim, and that is a little different than just adjusting the overall volume one might be using in their speakers or such.

Or as @jbphet suggests, allowing a teacher to disable (or perhaps set a max volume) to dings and boings and such.

ariel-phet commented 7 years ago

Please assign back to me once various parties have weighed in, might also warrant a dev meeting discussion.

terracoda commented 7 years ago

Once the sims make more sounds, allowing the user (student or teacher) as mentioned to control the overall volume would be desirable. I'm not sure that controlling the volume of individual sounds would be necessary as the sounds and their intensities would relate to the data they are representing.

pixelzoom commented 7 years ago

Shouldn't this new query parameter be added to phet.chipper.queryParameters? That's the point of https://github.com/phetsims/chipper/issues/516#issuecomment-260253434.

jonathanolson commented 7 years ago

Shouldn't this new query parameter be added to phet.chipper.queryParameters? That's the point of phetsims/chipper#516 (comment).

I believe I had added it in https://github.com/phetsims/chipper/commit/061b176d2ee17bd0a08f4b832e4a0c5f1d5f049a, let me know if there's something else I should do.

emily-phet commented 7 years ago

I agree with @terracoda - the sonifications for the end-user will be layered with volume controlled based on research findings. Users should only have control of overall (whole sim) volume.

Regarding volume control in general, I'm not sure how important within sim volume control is...I typically control the volume of the output device (e.g., the computer speaker) and not the app. I'm not opposed to volume control in the sim, I just wouldn't put it as a high priority.

pixelzoom commented 7 years ago

This:

gainNode.gain.value = QueryStringMachine.get( 'audioVolume', {
    type: 'number',
    defaultValue: 1
  } ); // TODO: replace with chipper QueryParameters usage

Should be this:

gainNode.gain.value = phet.chipper.queryParameters.audioVolume;
pixelzoom commented 7 years ago

Also need some additional doc for audioVolume. Is there a range of allowed values? Should there be a validateValue function in the schema?

jonathanolson commented 7 years ago

Also need some additional doc for audioVolume. Is there a range of allowed values? Should there be a validateValue function in the schema?

It should be a non-negative finite number (0.0 and up), with 1.0 being full volume (2.0 being double volume, etc.), and it would be great to have validateValue in the schema.

ariel-phet commented 7 years ago

Marking for dev meeting partially for me so I fully understand this issue (and making team aware of this parameter)

pixelzoom commented 7 years ago

@ariel-phet The primary use of audioVolume=0 is to turn off audio during automated testing, so you don't have to suffer through N minutes of bleeps and chirps. Highly unlikely that it would be public facing. (And if it were me, I wouldn't have bothered adding this, I would have just muted my audio while testing.)

samreid commented 7 years ago

it would be great to have validateValue in the schema.

@jonathanolson QueryStringMachine already supports validateValue, can you add it to the schema and test it?

samreid commented 7 years ago

Vibe already has an axon Property for audioEnabled. Perhaps (in the long run) it should also have an axon Property for gain. This will make it possible for us to easily add controls for this in the future.

jonathanolson commented 7 years ago

This will make it possible for us to easily add controls for this in the future.

Sounds good, but then I'll want a query parameter to mute that can't be un-done by a fuzzer doing random things.

pixelzoom commented 7 years ago

I documented audioVolume, added validation, and changed Sound.js to use phet.chipper.queryParameters.audioVolume. @jonathanolson please review.

jessegreenberg commented 7 years ago

IE11 doesn't support the Web Audio API so this was crashing sims that use Vibe. I added a check for the audio context before creating the gain node.

jonathanolson commented 7 years ago

@pixelzoom and @jessegreenberg changes look good (thanks for catching the IE11 bit).