Add music to main menu + moving backround on main menu #2301
Conversation
WalkthroughAdds main-menu music playback and volume controls: new locale keys, UserSettings getter/setter for main-menu volume, a MenuSoundManager singleton to play/stop/cycle menu tracks, client integration to start/stop and apply volume, plus a subtle background animation change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant AppMain as Main
participant Settings as UserSettings
participant MenuSM as MenuSoundManager
participant Howler
User->>AppMain: start app
AppMain->>Settings: mainMenuMusicVolume()
Settings-->>AppMain: 0.2
AppMain->>MenuSM: setBackgroundMusicVolume(0.2)
MenuSM->>Howler: set volume on tracks
AppMain->>MenuSM: playBackgroundMusic()
MenuSM->>Howler: play current track
Howler-->>MenuSM: onend
MenuSM->>MenuSM: playNext() -> advance index
MenuSM->>Howler: play next track
sequenceDiagram
autonumber
participant User
participant Slider
participant Modal as UserSettingModal
participant Settings as UserSettings
participant MenuSM as MenuSoundManager
User->>Slider: adjust (0–100)
Slider->>Modal: CustomEvent { value }
Modal->>Modal: onMenuVolumeChange(value)
Modal->>Settings: setMainMenuMusicVolume(value/100)
Modal->>MenuSM: setBackgroundMusicVolume(value/100)
MenuSM-->>User: audible volume change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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: 4
🧹 Nitpick comments (4)
src/client/Main.ts (1)
514-514: Consider: Should music resume when returning to main menu?The music stops when joining a lobby, but there's no code to resume it when the player exits back to the main menu. Consider whether music should restart in
handleLeaveLobby()(line 578).If desired, add to
handleLeaveLobby:private async handleLeaveLobby(/* event: CustomEvent */) { if (this.gameStop === null) { return; } console.log("leaving lobby, cancelling game"); this.gameStop(); this.gameStop = null; this.gutterAds.hide(); this.publicLobby.leaveLobby(); // Resume menu music MenuSoundManager.setBackgroundMusicVolume( this.userSettings.mainMenuMusicVolume(), ); MenuSoundManager.playBackgroundMusic(); }src/client/index.html (1)
62-82: Performance consideration: Animating background-position can impact performance.The background animation continuously moves a 200% scaled image over 200 seconds. On lower-end devices or mobile, animating
background-positioncan cause repaints and affect performance. Consider using CSStransformwithwill-changefor better GPU acceleration.More performant alternative:
.bg-image { background-size: 200%; background-position: 0% 50%; transform: translateX(0); will-change: transform; animation: move-background 200s linear infinite; } @keyframes move-background { 0% { transform: translateX(0); } 50% { transform: translateX(-50%); } 100% { transform: translateX(0); } }However, with
transform, you'll need to adjust the implementation sincebackground-positionandtransformwork differently for backgrounds. The current implementation is acceptable if performance is not a concern.src/client/sound/MenuSoundManager.ts (2)
34-41: Consider: Guard against rapid repeated calls.If
playBackgroundMusic()is called multiple times rapidly, theplaying()check might have race conditions. While the guard prevents playing multiple instances, consider adding a lock or state flag for better safety.Example with explicit state tracking:
private isPlaying: boolean = false; public playBackgroundMusic(): void { if (this.backgroundMusic.length > 0 && !this.isPlaying) { this.isPlaying = true; this.backgroundMusic[this.currentTrack].play(); } } public stopBackgroundMusic(): void { if (this.backgroundMusic.length > 0 && this.isPlaying) { this.backgroundMusic[this.currentTrack].stop(); this.isPlaying = false; } } private playNext(): void { this.isPlaying = false; this.currentTrack = (this.currentTrack + 1) % this.backgroundMusic.length; this.playBackgroundMusic(); }
62-62: Consider: Singleton instantiation could fail if audio context not ready.Exporting a singleton instance created immediately could fail if the browser's audio context isn't ready or if the module loads before DOM is ready. The current implementation works because it's only called after DOMContentLoaded in Main.ts, but this coupling is fragile.
Consider lazy initialization:
class MenuSoundManager { private static instance: MenuSoundManager | null = null; private constructor() { // ... existing constructor code } public static getInstance(): MenuSoundManager { if (!MenuSoundManager.instance) { MenuSoundManager.instance = new MenuSoundManager(); } return MenuSoundManager.instance; } // ... rest of the class } export default MenuSoundManager.getInstance();Or keep it simple but document the initialization dependency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
proprietary/sounds/music/iron-horizon.mp3is excluded by!**/*.mp3proprietary/sounds/music/prelude-to-war.mp3is excluded by!**/*.mp3proprietary/sounds/music/world-divided.mp3is excluded by!**/*.mp3
📒 Files selected for processing (7)
.husky/pre-commit(1 hunks)resources/lang/en.json(1 hunks)src/client/Main.ts(3 hunks)src/client/UserSettingModal.ts(3 hunks)src/client/index.html(1 hunks)src/client/sound/MenuSoundManager.ts(1 hunks)src/core/game/UserSettings.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/UserSettingModal.ts (1)
src/client/LangSelector.ts (1)
translateText(254-274)
🪛 GitHub Actions: 🧪 CI
src/client/Main.ts
[warning] 1-1: Prettier formatting issue detected. Run 'npx prettier --write' to fix code style issues in this file.
src/client/UserSettingModal.ts
[warning] 1-1: Prettier formatting issue detected. Run 'npx prettier --write' to fix code style issues in this file.
src/client/sound/MenuSoundManager.ts
[warning] 1-1: Prettier formatting issue detected. Run 'npx prettier --write' to fix code style issues in this file.
🔇 Additional comments (5)
src/client/UserSettingModal.ts (2)
210-219: Good implementation of volume change handler.The handler correctly:
- Converts slider value (0-100) to volume (0-1)
- Updates persistent storage via
setMainMenuMusicVolume- Applies volume immediately via
MenuSoundManager.setBackgroundMusicVolume- Includes error handling for missing detail.value
288-296: Good: Volume control placed prominently at top of settings.The main menu music slider is positioned first in basic settings, making it easy for users to find and adjust. The translation keys are properly referenced.
resources/lang/en.json (1)
390-393: Good: Clear distinction between menu and in-game music.The language keys now clearly separate "Main Menu Music" from "In-Game Music Volume", improving user understanding. The descriptions are clear and concise.
src/client/sound/MenuSoundManager.ts (2)
49-54: Good: Volume clamping and applying to all tracks.The method correctly clamps volume to [0, 1] range and applies it to all tracks, ensuring consistent behavior.
2-4: Licensing properly documented—no issues to address.The music files are correctly placed in the
/proprietarydirectory with clear licensing terms. LICENSE-ASSETS explicitly distinguishes the three asset categories: open CC BY-SA 4.0 resources, proprietary restricted assets, and external CDN assets. The imported files (iron-horizon.mp3, world-divided.mp3, prelude-to-war.mp3) are intentionally kept proprietary and restricted to OpenFront development use, which is documented in both/proprietary/LICENSEandLICENSE-ASSETS. This mixed licensing approach (AGPL-3.0 code + CC BY-SA 4.0 open assets + restricted proprietary assets) is valid and transparent.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
this is unrelated to the background music, can you put it in another PR? Looks good though
| <!-- Main Menu Music --> | ||
| <setting-slider | ||
| label="${translateText("user_setting.main_menu_music_volume")}" | ||
| description="${translateText( | ||
| "user_setting.main_menu_music_volume_desc", | ||
| )}" | ||
| min="0" | ||
| max="100" | ||
| .value=${this.userSettings.mainMenuMusicVolume() * 100} | ||
| @change=${this.onMenuVolumeChange} | ||
| ></setting-slider> |
There was a problem hiding this comment.
don't we already have a music setting?
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
Description:
Add main menu music+moving bg
Describe the PR.
Add main menu music+moving bg
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Lucas