Closed Ashton-Morris closed 3 years ago
While these sounds are needed by Fourier, they are not specific to Fourier. This is a general issue that belongs in vegas. So transferring this issue from fourier-making-waves to vegas.
To evaluate, I need information about the sound design. How did we get here? What were the requirements?
@arouinfar @Ashton-Morris were there meetings or discussions that I missed while I was on vacation? Can you fill me in?
The last time that I was involved in a discussion about sound for Level Selection buttons was Fourier design meeting on 4/22/21. From the meeting notes at https://github.com/phetsims/vegas/issues/88#issuecomment-825165387:
LevelSelectionButton might be thematically like radio buttons, where each one has a different pitch (increasing by level number). But note that there is currently no "button group" for LevelSelectionButton in vegas, so no way to programmatically assign/increment different pitches.
There was also a question at the time as to whether we needed different sounds for different levels, or whether 1 sound would suffice for all level-selection buttons. So I'm not clear on how we ended up here, where the papertrail of decisions lives, what the design is, or how I'm supposed to evaluate. For example... What is the design for these sounds? Does each button need a different sound? Is it up to the sim to choose which sounds, or is there a standard for each level? Are the sounds supposed to be different enough to be distinguishable? Are higher levels supposed to have higher pitches?
Feedback on these specific sounds:
Other things I'm not clear on...
Are these 10 sounds that can be assigned to any level, as desired? Or is level-selection-buttons-001.mp3 intended to be the standard sound for Level 1, level-selection-buttons-002.mp3 for Level 2, etc.?
Did implementation considerations inform this sound design? This approach of having 10 mp3 files has some significant implementation consequences. LevelSelectionButton is the common-code button, and (currently) clients create the number of these that they need. If we have 10 different sounds, then clients will need to assign the sounds to each button. That means duplicated code in clients, and the potential to end up with different sound assignments in different sims. Or we'll need to create a "level-selection button group" in vegas, to handle creation of LevelSelectionButtons and sound assignment - a non-trivial piece of work. Using a generated sound assigned that is created by the LevelSelectionButton, based on the level assigned to the button, would be less work, and easier to standardize.
Happy to discuss all of the above, either here or on Slack/Zoom.
These sounds are intended to be played in order. If a sim has 5 options then they should be played in the numbered order.
9 of the sounds are 32 kHz. One is different: level-selection-buttons-010.mp3 is 44.1 kHz. I don't know if that's significant, just mentioning in case it was not intentional. If it was intentional, it might be good to mention why (even if it's just in this issue).
@pixelzoom That was not intentional. I have replaced it with this file.
@jbphet pointed me to the documentation for PhET encoding standards, https://github.com/phetsims/tambo/blob/master/doc/encoding-audio.md. It says:
The .wav file should use a sample rate of 44.1 kHz, since it is the most commonly supported frequency for audio contexts, so re-sampling will not be required in most cases.
So it looks like the 32 kHZ files are the ones that need to be changed.
Questions for 5/13/21 design meeting:
Sound Design:
Different sounds for each level? (If the answer is "no", then most of the other questions here are irrelevant, and implementation is trivial.) YES
Standard sound for each level? YES, based on level (and maybe number of levels?)
Ability to override the standard sound for a level? YES
Should each sound be distinguishable from the others? YES. Some of the above sounds are too similar.
Is it OK to impose an upper limit on the number of levels? If so, what is that limit? NO. (Not a hard NO, but not really necessary to have a limit. Similar to radio buttons.)
Implementation:
mp3 vs generated sounds - limited number of levels, "range" of sound, etc. (I recommend generated sounds, because there's no inherent limit, and the range of tones can be tailored to the number of levels.) Take one recorded sound and change playback rate, like radio buttons.
How to integrate sounds? Options: (a) Have every sim explicitly add sounds to LevelSelectionButton. (I do not recommend this option. It duplicates code, and is will be difficult to ensure standardization.) (b) Add a level number to LevelSelectionButton. Have LevelSelectionButton assign it's own sound based on level number, with the ability to override that via an option. (I recommend this option.) (c) Create something analagous to RadioButtonGroup that creates a group of LevelSelectionButton, and handles sound assignment. This will be significantly more expensive, because we'll also have to deal with layout for the group, which varies from sim to sim (1 row, grid, etc.) (I do not recommend this option; it has little advantage over option b.) We talked about these, and (b) is probably what we'll do.
5/13/21 design meeting: @jbphet @arouinfar @kathy-phet @KatieWoe @pixelzoom
We discussed the bullet items in https://github.com/phetsims/vegas/issues/89#issuecomment-840809753. Answers/conclusions are inlined in https://github.com/phetsims/vegas/issues/89#issuecomment-840809753 in bold font.
Consensus was that the approach should be similar to radio buttons. Use one base sound, and change the playback rate based on level number.
@jbphet suggested having @Ashton-Morris create some candidates for the base sound, then choose one.
@jbphet @arouinfar @Ashton-Morris will follow up with design meetings.
Putting this on hold until we can discuss it further and clarify during our status meeting on the 27th.
This was discussed in today's sound design meeting and @Ashton-Morris is going to come up with a sound or two that we can then play at different rates for the various level selection buttons. Once he provides this, I'll hook it up and we can review it in a larger design meeting.
I have 4 candidates to try. game-button-001.mp3 game-button-002.mp3 game-button-004.mp3 game-button-003.mp3
@jbphet Can you share this with me once implemented so I can listen to them before you bring it up in the larger design meeting?
I've marked this as blocking publication. This is for any sim that uses the level selection buttons, since there will be some untested code in them for a while.
@Ashton-Morris, @emily-phet, and @arouinfar - I've published a dev version of the vegas demo that includes an "Options" dialog that allows us to choose between different base sounds for the level selection buttons, and I've integrated the four initial candidate sounds that @Ashton-Morris provided (see comments above). Please give it a try and log your feedback in this ticket. The dev version can be found at https://phet-dev.colorado.edu/html/vegas/1.0.0-dev.5/phet/vegas_en_phet.html. The level selection buttons are shown on the first screen of the demo.
A number of sims are failing with these types of errors on CT:
balancing-act : fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/balancing-act/balancing-act_en.html?continuousTest=%7B%22test%22%3A%5B%22balancing-act%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1622723211956%22%2C%22timestamp%22%3A1622725839138%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught TypeError: Cannot read property 'value' of undefined
TypeError: Cannot read property 'value' of undefined
at Object.play (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/vegas/js/LevelSelectionButton.js:148:95)
at playSound (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/sun/js/buttons/RectangularPushButton.js:54:51)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/TinyEmitter.js:86:18)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/Emitter.js:39:29
at Emitter.execute (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/Action.js:227:18)
at Emitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/Emitter.js:64:19)
at downPropertyObserver (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/sun/js/buttons/PushButtonModel.js:93:36)
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/TinyEmitter.js:86:18)
at BooleanProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/Property.js:271:23)
at BooleanProperty.set (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1622723211956/axon/js/Property.js:186:14)
id: Bayes Chrome
Snapshot from 6/3/2021, 6:26:51 AM
@samreid suggested it is connected to this issue.
Recommended to revert this work in master, and move it to a branch. This blocks publication of any sim that has a game, @jbphet is liable to be unavailable for awhile, and sounds could certainly be evaluated in a branch.
It is unclear what to move to a branch--instead I put a bandaid on the global access which appears to fix things (tested in balancing act).
The "bandaid" seems to have worked for LevelSelectionButton. But @jbphet also made changes to BackButton, and that's now failing CT in multiple sims:
equality-explorer : multitouch-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/equality-explorer/equality-explorer_en.html?continuousTest=%7B%22test%22%3A%5B%22equality-explorer%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1623075412716%22%2C%22timestamp%22%3A1623076712963%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Uncaught TypeError: Cannot read property 'value' of undefined
TypeError: Cannot read property 'value' of undefined
at Object.play (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/scenery-phet/js/buttons/BackButton.js:42:93)
at playSound (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/sun/js/buttons/RectangularPushButton.js:54:51)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/TinyEmitter.js:86:18)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/Emitter.js:39:29
at Emitter.execute (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/Action.js:227:18)
at Emitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/Emitter.js:64:19)
at downPropertyObserver (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/sun/js/buttons/PushButtonModel.js:93:36)
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/TinyEmitter.js:86:18)
at BooleanProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/Property.js:271:23)
at BooleanProperty.set (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1623075412716/axon/js/Property.js:186:14)
id: Bayes Chrome
Snapshot from 6/7/2021, 8:16:52 AM
I'm going to attempt to revert these changes from master.
Changes have been reverted from master. Here's what I did:
@Ashton-Morris @emily-phet @arouinfar As @jbphet mentioned in https://github.com/phetsims/vegas/issues/89#issuecomment-853445597, you can do your testing in https://phet-dev.colorado.edu/html/vegas/1.0.0-dev.5/phet/vegas_en_phet.html. You won't be able to test in master, since the changes have been reverted. Let me or @jbphet know if that presents any problems.
Removing the "blocks-publication" label, since these changes are no longer present in master.
Thanks @pixelzoom and @samreid for making repairs, and my apologies for introducing the problem. I should know better by now not to try to rush some experimental code and then leave town. I'll fix it all up when I'm back to full work hours.
In https://github.com/phetsims/scenery-phet/issues/677#issuecomment-858843778, @emily-phet said:
Just an update on progress for this issue (unrelated to what sims are impacted, etc.):
- @Ashton-Morris and I met on Wednesday to review the latest sound variants for the level selection and back buttons. @Ashton-Morris will update the issue with specifics on each of the three options we listened to for each, but the outcome is that we think we have a good option of the existing set for the level selection buttons, and @Ashton-Morris is going to make a new variant of one of the existing options to try for the back button.
@emily-phet and I reviewed the Vegas Buttons/ Level Selection Button sounds via the options on Wednesday which can be found here.
@jbphet I would love it if you leave your top choice here also so I can hear your opinion.
I think @Ashton-Morris is saying that he and @emily-phet liked the selection that corresponds to the file game-button-003.mp3
the best, but I will confirm in our next meeting. This is also my first choice.
Our next step should be to try it in Fractions Intro, which has 20 selection buttons for the game.
I reviewed a proposed API for the level selections with @pixelzoom this morning and he felt that it looked reasonable but suggested more documentation. I'll add it. The basic idea is that the client can specify an index number for the default sound player or a fully unique sound player. The code will assert if a client tries to specify both. If an index value is provided, the code will use this to vary the sound that is played for the button. These will be set and described in the options
object.
For option soundPlayerIndex
, it would also be good to note that this index is 0-based. If the client is using 1-based indexing for their levels, they'll need to compensate. Nothing is going to break if they don't compensate, but their levels will all be 1 pitch higher than "standard". This was the case in Fourier, see WaveGameLevelSelectionNode.js:
// {WaveGameLevelSelectionButton[]} a level-selection button for each level
const levelSelectionButtons = _.map( model.levels,
level => new WaveGameLevelSelectionButton( level, model.levels.length, model.levelProperty, {
soundPlayerIndex: level.levelNumber - 1,
tandem: options.tandem.createTandem( `level${level.levelNumber}SelectionButton` )
} )
);
I've added the documentation that @pixelzoom suggested in the previous comment.
We discussed the sounds at today's sound design meeting and decided to move forward with game-button-003.mp3
. I'll delete the other sounds from the repo and make this one official, and will also remove the ability to select the sound from the options dialog.
I have removed the unused sounds and renamed the one that we decided to keep to level-selection-button.mp3
.
@Ashton-Morris - Two things:
.wav
version of this sound for the assets directoryI have changed the filename in my system. Here is the .wav
I've added the asset and we did some level adjustment during today's sound design meeting. I think we're done here. Closing.
I have created 10 level selection/Vegas buttons based on the solute selection options used in Molarity. Since 10 is the highest number of buttons for "games" I have seen I created 10. They are all different from one and another but are still in the same sound family and sound like they consistently increase as the number gets higher.
Please assign anyone to this issue that you feel is needed.
All of the sounds can be found here.
Related to these issues - https://github.com/phetsims/vegas/issues/88#issuecomment-825165387 https://github.com/phetsims/fourier-making-waves/issues/54