Sound Effects [AARD-1887]

[AlexD717]

Description

Adds a SoundPlayer that allows any object to play a sound, and added sound effects for UI interactions

Objectives

Add sound into the simulator creating a more interactive and polished experience

Testing Done

• SFX plays on button press
• SFX plays on checkbox press
• SFX plays on dropdown press
• SFX plays on toggle button group buttons (menu selection, ex. robot or field)
• SFX volume can be adjusted in settings
• All sound can be muted with a checkbox in settings

JIRA Issue

[azaleacolburn]

Everything functions correctly, but there are a few issues with the code itself.

• Generally method names should be camelCase rather than PascalCase in Typescript
• If statements that do nothing but assign should be made into ternaries
• soundType should be ignored, since we're not using it yet

diff --git a/fission/src/systems/sound/SoundPlayer.ts b/fission/src/systems/sound/SoundPlayer.ts
index c91db05a8..409ba50bf 100644
--- a/fission/src/systems/sound/SoundPlayer.ts
+++ b/fission/src/systems/sound/SoundPlayer.ts
@@ -7,36 +7,31 @@ import dropdownMenuSound from "@/assets/sound-files/DullClick.wav"
 
 enum SoundType {
     SFX = 0,
-    Music = 1
+    Music = 1,
 }
 
 class SoundPlayer {
     private audio: HTMLAudioElement
 
-    constructor(filePath: string, soundType: SoundType) {
+    constructor(filePath: string, _soundType: SoundType) {
         this.audio = new Audio(filePath)
-        
-        if (PreferencesSystem.getGlobalPreference<boolean>("MuteAllSound")) {
-            this.audio.volume = 0;
-        }
-        else if (soundType == SoundType.SFX) {
-            let sfxVolume = PreferencesSystem.getGlobalPreference<number>("SFXVolume") / 100 // Changes value from percent (0 - 100) to decimal (0 - 1)
-            sfxVolume = clamp(sfxVolume, 0, 1)
-            this.audio.volume = sfxVolume;
-        }
+
+        this.audio.volume = PreferencesSystem.getGlobalPreference<boolean>("MuteAllSound")
+            ? 0
+            : clamp(PreferencesSystem.getGlobalPreference<number>("SFXVolume") / 100, 0, 1)
     }
 
-    Play(): Promise<void> {
-        return this.audio.play().catch((error) => {
+    async play(): Promise<void> {
+        return this.audio.play().catch(error => {
             console.error("Error playing the audio file:", error)
         })
     }
 
-    Pause(): void {
+    pause(): void {
         this.audio.pause()
     }
 
-    Stop(): void {
+    stop(): void {
         this.audio.pause()
         this.audio.currentTime = 0
     }
@@ -44,15 +39,15 @@ class SoundPlayer {
 
 export function buttonPressSFX() {
     const buttonPressedSFX = new SoundPlayer(buttonPressSound, SoundType.SFX)
-    buttonPressedSFX.Play()
+    buttonPressedSFX.play()
 }
 
 export function checkboxPressedSFX() {
     const checkboxPressedSFX = new SoundPlayer(checkboxPressSound, SoundType.SFX)
-    checkboxPressedSFX.Play()
+    checkboxPressedSFX.play()
 }
 
 export function dropdownMenuSFX() {
     const dropdownMenuSFX = new SoundPlayer(dropdownMenuSound, SoundType.SFX)
-    dropdownMenuSFX.Play()
+    dropdownMenuSFX.play()
 }

[azaleacolburn]

lgtm

[PepperLola]

I think that since we're loading the audio each time we play a sound anyways, it makes more sense to me to make the play function static and take the file path as a parameter, rather than creating a new SoundPlayer every time and calling play immediately after, then discarding the SoundPlayer. Unless we plan on eventually keeping the SoundPlayer around for longer and calling play multiple times, but that doesn't seem to be the case with the implementations of the different ...PressSFX functions. That way we wouldn't need to implement a new function in this file for each type of sound we eventually will want to play, but rather just import the sound effect and call the static SoundPlayer.play(soundPath) function.

[AlexD717]

I think that since we're loading the audio each time we play a sound anyways, it makes more sense to me to make the play function static and take the file path as a parameter, rather than creating a new SoundPlayer every time and calling play immediately after, then discarding the SoundPlayer. Unless we plan on eventually keeping the SoundPlayer around for longer and calling play multiple times, but that doesn't seem to be the case with the implementations of the different ...PressSFX functions. That way we wouldn't need to implement a new function in this file for each type of sound we eventually will want to play, but rather just import the sound effect and call the static SoundPlayer.play(soundPath) function.

If I understand your proposal correctly, wouldn’t it cause an error or unintended behavior when trying to play two sound effects simultaneously? With only one SoundPlayer, if a sound is already playing and we attempt to play another, wouldn’t the first one stop abruptly?

I also think it would be easier to just call a function compared to figuring out where the file is located and sending that each time.

[azaleacolburn]

I think that since we're loading the audio each time we play a sound anyways, it makes more sense to me to make the play function static and take the file path as a parameter, rather than creating a new SoundPlayer every time and calling play immediately after, then discarding the SoundPlayer. Unless we plan on eventually keeping the SoundPlayer around for longer and calling play multiple times, but that doesn't seem to be the case with the implementations of the different ...PressSFX functions. That way we wouldn't need to implement a new function in this file for each type of sound we eventually will want to play, but rather just import the sound effect and call the static SoundPlayer.play(soundPath) function.

If I understand your proposal correctly, wouldn’t it cause an error or unintended behavior when trying to play two sound effects simultaneously? With only one SoundPlayer, if a sound is already playing and we attempt to play another, wouldn’t the first one stop abruptly?

I also think it would be easier to just call a function compared to figuring out where the file is located and sending that each time.

Why would calling a function cause more issues than re-instantiating a class? There's no underlying memory that @PepperLola's SoundPlayer class would need to manipulate, so there's no change of a race condition. Plus if you're going to be doing something over a over, calling a function is probably more svelte than an entire new class instance.

[Dhruv-0-Arora]

merge conflicts

[PepperLola]

I think that since we're loading the audio each time we play a sound anyways, it makes more sense to me to make the play function static and take the file path as a parameter, rather than creating a new SoundPlayer every time and calling play immediately after, then discarding the SoundPlayer. Unless we plan on eventually keeping the SoundPlayer around for longer and calling play multiple times, but that doesn't seem to be the case with the implementations of the different ...PressSFX functions. That way we wouldn't need to implement a new function in this file for each type of sound we eventually will want to play, but rather just import the sound effect and call the static SoundPlayer.play(soundPath) function.

If I understand your proposal correctly, wouldn’t it cause an error or unintended behavior when trying to play two sound effects simultaneously? With only one SoundPlayer, if a sound is already playing and we attempt to play another, wouldn’t the first one stop abruptly?

I also think it would be easier to just call a function compared to figuring out where the file is located and sending that each time.

EDIT: I didn't see that the design was updated before I commented this, so in this comment when I say the "current design" I mean the one where a new SoundPlayer instance was created each time.

Adding on to what Azalea said, the way it's setup right now, if we were to replace the instantiation of the new SoundPlayer instance with the code that actually gets run in the constructor, you would be creating a new Audio instance and then calling play. This wouldn't be any different from calling a function (instead of a constructor) to do the same. Either way you would end up with multiple different instances of Audio that would not conflict. I believe your concerns would be correct were we to try to share the same Audio instance between multiple sound plays, since I think one Audio instance can only play once at a time.

Regarding figuring out where the file is located - I do agree that it would be a bit more effort since you would need to type the whole path, but I think it's only marginally more complicated. It makes more sense to me to maybe import the sound effects in SoundPlayer.ts and then export them from there, so a developer would only need to import sound effects from there (i.e. import { dropdownMenuSound } from "@/systems/sound/SoundPlayer")? If you manually import the sound effect everywhere you want to play it, it means developers would need to either remember or figure out where the sound effect files are every time they want to add a specific sound effect to a new file, but you would also not need to rely on a developer adding a sound effect to create a new function to play that sound effect (which would make the scope of a PR using the new sound effect smaller if the developer were to forget). If we exported the sound effects from SoundPlayer.ts, adding a new sound effect would require importing and exporting it in that file before it can be used elsewhere but it would also make importing the sound effect easier for future devs. With the current implementation, adding a new sound effect requires importing the sound effect in SoundPlayer.ts and creating a new function to play it, which is more effort for a developer adding a new sound effect than the previous design would require. I think the effort for a future developer is only marginally more difficult with the previous design since you would have to import one more variable. I think the latter two designs are about equivalent when it comes to developer effort adding and using sound effects.

The last thing to consider is that with the current implementation, if we were to add a longer-running sound effect and the user decided to change their SFX volume in the middle of it, we would have no way of changing the volume while the sound effect is playing so it would continue at the previous volume until we play it again. With the static play function, it would be pretty easy to store a list of the currently running Audio instances and expose a function to update the volume of each when the user changes their volume setting. I don't think this change would be as easy with the current implementation. I think we should expect to add many more sound effects of varying lengths (e.g. match sfx, robot collisions, maybe even music eventually) and design the sound system to support those in advance.

This is all obviously subjective, as the current implementation works (except for the issue regarding volume) so it's all up for debate, and I will defer to anyone who has been or will be working on match mode since they will be the ones to use the sound system more extensively.

[BrandonPacewic]

Standup Notes:

1. Adjust currently playing sound volume in the middle of a sound.
2. Change sound to rising edge and see how that feels (can reevaluate before merging).

[rutmanz]

lgtm but needs to update from dev

[BrandonPacewic]

I quite like how this is right now. Making a note to experiment with having a sound play at both the rising and falling edge (AARD-1930). Perhaps the first and second half of the current sound respectively? Definitely out of scope of this though.

Sounds good to me 🔈 (Get it?)

[BrandonPacewic]

@PepperLola @Dhruv-0-Arora

[PepperLola]

Nice, I like it

Requested changes were implemented