Nuke preview #1814
Nuke preview #1814
Conversation
|
|
WalkthroughAdds a new NukePreview rendering layer and UIState fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu as RadialMenu/BuildMenu
participant State as UIState
participant Renderer as GameRenderer
participant NP as NukePreview
User->>Menu: hover nuclear menu item
Menu->>State: set nukePreview = {active:true, nukeType}
Menu->>Menu: dim overlay
Renderer->>NP: renderLayer(ctx)
NP->>State: read nukePreview and nukeAnchor
NP->>NP: compute radii or MIRV targets (seeded)
NP->>Renderer: draw preview visuals
User-->>Menu: hover leave / hide
Menu->>State: nukePreview = undefined
Menu->>Menu: undim overlay
sequenceDiagram
participant GameLoop
participant Layers
participant NukePreview
loop each frame
GameLoop->>Layers: renderLayer(ctx)
alt nukePreview active
Layers->>NukePreview: renderLayer(ctx)
NukePreview->>NukePreview: compute & cache targets/radii
NukePreview->>GameLoop: draw previews
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/client/graphics/layers/PlayerActionHandler.ts (1)
38-40: Optional: Clear nukeAnchor on stop to avoid stale anchorsIf the menu closes while previewing, you can end up with a leftover anchor. Consider also clearing
nukeAnchorwhen stopping the preview. If other flows rely on keeping the last anchor, ignore this.For clarity, update as follows (outside the shown segment):
stopNukePreview() { this.uiState.nukePreview = undefined; + this.uiState.nukeAnchor = undefined; }src/client/graphics/GameRenderer.ts (1)
52-52: Annotate uiState and drop redundant undefinedPrefer an explicit type and omit optional fields when not set. This makes the intent clearer and avoids accidental shape drift.
Apply this diff:
- const uiState = { attackRatio: 20, nukePreview: undefined }; + const uiState: UIState = { attackRatio: 20 };tests/client/graphics/RadialMenuElements.test.ts (1)
515-579: Great coverage for hover-driven nuke previewsThe tests assert isNuke, set/clear on hover, and exclude non-nukes. Consider adding a Hydrogen Bomb case to cover the full nuclear set.
src/client/graphics/layers/BuildMenu.ts (1)
372-378: Duplicate helper function across files.The
isNukeTypehelper is duplicated in both BuildMenu.ts and RadialMenuElements.ts. Consider extracting this to a shared utility.Would you like me to create a shared utility file for nuclear-related helpers to avoid duplication?
src/client/graphics/layers/NukePreview.ts (1)
189-189: Type assertion could be improved.The code uses
(anchorOwner as any).id()which bypasses TypeScript's type checking. Consider using a proper type guard.- const sig = `MIRV|${ax}|${ay}|${anchorOwner.isPlayer() ? (anchorOwner as any).id() : "TN"}`; + const sig = `MIRV|${ax}|${ay}|${anchorOwner.isPlayer() ? (anchorOwner as PlayerView).id() : "TN"}`;Note: You'll need to import
PlayerViewfrom the appropriate module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/client/graphics/GameRenderer.ts(4 hunks)src/client/graphics/UIState.ts(1 hunks)src/client/graphics/layers/BuildMenu.ts(8 hunks)src/client/graphics/layers/MainRadialMenu.ts(2 hunks)src/client/graphics/layers/NukePreview.ts(1 hunks)src/client/graphics/layers/PlayerActionHandler.ts(2 hunks)src/client/graphics/layers/RadialMenu.ts(6 hunks)src/client/graphics/layers/RadialMenuElements.ts(6 hunks)tests/client/graphics/NukePreview.test.ts(1 hunks)tests/client/graphics/RadialMenuElements.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/PlayerActionHandler.tssrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
tests/client/graphics/RadialMenuElements.test.tssrc/client/graphics/layers/RadialMenu.tssrc/client/graphics/layers/MainRadialMenu.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/BuildMenu.tssrc/client/graphics/layers/RadialMenuElements.ts
🧬 Code Graph Analysis (7)
tests/client/graphics/NukePreview.test.ts (1)
src/client/graphics/layers/NukePreview.ts (1)
NukePreview(8-292)
src/client/graphics/layers/NukePreview.ts (6)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (1)
GameView(360-692)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-296)src/client/graphics/UIState.ts (1)
UIState(1-8)src/core/StatsSchemas.ts (1)
NukeType(11-15)src/core/game/Game.ts (1)
Cell(285-307)
tests/client/graphics/RadialMenuElements.test.ts (2)
src/core/game/GameView.ts (1)
PlayerView(180-358)src/client/graphics/layers/RadialMenuElements.ts (1)
attackMenuElement(428-439)
src/client/graphics/layers/RadialMenu.ts (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
MenuElementParams(29-43)
src/client/graphics/layers/BuildMenu.ts (1)
src/client/graphics/UIState.ts (1)
UIState(1-8)
src/client/graphics/layers/RadialMenuElements.ts (2)
src/client/graphics/UIState.ts (1)
UIState(1-8)src/client/graphics/layers/BuildMenu.ts (1)
isNukeType(372-378)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/NukePreview.ts (1)
NukePreview(8-292)
🔇 Additional comments (16)
src/client/graphics/layers/MainRadialMenu.ts (1)
133-134: Passing uiState to radial params is correctThis enables hover-driven preview toggles in menu items and keeps state shared across layers. Looks good.
src/client/graphics/GameRenderer.ts (3)
26-26: Adding NukePreview to the renderer is correctImport and registration look good and match the new layer’s constructor.
76-77: Wiring uiState into BuildMenu is correctEnsures build menu can dim and drive previews. Good.
274-275: Layer order: NukePreview after world layers is sensibleRendering the preview after terrain/units ensures it overlays the map while DOM-based UI stays above the canvas. Good placement.
tests/client/graphics/RadialMenuElements.test.ts (1)
150-155: Test params shape for uiState is fineStubbed UI state matches what the radial elements expect.
src/client/graphics/layers/RadialMenu.ts (3)
378-382: LGTM! Clean dimming implementation for nuclear previews.The
setMenuDimmethod provides a simple, focused way to dim the radial menu during nuclear hover states. The opacity value and transition timing align well with the build menu's dimming behavior.
408-409: Good integration with nuclear preview system.The hover enter logic correctly triggers dimming and invokes the item's hover callback when
isNukeis true, enabling coordinated preview rendering.
789-789: Good cleanup of dim state on menu hide.Properly resetting the dim state when hiding the menu ensures the UI remains consistent.
tests/client/graphics/NukePreview.test.ts (1)
1-144: Well-structured test coverage for nuclear preview rendering.The tests effectively verify both Atom Bomb and MIRV rendering paths with proper mocking and assertions. The deterministic time mocking ensures stable dash pattern calculations across test runs.
src/client/graphics/layers/RadialMenuElements.ts (2)
347-353: Clean nuclear type detection helper.The
isNukeTypefunction provides a clear, centralized way to identify nuclear units. This matches the pattern used in BuildMenu.ts.
415-424: Well-implemented hover handlers for nuclear previews.The hover enter/leave callbacks properly manage the UI state for nuclear previews, setting the active state and nuke type on hover and clearing it on leave.
src/client/graphics/layers/BuildMenu.ts (3)
157-158: Good anchor position storage for nuclear previews.Setting
nukeAnchorwhen showing the build menu ensures the preview system has the correct reference point for rendering.
380-399: Clean nuclear hover implementation with proper UI state management.The hover handlers correctly manage both the dimming state and the UI state's nuclear preview, ensuring coordinated behavior across components.
470-475: Efficient conditional hover handling for nuclear units.The pointer event handlers only trigger nuclear preview logic when the unit type is nuclear and enabled, avoiding unnecessary processing.
src/client/graphics/layers/NukePreview.ts (2)
1-24: Clean layer implementation with proper TypeScript patterns.The class structure follows good TypeScript practices with typed imports, clear interface implementation, and proper constructor parameter typing.
156-291: Excellent MIRV preview implementation with deterministic targeting.The MIRV rendering logic is well-structured with:
- Deterministic target generation using seeded random sampling
- Proper owner and terrain validation
- Efficient caching with signature-based invalidation
- Clear spacing constraints between targets
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/client/graphics/layers/RadialMenu.ts (3)
443-443: Guard onHoverLeave call; remove non-null assertionNon-null assertion is unnecessary and flagged by ESLint. Also aligns with earlier feedback to ensure params is non-null before invoking callbacks.
Apply:
- d.data.onHoverLeave?.(this.params!); + if (this.params) { + d.data.onHoverLeave?.(this.params); + }
455-457: Guard onHoverLeave on click; remove non-null assertionSame issue as Line 443. Also satisfies the ESLint hint (“This assertion is unnecessary…”).
Apply:
- if (d.data.isNuke) this.setMenuDim(false); - d.data.onHoverLeave?.(this.params!); + if (d.data.isNuke) this.setMenuDim(false); + if (this.params) { + d.data.onHoverLeave?.(this.params); + }
408-410: Guard params before invoking onHoverEnter; avoid the castAvoid relying on the earlier disabled check; make the callback call explicit and remove the unnecessary type assertion.
Apply:
- if (d.data.isNuke) this.setMenuDim(true); - d.data.onHoverEnter?.(this.params as MenuElementParams); + if (d.data.isNuke) this.setMenuDim(true); + if (this.params) { + d.data.onHoverEnter?.(this.params); + }
🧹 Nitpick comments (4)
tests/client/graphics/RadialMenuElements.test.ts (1)
516-580: Broaden hover-preview tests to cover edge casesTwo useful additions:
- Add a Hydrogen Bomb case to ensure parity with Atom/MIRV.
- Verify that hovering a non-nuke does not clear an active preview set by a prior nuke hover (to prevent accidental preview reset due to out-of-order mouse events).
Example to add after your current tests:
it("does not clear active nukePreview on non-nuke hover leave", () => { const enemyPlayer = { id: () => 2, isPlayer: jest.fn(() => true) } as unknown as PlayerView; mockParams.selected = enemyPlayer; const subMenu = attackMenuElement.subMenu!(mockParams); const atom = subMenu.find((i) => i.id === "attack_Atom Bomb"); const warship = subMenu.find((i) => i.id === "attack_Warship"); atom!.onHoverEnter?.(mockParams); expect(mockParams.uiState.nukePreview?.nukeType).toBe(UnitType.AtomBomb); warship!.onHoverLeave?.(mockParams); // still active since the leave event was from a non-nuke item expect(mockParams.uiState.nukePreview?.nukeType).toBe(UnitType.AtomBomb); }); it("sets and clears nukePreview on hover for Hydrogen Bomb", () => { const enemyPlayer = { id: () => 2, isPlayer: jest.fn(() => true) } as unknown as PlayerView; mockParams.selected = enemyPlayer; const subMenu = attackMenuElement.subMenu!(mockParams); const hydro = subMenu.find((i) => i.id === "attack_Hydrogen Bomb"); expect(hydro).toBeDefined(); expect(hydro!.isNuke).toBe(true); hydro!.onHoverEnter?.(mockParams); expect(mockParams.uiState.nukePreview).toEqual({ active: true, nukeType: UnitType.HydrogenBomb }); hydro!.onHoverLeave?.(mockParams); expect(mockParams.uiState.nukePreview).toBeUndefined(); });src/client/graphics/layers/RadialMenu.ts (1)
378-383: Minor nit: comment typo and styleThe trailing parenthesis in the comment is stray.
Apply:
- .style("transition", "opacity 120ms ease"); // smooth like the build menu) + .style("transition", "opacity 120ms ease"); // smooth like the build menusrc/client/graphics/layers/RadialMenuElements.ts (2)
347-354: Avoid duplicate isNukeType definitions; centralize in one helperBuildMenu.ts already defines isNukeType. Duplicating here risks drift. Prefer a shared helper (e.g., src/client/graphics/Nuke.ts) and import it in both places.
Example:
// src/client/graphics/Nuke.ts import { UnitType } from "../../core/game/Game"; export const isNukeType = (t: UnitType) => t === UnitType.AtomBomb || t === UnitType.HydrogenBomb || t === UnitType.MIRV;Then:
// here and in BuildMenu.ts import { isNukeType } from "../Nuke";
27-27: Consider tightening UIState.nukePreview typing to NukeType (union of UnitType values)UIState defines nukeType as string, but you consistently use UnitType.* values (tests included). Prefer a precise union type to catch errors at compile time.
Additional change outside this file (UIState.ts):
// src/client/graphics/UIState.ts import { UnitType } from "../../core/game/Game"; export type NukeType = | UnitType.AtomBomb | UnitType.HydrogenBomb | UnitType.MIRV | UnitType.MIRVWarhead; export type UIState = { attackRatio: number; nukePreview?: { active: boolean; nukeType: NukeType; }; nukeAnchor?: { x: number; y: number }; };This keeps types aligned with how the rest of the code uses nuke types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/client/graphics/GameRenderer.ts(4 hunks)src/client/graphics/layers/BuildMenu.ts(8 hunks)src/client/graphics/layers/MainRadialMenu.ts(2 hunks)src/client/graphics/layers/RadialMenu.ts(6 hunks)src/client/graphics/layers/RadialMenuElements.ts(6 hunks)tests/client/graphics/RadialMenuElements.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/graphics/layers/MainRadialMenu.ts
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/BuildMenu.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
tests/client/graphics/RadialMenuElements.test.tssrc/client/graphics/layers/RadialMenu.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 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/RadialMenuElements.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/RadialMenuElements.ts
🧬 Code Graph Analysis (3)
tests/client/graphics/RadialMenuElements.test.ts (2)
src/core/game/GameView.ts (1)
PlayerView(180-358)src/client/graphics/layers/RadialMenuElements.ts (1)
attackMenuElement(428-439)
src/client/graphics/layers/RadialMenu.ts (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
MenuElementParams(29-43)
src/client/graphics/layers/RadialMenuElements.ts (1)
src/client/graphics/UIState.ts (1)
UIState(1-8)
🪛 GitHub Check: 🔍 ESLint
src/client/graphics/layers/RadialMenu.ts
[failure] 456-456:
This assertion is unnecessary since it does not change the type of the expression.
src/client/graphics/layers/RadialMenuElements.ts
[failure] 62-62:
Missing semicolon.
🔇 Additional comments (4)
tests/client/graphics/RadialMenuElements.test.ts (1)
151-155: LGTM: uiState scaffolding in test params is correctProviding attackRatio and the optional nukePreview/nukeAnchor aligns with the new MenuElementParams shape. This keeps tests realistic and future-proof.
src/client/graphics/layers/RadialMenu.ts (1)
789-790: LGTM: reset dimming on hideEnsures the overlay is undimmed on menu close, keeping UI state consistent.
src/client/graphics/layers/RadialMenuElements.ts (2)
41-41: LGTM: uiState added to MenuElementParamsPlumbing shared UI state into menu callbacks is the right surface for hover-driven previews.
386-386: LGTM: emit isNuke flag on menu itemsThis pairs cleanly with RadialMenu’s dimming and hover hooks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/graphics/layers/RadialMenu.ts (2)
378-382: Solid helper; minor polish on transition and comment.The dimming helper is clean and easy to reuse. Two small tweaks:
- Set the transition once during element creation to avoid resetting it on every call.
- Fix the stray parenthesis in the comment.
Apply this diff here:
private setMenuDim(dim: boolean) { this.menuElement .style("opacity", dim ? "0.1" : "1") - .style("transition", "opacity 120ms ease"); // smooth like the build menu) + .style("transition", "opacity 120ms ease"); // smooth like the build menu }Optionally set the transition once in createMenuElement (outside the selected range):
// after creating this.menuElement in createMenuElement() this.menuElement.style("transition", "opacity 120ms ease");
408-410: GuardonHoverEnterwith an explicit params check to avoid the cast.You already gate earlier when disabled, but a direct guard keeps the types honest and matches our explicit-null-check style.
Apply this diff:
- if (d.data.isNuke) this.setMenuDim(true); - d.data.onHoverEnter?.(this.params as MenuElementParams); + if (d.data.isNuke) this.setMenuDim(true); + if (this.params) { + d.data.onHoverEnter?.(this.params); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/client/graphics/UIState.ts(1 hunks)src/client/graphics/layers/MainRadialMenu.ts(3 hunks)src/client/graphics/layers/NukePreview.ts(1 hunks)src/client/graphics/layers/PlayerActionHandler.ts(2 hunks)src/client/graphics/layers/RadialMenu.ts(6 hunks)src/client/graphics/layers/RadialMenuElements.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/graphics/layers/PlayerActionHandler.ts
- src/client/graphics/layers/MainRadialMenu.ts
- src/client/graphics/UIState.ts
- src/client/graphics/layers/RadialMenuElements.ts
- src/client/graphics/layers/NukePreview.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/RadialMenu.ts
🧬 Code Graph Analysis (1)
src/client/graphics/layers/RadialMenu.ts (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
MenuElementParams(29-43)
🔇 Additional comments (4)
src/client/graphics/layers/RadialMenu.ts (4)
423-423: Good: undim on mouseout before early returns.Resetting dim state even when the handler bails keeps the UI consistent.
443-445: Nice fix: hover-leave callback now guarded by params.This addresses the prior null assertion risk and aligns with our explicit checks preference.
457-461: Good consistency: clear dim and guardonHoverLeaveon click.Prevents lingering dim state and avoids null assertions during click handling.
793-794: Correct reset of dim state when hiding the menu.Ensures the radial always returns to a clean state across close paths.
There was a problem hiding this comment.
- Please embed the videos/images directly into the PR description, instead of linking to them.
- This ticket is not associated with its related issue. Please see https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue for details on how to link tickets properly.
- The UI here is overly complex and visually busy, which I think will confuse players and lead to negative feedback. I’m not sure players are asking for all of this information.
|
@TheGiraffe3 @scottanderson
Please let me know if I've misunderstood your point, or you have another suggestion. |
There was a problem hiding this comment.
@Dimitrije-V sorry I thought this was for the ticket in the current release related to showing SAM protection radius. Please come talk to us in discord about this feature.
|
This feature is similar to #1157 although they are a little bit different in what information they provide. |
|
i agree with @Dimitrije-V. Nuke preview feature really removes the depth of the game a little |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/client/graphics/layers/NukePreview.ts (8)
49-56: Make MIRV seed deterministic per (type, anchor, owner) as designedRight now
_npSeedchanges only with(type, anchor). The MIRV target set is re-generated when owner changes, but it still uses the old seed. To fully match “deterministic per (type, anchor, owner)”, include owner in the seed signature when MIRV is active.- const sig = `${p.nukeType}|${anchor.x}|${anchor.y}`; + const sig = + p.nukeType === UnitType.MIRV + ? (() => { + const o = this.game.owner(this.game.ref(anchor.x, anchor.y)); + const ownerKey = o.isPlayer() ? (o as any).id() : "TN"; + return `${p.nukeType}|${anchor.x}|${anchor.y}|${ownerKey}`; + })() + : `${p.nukeType}|${anchor.x}|${anchor.y}`; if (this._npSig !== sig) { this._npSig = sig; this._npSeed = this.game.ticks(); }
8-10: Upgrade helper to a type guard for better narrowingTurn
isNukeTypeinto a type guard so callers get narrowed types (clean, idiomatic TS).-export const isNukeType = (t: UnitType) => - t === UnitType.AtomBomb || t === UnitType.HydrogenBomb || t === UnitType.MIRV; +export const isNukeType = ( + t: UnitType, +): t is Exclude<NukeType, UnitType.MIRVWarhead> => + t === UnitType.AtomBomb || t === UnitType.HydrogenBomb || t === UnitType.MIRV;
97-119: Reduce per-frame work in probabilistic band (cache tiles per signature)This double loop does thousands of
worldToScreenCoordinatescalls andfillRects every frame. Since the band is stable per(type, anchor, seed), precompute the list of band tiles once per_npSigand reuse during render.Happy to sketch a small cache like
_bandFill: Array<{px:number;py:number;size:number}>keyed by_npSig.
183-185: Compare owner by ID, not by object identity
owner !== anchorOwnermay fail even for the same player if different object instances are returned. Compare stable IDs (or small IDs).- const owner = this.game.owner(tile); - if (owner !== anchorOwner) continue; + const owner = this.game.owner(tile); + const anchorOwnerId = anchorOwner.isPlayer() ? (anchorOwner as any).id() : -1; + const ownerId = owner.isPlayer() ? (owner as any).id() : -1; + if (ownerId !== anchorOwnerId) continue;
146-150: Avoid magic numbers: derive MIRV constants from config or shared constantsHardcoding
MIRV_RANGE = 1500andMIN_SPACING = 25may drift from server/game logic. Prefer reading from config, or import a shared constant (e.g., MirvExecution.mirvRange, proximity spacing).Example:
- Read
mirvRangefrom config.- Export a single source of truth for proximity/spacings.
112-117: Pixel snapping for crisper rectanglesWhen
tileStep > 1or at fractional scales,fillRectat fractionalpx/pycan blur. Snap to device pixels.- const px = pt.x - rect.left; - const py = pt.y - rect.top; + const px = Math.round(pt.x - rect.left) + 0.5; + const py = Math.round(pt.y - rect.top) + 0.5;Note: also consider rounding
sizeto avoid gaps.
237-255: Draw once, fill and stroke the same pathYou create two arcs for the same circle. Use one path, fill then stroke for a small perf win.
- ctx.beginPath(); - ctx.setLineDash([]); - ctx.lineWidth = opts.strokeWidth; - ctx.strokeStyle = opts.stroke; - ctx.arc(cx, cy, r, 0, Math.PI * 2); - ctx.stroke(); - - ctx.beginPath(); - ctx.fillStyle = opts.fill; - ctx.arc(cx, cy, r, 0, Math.PI * 2); - ctx.fill(); + ctx.beginPath(); + ctx.setLineDash([]); + ctx.arc(cx, cy, r, 0, Math.PI * 2); + ctx.fillStyle = opts.fill; + ctx.fill(); + ctx.lineWidth = opts.strokeWidth; + ctx.strokeStyle = opts.stroke; + ctx.stroke();
141-145: Consistent visual padding for outer boundaryThe single-blast safety line uses a padding based on
tileStep; MIRV uses a fixed-1. Consider harmonizing the padding logic for visual consistency at different zoom levels.Also applies to: 221-229
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/client/graphics/layers/NukePreview.ts(1 hunks)src/client/graphics/layers/PlayerActionHandler.ts(3 hunks)src/client/graphics/layers/RadialMenu.ts(6 hunks)src/client/graphics/layers/RadialMenuElements.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/graphics/layers/PlayerActionHandler.ts
- src/client/graphics/layers/RadialMenuElements.ts
- src/client/graphics/layers/RadialMenu.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/graphics/layers/NukePreview.ts (6)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (1)
GameView(360-692)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-296)src/client/graphics/UIState.ts (1)
UIState(3-10)src/core/StatsSchemas.ts (1)
NukeType(11-15)src/core/game/Game.ts (1)
Cell(285-307)
🔇 Additional comments (2)
src/client/graphics/layers/NukePreview.ts (2)
31-43: Nice: deterministic PRNG and small, typed helpersThe
h32+rand01approach is clean and deterministic, with unsigned return. Good foundation for stable previews.
65-128: Overall: solid composition, clear separation of MIRV vs single-blast, correct transform usageLayer stays untransformed, converts world→screen precisely, and restores context. Readability is good and sticks to simple TypeScript.
Also applies to: 130-234
| if (p.nukeType === "MIRV") { | ||
| this.renderMirvPreview(ctx, anchor.x, anchor.y, seed); | ||
| return; | ||
| } | ||
|
|
||
| this.renderSingleBlastPreview(ctx, anchor.x, anchor.y, p.nukeType as NukeType, seed); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: MIRV type compared to string instead of enum
This branch will never trigger if UnitType is an enum (very likely), so MIRV previews won’t render. Compare against UnitType.MIRV.
- if (p.nukeType === "MIRV") {
+ if (p.nukeType === UnitType.MIRV) {
this.renderMirvPreview(ctx, anchor.x, anchor.y, seed);
return;
}Run to find any other string comparisons:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '===\s*["'\''](?:MIRV|AtomBomb|HydrogenBomb)["'\'']' src -g '!**/node_modules/**'Length of output: 459
Use the UnitType.MIRV enum constant instead of the string literal
The check p.nukeType === "MIRV" will never succeed when UnitType is an enum (whether numeric or string-backed). Swap it for the enum value to ensure the MIRV preview renders correctly.
• File: src/client/graphics/layers/NukePreview.ts
Line: 57
• Diff to apply:
- if (p.nukeType === "MIRV") {
+ if (p.nukeType === UnitType.MIRV) {
this.renderMirvPreview(ctx, anchor.x, anchor.y, seed);
return;
}• I searched for any other literal comparisons of MIRV, AtomBomb, or HydrogenBomb via:
rg -nP -C2 '===\s*["'"'"'](?:MIRV|AtomBomb|HydrogenBomb)["'"'"']' src -g '!**/node_modules/**'and found no additional instances in src.
🤖 Prompt for AI Agents
In src/client/graphics/layers/NukePreview.ts around lines 57 to 63, the check
currently compares p.nukeType to the string literal "MIRV", which will never
match when UnitType is an enum; replace the string literal with the enum
constant UnitType.MIRV, ensure UnitType is imported in this file (add an import
if missing), and keep the rest of the conditional flow the same so
renderMirvPreview is called when p.nukeType === UnitType.MIRV.
Description:
This PR adds interactive nuke previews (Atom, Hydrogen, MIRV) to the build & radial menus, and dims the UI while previewing so players can aim with confidence.
What’s included
(src/client/graphics/layers/NukePreview.ts)UIStateextended to contain:NukePreviewlayer and providesuiState.uiState.nukeAnchorto the clicked world tile when the menu opens.uiState.nukePreviewand adds a subtle “dim” state to the menu; clears on leave/close.uiStateinMenuElementParams.createMenuElements: marks nukes (isNuke) and providesonHoverEnter/onHoverLeaveto set/clearuiState.nukePreview.uiState.nukeAnchorwhen opening on a tile.You can see what this looks like here:
Atom.mp4
Hydro.mp4
MIRV.mp4
Build.Menu.mp4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
.dimitrije