sound effects #2140
sound effects #2140
Conversation
WalkthroughAdds a new instance-based sound system. Introduces SoundLayer for nuke launch/hit sounds, expands SoundManager with more effects and volumes, and removes default singleton export. Wires SoundManager and SoundLayer into ClientGameRunner, GameRenderer, and SettingsModal. Adjusts FxLayer to avoid duplicate nuke launches and removes a conquest sound. Fixes a LocalServer hash guard. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ClientGameRunner
participant SoundManager
participant SoundLayer
participant GameRenderer
participant SettingsModal
User->>ClientGameRunner: start()
ClientGameRunner->>SoundManager: new SoundManager()
ClientGameRunner->>SoundLayer: new SoundLayer(game, soundManager)
ClientGameRunner->>GameRenderer: createRenderer(canvas, game, bus, soundManager, soundLayer)
GameRenderer->>SettingsModal: settingsModal.soundManager = soundManager
ClientGameRunner->>SoundManager: playBackgroundMusic()
sequenceDiagram
autonumber
participant TickLoop as Game Tick
participant SoundLayer
participant GameView
participant SoundManager
TickLoop->>SoundLayer: tick()
SoundLayer->>GameView: get units updated since last tick
loop For each updated unit
SoundLayer->>SoundLayer: onUnitEvent(unit)
alt Nuke launch (owned/targeting player) and not seen
SoundLayer->>SoundManager: play(AtomLaunch|HydroLaunch|MirvLaunch)
SoundLayer->>SoundLayer: mark unit as seen
end
alt Nuke reached target (relevant)
SoundLayer->>SoundManager: play(AtomHit|HydroHit|MirvHit)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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)
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: 1
🧹 Nitpick comments (1)
src/client/sound/SoundManager.ts (1)
55-55: Reusing atom hit sound for MIRV.Line 55 loads
atomHitSoundforSoundEffect.MirvHitinstead of a dedicated MIRV hit sound. If this is intentional (to save on asset size or because they should sound similar), consider adding a comment explaining the reuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
proprietary/sounds/effects/Atom_Hit.mp3is excluded by!**/*.mp3proprietary/sounds/effects/atom_launch.mp3is excluded by!**/*.mp3proprietary/sounds/effects/click.mp3is excluded by!**/*.mp3proprietary/sounds/effects/hydrogen_hit.mp3is excluded by!**/*.mp3proprietary/sounds/effects/hydrogen_launch.mp3is excluded by!**/*.mp3proprietary/sounds/effects/mirv_launch.mp3is excluded by!**/*.mp3
📒 Files selected for processing (3)
src/client/LocalServer.ts(1 hunks)src/client/graphics/layers/FxLayer.ts(3 hunks)src/client/sound/SoundManager.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/client/graphics/layers/FxLayer.ts
🪛 GitHub Actions: 🧪 CI
src/client/sound/SoundManager.ts
[error] 2-76: Can't resolve '../../../proprietary/sounds/effects/atom_hit.mp3' in '/home/runner/work/OpenFrontIO/OpenFrontIO/src/client/sound'
🪛 GitHub Actions: 🚀 Deploy
src/client/sound/SoundManager.ts
[error] 2-76: Module not found: Can't resolve '../../../proprietary/sounds/effects/atom_hit.mp3' in '/usr/src/app/src/client/sound' (webpack: Can't resolve the MP3 file; the asset may be missing or path wrong).
🔇 Additional comments (4)
src/client/LocalServer.ts (1)
112-114: Good defensive guard.The check prevents assigning a hash to an undefined turn, which could happen if the hash message arrives before the turn is created in the array.
src/client/graphics/layers/FxLayer.ts (3)
145-165: Launch sound tracking looks correct.The per-unit de-duplication via
seenNukesensures each nuclear unit plays its launch sound only once. The distinction between MIRV (launcher) and MIRVWarhead (actual warhead) is intentional - MIRV only plays launch sound while MIRVWarhead triggers the full nuke event.
283-289: Hit sound logic is correct.Playing specific hit sounds based on the nuclear unit type (AtomHit, HydroHit, MirvHit) provides appropriate audio feedback for each explosion type.
22-22: Verify clearing ofseenNukes. The Set on line 22 is never cleared and may grow unbounded if unit IDs stay unique across sessions. Confirm whether IDs reset or ifFxLayeris reinitialized on game restart; otherwise clearseenNukeswhen the game state resets.
There was a problem hiding this comment.
Few comments:
-
I think we should only play nuke effect if you are launching the nuke or the nuke is targeting your territory.
-
core/ is just a simulation, it should have no dependencies on graphics. So I think it's best to create a SoundLayer that listens for updates. (similar to how EventsDisplay works). And we should probably not have SoundManager be a singleton.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/sound/SoundManager.ts (1)
2-6: Critical: Import path case mismatch still blocking build.The past review comment correctly identified that the import path on line 2 uses
atom_hit.mp3but the actual file is namedAtom_Hit.mp3. This case mismatch prevents the module from resolving and causes the build to fail. Please update the import to match the exact filename casing.Apply this diff to fix the import path:
-import atomHitSound from "../../../proprietary/sounds/effects/atom_hit.mp3"; +import atomHitSound from "../../../proprietary/sounds/effects/Atom_Hit.mp3";
🧹 Nitpick comments (1)
src/client/sound/SoundLayer.ts (1)
31-64: Consider mapping unit types to sound effects with an object.The launch sound logic is correct but uses multiple if-else statements to map unit types to sound effects. A map-based approach would be cleaner and easier to extend.
Apply this diff to use a typed map:
onUnitEvent(unit: UnitView) { const myPlayer = this.game.myPlayer(); if (!myPlayer) return; const isNuke = unit.type() === UnitType.AtomBomb || unit.type() === UnitType.HydrogenBomb || unit.type() === UnitType.MIRV; if (isNuke) { if (!this.seenNukes.has(unit.id())) { const owner = unit.owner(); const targetTile = unit.targetTile(); const targetPlayer = targetTile ? this.game.playerBySmallID(this.game.ownerID(targetTile)) : undefined; if (owner === myPlayer || targetPlayer === myPlayer) { - let soundEffect: SoundEffect | undefined; - if (unit.type() === UnitType.AtomBomb) { - soundEffect = SoundEffect.AtomLaunch; - } else if (unit.type() === UnitType.HydrogenBomb) { - soundEffect = SoundEffect.HydroLaunch; - } else if (unit.type() === UnitType.MIRV) { - soundEffect = SoundEffect.MirvLaunch; - } - - if (soundEffect) { - this.soundManager.playSoundEffect(soundEffect); - } + const launchSounds: Record<UnitType, SoundEffect | undefined> = { + [UnitType.AtomBomb]: SoundEffect.AtomLaunch, + [UnitType.HydrogenBomb]: SoundEffect.HydroLaunch, + [UnitType.MIRV]: SoundEffect.MirvLaunch, + }; + const soundEffect = launchSounds[unit.type()]; + if (soundEffect) { + this.soundManager.playSoundEffect(soundEffect); + } } this.seenNukes.add(unit.id()); } }Apply the same pattern to the hit sound logic at lines 80-91.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
proprietary/sounds/effects/atom_hit.mp3is excluded by!**/*.mp3
📒 Files selected for processing (7)
.husky/pre-commit(1 hunks)src/client/ClientGameRunner.ts(6 hunks)src/client/graphics/GameRenderer.ts(6 hunks)src/client/graphics/layers/FxLayer.ts(3 hunks)src/client/graphics/layers/SettingsModal.ts(4 hunks)src/client/sound/SoundLayer.ts(1 hunks)src/client/sound/SoundManager.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/FxLayer.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/client/ClientGameRunner.ts (3)
src/client/sound/SoundManager.ts (1)
SoundManager(22-124)src/client/sound/SoundLayer.ts (1)
SoundLayer(7-99)src/client/graphics/GameRenderer.ts (1)
createRenderer(45-294)
src/client/graphics/GameRenderer.ts (2)
src/client/sound/SoundManager.ts (1)
SoundManager(22-124)src/client/sound/SoundLayer.ts (1)
SoundLayer(7-99)
src/client/sound/SoundLayer.ts (3)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (4)
GameView(423-754)unit(634-636)UnitView(44-175)myPlayer(568-570)src/client/sound/SoundManager.ts (1)
SoundManager(22-124)
src/client/graphics/layers/SettingsModal.ts (1)
src/client/sound/SoundManager.ts (1)
SoundManager(22-124)
⏰ 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). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (7)
src/client/sound/SoundManager.ts (1)
22-57: Good refactoring from singleton to exported class.The change from a default export of an instantiated object to an exported class improves testability and allows multiple instances if needed. The dependency injection pattern used throughout the codebase now has proper control over sound manager lifecycle.
src/client/ClientGameRunner.ts (2)
179-202: LGTM: Clean dependency injection.The sound management integration follows proper dependency injection. Creating
soundManagerandsoundLayerinstances at the client game creation level and passing them through to the renderer ensures clean separation of concerns and testability.
259-259: LGTM: Instance-based sound control.Background music is now controlled through the injected
soundManagerinstance rather than static calls. This provides better lifecycle management and aligns with the dependency injection pattern used throughout the codebase.Also applies to: 388-388
src/client/graphics/layers/SettingsModal.ts (1)
18-18: LGTM: Instance-based sound management.The migration from static
SoundManagerusage to instance-based control is clean. ThesoundManagerfield is properly injected (wired inGameRenderer.createRenderer()) and used consistently throughout volume control methods.Also applies to: 33-33, 51-56, 165-173
src/client/graphics/GameRenderer.ts (1)
45-51: LGTM: Sound subsystem properly wired through renderer.The sound integration is clean:
soundManagerandsoundLayerare passed as parameters tocreateRenderer()settingsModal.soundManageris assigned for volume controlsoundLayeris added to the layers array for sound effectsGameRendererstores thesoundManagerreferenceThis architecture allows sound management to participate in the rendering lifecycle while maintaining separation of concerns.
Also applies to: 167-167, 281-281, 292-293, 307-307
src/client/sound/SoundLayer.ts (2)
7-29: LGTM: Clean Layer implementation.The
SoundLayerproperly implements theLayerinterface. UsingshouldTransform(): falseis correct since sound logic doesn't need canvas transformations. Thetick()method efficiently processes only units updated since the last tick.
66-94: Hit sound logic looks correct.The hit sound logic properly checks that the unit is inactive and reached its target before playing the sound. Since inactive units are deleted after the update (as shown in
GameView.update()), duplicate hit sounds shouldn't occur.The comparison
owner === myPlayer || targetPlayer === myPlayercorrectly handles the case wheretargetPlayermight beTerraNullius(the comparison would be false, which is the desired behavior).
FIXED! |
| if (this.turns[clientMsg.turnNumber]) { | ||
| this.turns[clientMsg.turnNumber].hash = clientMsg.hash; | ||
| } |
There was a problem hiding this comment.
Is this change necessary?
| import { UnitExplosionFx } from "../fx/UnitExplosionFx"; | ||
| import { Layer } from "./Layer"; | ||
| export class FxLayer implements Layer { | ||
| private seenNukes: Set<number> = new Set(); |
| this.onNukeEvent(unit, 70); | ||
| break; | ||
| case UnitType.MIRV: | ||
| break; |
There was a problem hiding this comment.
is this related to sounds?
| export enum SoundEffect { | ||
| KaChing = "ka-ching", | ||
| AtomLaunch = "atom_launch", | ||
| AtomHit = "atom_hit", | ||
| HydroLaunch = "hydro_launch", | ||
| HydroHit = "hydro_hit", | ||
| MirvHit = "mirv_hit", | ||
| MirvLaunch = "mirv_launch", | ||
| } |
There was a problem hiding this comment.
we should try to avoid enums, instead do:
export const SoundEffect = {
KaChing: "ka-ching",
AtomLaunch: "atom_launch",
AtomHit: "atom_hit",
HydroLaunch: "hydro_launch",
HydroHit: "hydro_hit",
MirvHit: "mirv_hit",
MirvLaunch: "mirv_launch",
} as const;
export type SoundEffect = typeof SoundEffect[keyof typeof SoundEffect];
| unit.type() === UnitType.AtomBomb || | ||
| unit.type() === UnitType.HydrogenBomb || | ||
| unit.type() === UnitType.MIRV; | ||
|
|
There was a problem hiding this comment.
return early do:
if(!isNuke) {
return
}
| if (owner === myPlayer || targetPlayer === myPlayer) { | ||
| let soundEffect: SoundEffect | undefined; | ||
| if (unit.type() === UnitType.AtomBomb) { | ||
| soundEffect = SoundEffect.AtomLaunch; | ||
| } else if (unit.type() === UnitType.HydrogenBomb) { | ||
| soundEffect = SoundEffect.HydroLaunch; | ||
| } else if (unit.type() === UnitType.MIRV) { | ||
| soundEffect = SoundEffect.MirvLaunch; | ||
| } | ||
|
|
||
| if (soundEffect) { | ||
| this.soundManager.playSoundEffect(soundEffect); | ||
| } |
There was a problem hiding this comment.
this can be simplified:
switch(unit.type()) {
case UnitType.AtomBomb:
this.soundManager.playerSoundEffect(SoundEffect.AtomLaunch)
| const isNukeHit = | ||
| unit.type() === UnitType.AtomBomb || | ||
| unit.type() === UnitType.HydrogenBomb || | ||
| unit.type() === UnitType.MIRVWarhead; |
There was a problem hiding this comment.
we already have the isNuke variable, reuse that.
| const owner = unit.owner(); | ||
| const targetTile = unit.lastTile(); | ||
| const targetPlayer = this.game.playerBySmallID( | ||
| this.game.ownerID(targetTile), |
There was a problem hiding this comment.
this.game.owner(targetTile)
| const owner = unit.owner(); | ||
| const targetTile = unit.targetTile(); | ||
| const targetPlayer = targetTile | ||
| ? this.game.playerBySmallID(this.game.ownerID(targetTile)) |
There was a problem hiding this comment.
use this.game.owner(targetTile)
| if (this.turns[clientMsg.turnNumber]) { | ||
| this.turns[clientMsg.turnNumber].hash = clientMsg.hash; | ||
| } |
Description:
add sound effects for bombs
Describe the PR.
sound effects for BOMBS
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Lucas