Bomb Direction #2435
Bomb Direction #2435
Conversation
WalkthroughAdds a rocketDirectionUp toggle (KeyU) and threads it through input events, UI state, build intents, transport/schema, execution, and parabola pathfinding; renderer and preview layers use the flag to flip nuke arc orientation and invalidate cached trajectories. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant InputHandler
participant EventBus
participant NukePreview as NukeTrajectoryPreviewLayer
participant UIState
participant Parabola as ParabolaPathFinder
participant ExecMgr as ExecutionManager
participant NukeExec as NukeExecution
Player->>InputHandler: KeyU (keyup)
InputHandler->>EventBus: emit SwapRocketDirectionEvent
EventBus->>NukePreview: deliver SwapRocketDirectionEvent
NukePreview->>UIState: toggle rocketDirectionUp
NukePreview->>NukePreview: invalidate cached trajectory
NukePreview->>Parabola: computeControlPoints(orig,dst,..., directionUp=UIState.rocketDirectionUp)
Parabola-->>NukePreview: control points
Note over ExecMgr,NukeExec: Build intent carries rocketDirectionUp
ExecMgr->>NukeExec: new NukeExecution(..., rocketDirectionUp)
NukeExec->>Parabola: computeControlPoints(..., directionUp=rocketDirectionUp)
Parabola-->>NukeExec: control points
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Organization 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 (4)
tests/InputHandler.test.ts (1)
40-40: Consider adding tests for SwapRocketDirectionEvent.The
rocketDirectionUp: trueaddition is correct to satisfy the updated UIState interface. However, there are no tests verifying that pressing the swap direction key (KeyU) actually emits a SwapRocketDirectionEvent.Would you like me to generate a test case for the swap direction functionality?
src/client/graphics/layers/StructureIconsLayer.ts (1)
337-347: Extract a shared helper for rocket direction unitsThe same
AtomBomb/HydrogenBombcheck now lives here and inBuildMenu.ts. A tiny helper likeisDirectionalNuke(unitType: UnitType): unitType is UnitType.AtomBomb | UnitType.HydrogenBomb(or a small readonly set) would keep the rule in one place and make future tweaks safer.src/client/graphics/layers/BuildMenu.ts (1)
400-407: Reuse a helper for directional nukesWe now repeat the
AtomBomb/HydrogenBombguard here and inStructureIconsLayer. Pulling it into a small helper (for exampleisDirectionalNuke(unitType)) keeps the list of directional units in one place and stays friendly for future changes.src/client/graphics/GameRenderer.ts (1)
52-56: Prefer type-safeuiStateinitializationCasting with
as UIStatehides missing or extra fields. Declaring the variable with an explicit type (orsatisfies UIState) keeps the compiler on our side:- const uiState = { - attackRatio: 20, - ghostStructure: null, - rocketDirectionUp: true, - } as UIState; + const uiState: UIState = { + attackRatio: 20, + ghostStructure: null, + rocketDirectionUp: true, + };Simple change, but it saves us from silent shape drift later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/client/InputHandler.ts(3 hunks)src/client/Transport.ts(2 hunks)src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/UIState.ts(1 hunks)src/client/graphics/layers/BuildMenu.ts(3 hunks)src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts(4 hunks)src/client/graphics/layers/StructureIconsLayer.ts(1 hunks)src/core/Schemas.ts(1 hunks)src/core/execution/ConstructionExecution.ts(2 hunks)src/core/execution/ExecutionManager.ts(1 hunks)src/core/execution/NukeExecution.ts(2 hunks)src/core/pathfinding/PathFinding.ts(2 hunks)tests/InputHandler.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/NukeExecution.tssrc/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/execution/ExecutionManager.tssrc/core/execution/ConstructionExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/execution/ConstructionExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/execution/ExecutionManager.tssrc/core/execution/ConstructionExecution.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 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/core/execution/NukeExecution.tssrc/core/execution/ConstructionExecution.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 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/core/execution/ConstructionExecution.ts
🧬 Code graph analysis (7)
src/client/InputHandler.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/client/graphics/layers/BuildMenu.ts (2)
src/client/graphics/UIState.ts (1)
UIState(3-7)src/client/Transport.ts (1)
BuildUnitIntentEvent(88-94)
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (2)
src/client/graphics/UIState.ts (1)
UIState(3-7)src/client/InputHandler.ts (1)
SwapRocketDirectionEvent(92-92)
src/core/execution/ExecutionManager.ts (1)
src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(21-160)
src/client/graphics/GameRenderer.ts (2)
src/client/graphics/UIState.ts (1)
UIState(3-7)src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (1)
NukeTrajectoryPreviewLayer(19-275)
src/client/graphics/layers/StructureIconsLayer.ts (1)
src/client/Transport.ts (1)
BuildUnitIntentEvent(88-94)
src/core/execution/ConstructionExecution.ts (1)
src/core/execution/NukeExecution.ts (1)
NukeExecution(19-338)
🔇 Additional comments (11)
src/client/InputHandler.ts (2)
92-93: LGTM!Event definition follows the established pattern for parameterless events in this codebase.
205-205: LGTM!Keybind definition follows the existing pattern and can be customized by users via localStorage.
src/client/graphics/UIState.ts (1)
6-6: LGTM!Adding
rocketDirectionUpto the UIState interface is the correct approach for shared UI state. The property is appropriately typed and named.src/core/execution/ExecutionManager.ts (1)
108-113: LGTM!The
rocketDirectionUpparameter is correctly threaded from the intent to the ConstructionExecution. The optional parameter will beundefinedwhen not provided, which aligns with the ConstructionExecution constructor signature.src/core/execution/ConstructionExecution.ts (2)
34-34: LGTM!The optional
rocketDirectionUpparameter is correctly added to the constructor, providing backward compatibility when the parameter is not provided.
108-116: No changes needed—MIRV direction control is intentional design.MirvExecution deliberately accepts only
(player, dst)parameters with no direction control, unlike NukeExecution which acceptsrocketDirectionUp. This is by design: AtomBomb and HydrogenBomb use a single directional arc (up or down), while MIRV uses intelligent dispersal across multiple targets in a scatter pattern. Both weapon types work as intended.src/core/Schemas.ts (1)
305-305: LGTM!The optional
rocketDirectionUpfield is correctly added to the BuildUnitIntentSchema. Making it optional ensures backward compatibility with existing code that doesn't provide this parameter.src/client/Transport.ts (2)
92-92: LGTM!The optional
rocketDirectionUpparameter is correctly added to BuildUnitIntentEvent, maintaining backward compatibility for existing callers.
553-553: LGTM!The
rocketDirectionUpparameter is correctly propagated from the event to the intent payload. When undefined, this will result in the field being omitted from the payload or serialized as undefined, which aligns with the optional schema definition.src/core/execution/NukeExecution.ts (2)
33-33: LGTM!The default value of
trueforrocketDirectionUppreserves the existing upward trajectory behavior, ensuring backward compatibility for code paths that don't explicitly provide this parameter.
119-119: LGTM!The
rocketDirectionUpparameter is correctly passed to the pathfinder, enabling direction-aware trajectory computation as intended by this PR.
|
This seems good yea |
HimangshuPronoy
left a comment
HimangshuPronoy
left a comment
There was a problem hiding this comment.
Everything seems good. high quality code.
cheers |
|
I've just added the "U" hotkey instruction to the helpmodal + en.json list as I forgot to do this |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.json(1 hunks)src/client/HelpModal.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 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.
🧬 Code graph analysis (1)
src/client/HelpModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(254-274)
🔇 Additional comments (1)
src/client/HelpModal.ts (1)
58-61: LGTM!The hotkey table row is correctly implemented and follows the established pattern. The translation key properly references the new entry in en.json.
|
Are mobile players able to use this feature? If not, we should probably let them do it. At the very least in the settings as a toggle, similar to terrain view. |
They cannot use this, do you think it should be usable by them? They have hardly any space to do stuff as it is |
That's why I was thinking to put it as a toggleable option in the settings menu, instead of as a distinct UI element |
made it a new issue, i think this pr is probably too big as it is |
|
Really looking forward to this feature! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
resources/lang/en.json (1)
134-134: The previous review feedback still applies.The text "Atom / Hydrogen bomb arc direction" remains passive. As noted in the earlier review, other hotkey descriptions use action verbs ("Open build menu", "Alternate view"). Consider rephrasing to something like "Toggle bomb arc direction" or "Switch nuke arc direction (up/down)" to match the active voice used elsewhere.
Additionally, the earlier question about whether MIRV trajectories are affected remains unaddressed. If MIRV is included, the text should reflect that.
src/client/graphics/layers/BuildMenu.ts (1)
400-407: This logic duplicates the pattern in StructureIconsLayer.ts.The same concerns apply here:
- The rocketDirectionUp conditional logic is duplicated between this file and
StructureIconsLayer.ts(lines 377-380).- Verify whether
UnitType.MIRVshould also be included in the conditional.See the review comments on
StructureIconsLayer.tsfor suggested refactoring and verification steps.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
376-387: Consider extracting the rocketDirectionUp logic into a helper function.This exact pattern appears in both
StructureIconsLayer.tsandBuildMenu.ts. Extracting to a shared helper would reduce duplication and make future unit-type changes easier to maintain.🔎 Example helper function
Create a utility function (e.g., in a shared utils file):
function shouldApplyRocketDirection(unitType: UnitType): boolean { return ( unitType === UnitType.AtomBomb || unitType === UnitType.HydrogenBomb ); }Then use it in both files:
-const rocketDirectionUp = - unitType === UnitType.AtomBomb || unitType === UnitType.HydrogenBomb - ? this.uiState.rocketDirectionUp - : undefined; +const rocketDirectionUp = shouldApplyRocketDirection(unitType) + ? this.uiState.rocketDirectionUp + : undefined;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/core/Schemas.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/InputHandler.test.ts
- src/core/Schemas.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/client/graphics/layers/BuildMenu.ts (2)
src/client/graphics/UIState.ts (1)
UIState(3-7)src/client/Transport.ts (1)
BuildUnitIntentEvent(90-96)
src/client/graphics/layers/StructureIconsLayer.ts (3)
src/client/Transport.ts (1)
BuildUnitIntentEvent(90-96)src/core/game/UnitImpl.ts (1)
tile(178-180)src/core/game/GameView.ts (1)
tile(110-112)
🔇 Additional comments (3)
resources/lang/en.json (1)
133-133: LGTM!The trailing comma follows JSON best practices for multi-property objects.
src/client/graphics/layers/StructureIconsLayer.ts (1)
377-380: Verify whether MIRV should also use rocketDirectionUp.The conditional only checks for
AtomBombandHydrogenBomb. Looking at the build table,UnitType.MIRVis also a rocket-based nuke weapon. Should MIRV trajectories also respect the direction toggle?If MIRV should be included, consider this adjustment:
🔎 Proposed fix to include MIRV
const rocketDirectionUp = - unitType === UnitType.AtomBomb || unitType === UnitType.HydrogenBomb + unitType === UnitType.AtomBomb || + unitType === UnitType.HydrogenBomb || + unitType === UnitType.MIRV ? this.uiState.rocketDirectionUp : undefined;src/client/graphics/layers/BuildMenu.ts (1)
25-25: LGTM!The
UIStateimport and property addition integrate cleanly with the existing code structure.Also applies to: 129-129
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
7 participants
Resolves #2434
Description:
Allows bomb direction to be inverted by pressing a hotkey - currently "U".
Check the issue for screenshots / videos.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n