Implement a "ka-ching" sound effect on kill #2097
Conversation
|
|
WalkthroughAdds sound effects support. Introduces SoundEffect enum and APIs in SoundManager, wires a KaChing effect on ConquestEvent in FxLayer, and adds background music and sound effects volume controls in SettingsModal synced with UserSettings. Extends UserSettings with float getters/setters. Adds one new locale string. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant UI as SettingsModal
participant US as UserSettings
participant SM as SoundManager
Player->>UI: Open settings
UI->>US: Read backgroundMusicVolume(), soundEffectsVolume()
UI->>SM: setBackgroundMusicVolume(v)\nsetSoundEffectsVolume(v)
Player->>UI: Move sliders
UI->>US: setBackgroundMusicVolume(v)\nsetSoundEffectsVolume(v)
UI->>SM: setBackgroundMusicVolume(v)\nsetSoundEffectsVolume(v)
UI-->>Player: Updated values shown (%)
sequenceDiagram
autonumber
participant Game as Game Engine
participant Fx as FxLayer
participant SM as SoundManager
Game->>Fx: Process ConquestEvent
Fx->>SM: playSoundEffect(KaChing)
note right of SM: Plays preloaded effect at current SFX volume
Fx->>Fx: Create conquest FX visuals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/global.d.ts (1)
44-47: Add declarations for wav/ogg imports too.Webpack already treats
.wavand.oggas resources, so TypeScript should know them. Without the declarations, any future import of those files will throw a type error.Apply this diff:
declare module "*.mp3" { const value: string; export default value; } + +declare module "*.wav" { + const value: string; + export default value; +} + +declare module "*.ogg" { + const value: string; + export default value; +}src/client/sound/SoundManager.ts (1)
7-109: Switch to module functions and literal unions.We ship this as a singleton, so the class and enum only add runtime weight. Please flatten everything into plain functions with captured state and expose the effect ids through a const-based string union. That keeps the code compositional and avoids the class/enum hierarchy.
-export enum SoundEffect { - KaChing = "ka-ching", -} - -class SoundManager { - private backgroundMusic: Howl[] = []; - private currentTrack: number = 0; - private soundEffects: Map<SoundEffect, Howl> = new Map(); - private soundEffectsVolume: number = 1; - private backgroundMusicVolume: number = 0; - - constructor() { - this.backgroundMusic = [ - new Howl({ - src: [evan], - loop: false, - onend: this.playNext.bind(this), - volume: 0, - }), - new Howl({ - src: [openfront], - loop: false, - onend: this.playNext.bind(this), - volume: 0, - }), - new Howl({ - src: [war], - loop: false, - onend: this.playNext.bind(this), - volume: 0, - }), - ]; - this.loadSoundEffect(SoundEffect.KaChing, kaChingSound); - } - - public playBackgroundMusic(): void { - if ( - this.backgroundMusic.length > 0 && - !this.backgroundMusic[this.currentTrack].playing() - ) { - this.backgroundMusic[this.currentTrack].play(); - } - } - - public stopBackgroundMusic(): void { - if (this.backgroundMusic.length > 0) { - this.backgroundMusic[this.currentTrack].stop(); - } - } - - public setBackgroundMusicVolume(volume: number): void { - this.backgroundMusicVolume = Math.max(0, Math.min(1, volume)); - this.backgroundMusic.forEach((track) => { - track.volume(this.backgroundMusicVolume); - }); - } - - private playNext(): void { - this.currentTrack = (this.currentTrack + 1) % this.backgroundMusic.length; - this.playBackgroundMusic(); - } - - public loadSoundEffect(name: SoundEffect, src: string): void { - if (!this.soundEffects.has(name)) { - const sound = new Howl({ - src: [src], - volume: this.soundEffectsVolume, - }); - this.soundEffects.set(name, sound); - } - } - - public playSoundEffect(name: SoundEffect): void { - const sound = this.soundEffects.get(name); - if (sound) { - sound.play(); - } - } - - public setSoundEffectsVolume(volume: number): void { - this.soundEffectsVolume = Math.max(0, Math.min(1, volume)); - this.soundEffects.forEach((sound) => { - sound.volume(this.soundEffectsVolume); - }); - } - - public stopSoundEffect(name: SoundEffect): void { - const sound = this.soundEffects.get(name); - if (sound) { - sound.stop(); - } - } - - public unloadSoundEffect(name: SoundEffect): void { - const sound = this.soundEffects.get(name); - if (sound) { - sound.unload(); - this.soundEffects.delete(name); - } - } -} - -export default new SoundManager(); +export const SOUND_EFFECT = { + KaChing: "ka-ching", +} as const; +export type SoundEffect = (typeof SOUND_EFFECT)[keyof typeof SOUND_EFFECT]; + +const soundEffects = new Map<SoundEffect, Howl>(); +let soundEffectsVolume = 1; +let backgroundMusicVolume = 0; +let currentTrack = 0; + +function clampVolume(volume: number): number { + return Math.max(0, Math.min(1, volume)); +} + +function playBackgroundMusic(): void { + if (backgroundMusic.length === 0) { + return; + } + + const track = backgroundMusic[currentTrack]; + if (!track.playing()) { + track.play(); + } +} + +function stopBackgroundMusic(): void { + if (backgroundMusic.length === 0) { + return; + } + + backgroundMusic[currentTrack].stop(); +} + +function setBackgroundMusicVolume(volume: number): void { + backgroundMusicVolume = clampVolume(volume); + backgroundMusic.forEach((track) => { + track.volume(backgroundMusicVolume); + }); +} + +function handleTrackEnd(): void { + if (backgroundMusic.length === 0) { + return; + } + + currentTrack = (currentTrack + 1) % backgroundMusic.length; + playBackgroundMusic(); +} + +function loadSoundEffect(name: SoundEffect, src: string): void { + if (!soundEffects.has(name)) { + const sound = new Howl({ + src: [src], + volume: soundEffectsVolume, + }); + soundEffects.set(name, sound); + } +} + +function playSoundEffect(name: SoundEffect): void { + const sound = soundEffects.get(name); + if (sound) { + sound.play(); + } +} + +function setSoundEffectsVolume(volume: number): void { + soundEffectsVolume = clampVolume(volume); + soundEffects.forEach((sound) => { + sound.volume(soundEffectsVolume); + }); +} + +function stopSoundEffect(name: SoundEffect): void { + const sound = soundEffects.get(name); + if (sound) { + sound.stop(); + } +} + +function unloadSoundEffect(name: SoundEffect): void { + const sound = soundEffects.get(name); + if (sound) { + sound.unload(); + soundEffects.delete(name); + } +} + +const backgroundMusic: Howl[] = [ + new Howl({ + src: [evan], + loop: false, + onend: handleTrackEnd, + volume: backgroundMusicVolume, + }), + new Howl({ + src: [openfront], + loop: false, + onend: handleTrackEnd, + volume: backgroundMusicVolume, + }), + new Howl({ + src: [war], + loop: false, + onend: handleTrackEnd, + volume: backgroundMusicVolume, + }), +]; + +loadSoundEffect(SOUND_EFFECT.KaChing, kaChingSound); + +export default { + playBackgroundMusic, + stopBackgroundMusic, + setBackgroundMusicVolume, + loadSoundEffect, + playSoundEffect, + setSoundEffectsVolume, + stopSoundEffect, + unloadSoundEffect, +};Call sites will need to swap to
SOUND_EFFECT.KaChing(or a similar alias) but will gain a typed union for free.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
package-lock.jsonis excluded by!**/package-lock.jsonresources/images/music.svgis excluded by!**/*.svgresources/sounds/effects/ka-ching.mp3is excluded by!**/*.mp3resources/sounds/music/evan.mp3is excluded by!**/*.mp3resources/sounds/music/of2.mp3is excluded by!**/*.mp3resources/sounds/music/of4.mp3is excluded by!**/*.mp3resources/sounds/music/openfront.mp3is excluded by!**/*.mp3resources/sounds/music/war.mp3is excluded by!**/*.mp3resources/sounds/music/win.mp3is excluded by!**/*.mp3
📒 Files selected for processing (9)
package.json(2 hunks)resources/lang/en.json(1 hunks)src/client/ClientGameRunner.ts(3 hunks)src/client/graphics/layers/FxLayer.ts(2 hunks)src/client/graphics/layers/SettingsModal.ts(4 hunks)src/client/sound/SoundManager.ts(1 hunks)src/core/game/UserSettings.ts(2 hunks)src/global.d.ts(1 hunks)webpack.config.js(1 hunks)
| "@types/howler": "^2.2.12", | ||
| "@types/jest": "^30.0.0", |
There was a problem hiding this comment.
Fix invalid @types/howler version.
@types/howler was never published at 2.2.12, so npm install will fail. Please pin to a shipped release (latest is 2.2.4).
Apply this diff:
- "@types/howler": "^2.2.12",
+ "@types/howler": "^2.2.4",Based on learnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@types/howler": "^2.2.12", | |
| "@types/jest": "^30.0.0", | |
| "@types/howler": "^2.2.4", | |
| "@types/jest": "^30.0.0", |
🤖 Prompt for AI Agents
In package.json around lines 42 to 43, the @types/howler version is invalid
(2.2.12) causing npm install to fail; update the version specifier to a
published release (e.g., "2.2.4") where @types/howler exists. Edit the
dependency entry to pin to "2.2.4" (or a valid semver matching 2.2.4), save
package.json, then run npm install (and regenerate lockfile) to ensure the
correct version is installed.
|
mind resolving conflicts, thanks! |
Will do tonight @evanpelle! |
Resolved conflicts by keeping sound effects feature changes: - Added sound effects volume control in settings - Updated SoundManager with separate volume controls for music and effects - Updated language files with sound effects volume label - Merged with upstream changes including new music tracks (of4.mp3)
GabeK0
force-pushed
the
gabek0/sound-effects
branch
from
74855e5 to
a720615
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/sound/SoundManager.ts (1)
79-106: LGTM: Clean sound effect API.All methods safely handle missing effects. Note that
playSoundEffectallows overlapping plays (multipleplay()calls create multiple howler instances), which is typically desired for effects like ka-ching on rapid kills.Consider adding a JSDoc comment on
playSoundEffectto document the overlapping behavior if it's intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.json(1 hunks)src/client/sound/SoundManager.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🔇 Additional comments (5)
src/client/sound/SoundManager.ts (5)
7-9: LGTM: Clean typed enum.String enum avoids magic strings and provides type safety. Extensible for future effects.
14-16: Consider consistent volume defaults.
soundEffectsVolumedefaults to 1 (full volume) whilebackgroundMusicVolumedefaults to 0 (muted). If background music is muted by default for user preference, should sound effects also start muted?Verify the intended UX: should sound effects be audible immediately, or wait for user opt-in like background music?
18-40: LGTM: Per-track volume management.Correctly initializes each track with
volume: 0instead of relying on globalHowler.volume(). TheKaChingeffect is loaded withsoundEffectsVolume(1.0), which will be synced whenSettingsModalreadsUserSettingsand callssetSoundEffectsVolume.
57-62: LGTM: Volume clamping and per-track updates.Correctly clamps input and applies volume to all tracks, ensuring consistent volume when tracks change.
69-77: LGTM: Safe loading with duplicate check.Guards against reloading and initializes with current
soundEffectsVolume.
|
Ok fixed the conflicts @evanpelle |
There was a problem hiding this comment.
I think this file should be reverted? Same with UserSettings & SoundManager?
There was a problem hiding this comment.
Allow me to explain why I believe the changes to these files are helpful and necessary!
- We should ideally support having separate settings sliders for background music volume and sound effects volume. This is a better UX since many users may want to mute the music but not the effects, etc etc
- Background music volume was previously implemented via
Howler.volume(newVolume);, which sets the volume for all sounds played through Howler. So in order to implement separate settings sliders for background music and sound effects, we'll need to change this. - We do this by adding new logic for a
private backgroundMusicVolume: number = 0;var, in addition to theprivate soundEffectsVolume: number = 1;logic
Hopefully this makes sense, happy to explain anything else or make further changes as needed
|
Looks good! just one more question: where did you find the sound effect? Or did you create it yourself? Want to make sure it's licensed properly. Thanks! |
|
@evanpelle See the PR description, I linked where I found the sound. It's Creative Commons licensed |
whoops, mb. Thanks! |
Description:
Building off of this PR which implemented music, I extend this functionality to add sound effects. Diff will be reduced if and when that PR gets merged!
I think the game would benefit from more sound effects, and adding a "ka-ching" sound effect on kill seems like an easy place to start.
The ka-ching sound effect was found here and is licensed under Creative Commons.
Please complete the following:
Demo video with sound:
https://github.com/user-attachments/assets/18c857a4-a741-492a-bbc1-68d4f3ba38da
Please put your Discord username so you can be contacted if a bug or regression is found:
basedgob