feat: Attacking Troops Overlay#3427
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds an Attacking Troops Overlay layer and UI toggle; replaces single-average attack positioning with cluster-based positions across game, runner, and worker plumbing; updates worker messages and client APIs to request clustered attack positions; implements clustering in AttackImpl and updates callers to use clustered results. Changes
Sequence DiagramsequenceDiagram
participant Overlay as AttackingTroopsOverlay
participant View as GameView
participant Worker as Worker.worker
participant Runner as GameRunner
participant Attack as Attack objects
Overlay->>View: request attackClusteredPositions(attackID?)
View->>Worker: postMessage { type: "attack_clustered_positions", playerID, attackID? }
Worker->>Runner: call attackClusteredPositions(playerID, attackID?)
Runner->>Attack: invoke clusteredPositions() for each attack
Attack-->>Runner: return TileRef[] per attack
Runner-->>Worker: return attacks with clustered positions
Worker-->>View: postMessage { type: "attack_clustered_positions_result", attacks }
View-->>Overlay: resolve promise with attacks
Overlay->>Overlay: reconcile labels, update content, renderLayer()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 (1)
src/client/graphics/GameRenderer.ts (1)
286-287: GroupTroopAdvantageLayerwith non-transformed layers.Line 286 inserts a
shouldTransform() === falselayer between map layers. That adds extra transform state switches each frame. Move it to the non-transform layer block to keep render-state changes minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/GameRenderer.ts` around lines 286 - 287, The TroopAdvantageLayer (which returns shouldTransform() === false) is currently inserted between transformed map layers causing extra transform state switches; relocate the new TroopAdvantageLayer(game, transformHandler) entry out of the transformed-layer list and add it to the non-transformed layer block alongside other layers like RailroadLayer (or whichever block groups non-transformed layers) so all shouldTransform() === false layers are grouped together to minimize per-frame render-state changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/TroopAdvantageLayer.ts`:
- Line 148: In TroopAdvantageLayer (the code that sets el.innerHTML), avoid
building HTML strings with dynamic values; instead create three span elements,
set their textContent to the literal pieces ("⚔ " + atkStr, " vs ", defStr) and
style each span's color via the style.color property, then append the spans to
el (clearing existing children first) so atkStr and defStr are inserted safely
without using innerHTML.
- Around line 15-16: The per-tick position lookup is starting new async requests
for each AttackLabel (labels: Map<string, AttackLabel>) without in-flight
control, ignores null results, and can produce unhandled rejections; fix by
adding an in-flight guard (e.g. a Map<string, boolean> or an inFlight property
on AttackLabel) in the method that issues the position request so you skip
starting a request when one is already pending, wrap the await call in try/catch
to handle and log errors, and when the async position result is null or
indicates stale data remove/hide the corresponding AttackLabel from labels
instead of leaving old positions visible. Ensure you clear the in-flight flag on
both success and error paths so future ticks can re-request.
---
Nitpick comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 286-287: The TroopAdvantageLayer (which returns shouldTransform()
=== false) is currently inserted between transformed map layers causing extra
transform state switches; relocate the new TroopAdvantageLayer(game,
transformHandler) entry out of the transformed-layer list and add it to the
non-transformed layer block alongside other layers like RailroadLayer (or
whichever block groups non-transformed layers) so all shouldTransform() ===
false layers are grouped together to minimize per-frame render-state changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66c824fd-1749-4475-ae0e-89c4d743148e
📒 Files selected for processing (2)
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/TroopAdvantageLayer.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
ralfisalhon
force-pushed
the
feature/troop-advantage-layer
branch
from
64c4b72 to
0a32cd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/TroopAdvantageLayer.ts`:
- Around line 63-67: When you bail out of rendering because an attack has no
targetID or no valid defender (checks using attack.targetID,
this.game.playerBySmallID(...) and defender.isPlayer()), make sure to remove any
existing on-screen label for that attack before you continue; call the existing
cleanup routine (e.g. this.removeLabelForAttack(attack.id)) or delete the entry
from the labels collection (e.g. this.attackLabels.delete(attack.id) or delete
this.attackLabels[attack.id]) prior to the continue so stale labels are removed
when an attack becomes non-renderable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9899d32d-d176-4397-b1d1-a5381aa5e881
📒 Files selected for processing (1)
src/client/graphics/layers/TroopAdvantageLayer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/TroopAdvantageLayer.ts (1)
14-14: Add definite assignment assertion forcontainer.The
containerproperty is assigned ininit()but TypeScript cannot verify thatinit()is called before other methods access it. Use the!assertion to indicate you guarantee it will be assigned before use.Proposed fix
- private container: HTMLDivElement; + private container!: HTMLDivElement;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TroopAdvantageLayer.ts` at line 14, In TroopAdvantageLayer, mark the container property with a definite assignment assertion to satisfy TypeScript (change the declaration of container in the class to use the ! operator), e.g., update the field declaration for container to indicate it will be assigned in init(); ensure init() remains responsible for assigning container and that methods using container (like any render/tearDown methods) are still called after init().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/TroopAdvantageLayer.ts`:
- Line 14: In TroopAdvantageLayer, mark the container property with a definite
assignment assertion to satisfy TypeScript (change the declaration of container
in the class to use the ! operator), e.g., update the field declaration for
container to indicate it will be assigned in init(); ensure init() remains
responsible for assigning container and that methods using container (like any
render/tearDown methods) are still called after init().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50304de0-468c-418b-9bd5-5c5fac2e46d3
📒 Files selected for processing (1)
src/client/graphics/layers/TroopAdvantageLayer.ts
ralfisalhon
changed the title
TroopAdvantageLayer
ralfisalhon
changed the title
TroopAdvantageLayerTroopAdvantageLayer
ralfisalhon
force-pushed
the
feature/troop-advantage-layer
branch
from
b478222 to
42fa7d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/TroopAdvantageLayer.ts`:
- Around line 33-45: TroopAdvantageLayer.init currently appends this.container
to document.body and registers an event listener on this.eventBus for
AlternateViewEvent but never cleans them up; add a public destroy() method on
TroopAdvantageLayer that removes the DOM node (this.container.remove() or
removeChild) and unregisters the event listener (store the handler reference
returned or the bound callback and call eventBus.off/unsubscribe) to teardown
the container and event subscription; implement the cleanup logic referencing
this.container and the AlternateViewEvent handler so future updates to
GameRenderer or the Layer interface can call destroy() to avoid leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd831a0f-69da-4785-8d19-bf7ec2ea7efe
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/TroopAdvantageLayer.tssrc/core/game/UserSettings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/GameRenderer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/graphics/GameRenderer.ts (1)
286-286: Layer ordering note:TroopAdvantageLayerbreaksshouldTransformgrouping.Per the comment at lines 280-282, layers should be grouped by
shouldTransform()return value.TroopAdvantageLayerreturnsfalse, but it sits betweenTerritoryLayerandRailroadLayerwhich both returntrue.Since
TroopAdvantageLayeruses DOM overlays and doesn't draw to the canvas, the extrasave()/restore()calls have minimal impact. This is fine to keep as-is, but moving it after canvas-based layers would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/GameRenderer.ts` at line 286, TroopAdvantageLayer is placed between TerritoryLayer and RailroadLayer but its shouldTransform() returns false while those neighbors return true; move the instantiation/new TroopAdvantageLayer(game, transformHandler, eventBus) out of the transform-true group and insert it after the canvas-based layers (e.g., after RailroadLayer or after the block of layers that return true) so all layers where shouldTransform() === true remain contiguous while DOM-overlay TroopAdvantageLayer stays with other non-transform layers.src/client/graphics/layers/TroopAdvantageLayer.ts (1)
16-27: Consider receivingUserSettingsvia constructor injection for consistency.Other layers in
GameRenderer.tsreceiveuserSettingsas a dependency (e.g.,TerritoryLayer). Creating a new instance works, but dependency injection keeps the pattern uniform and makes testing easier.Proposed change
export class TroopAdvantageLayer implements Layer { private container: HTMLDivElement; private labels = new Map<string, AttackLabel>(); private inFlightPositionRequests = new Set<string>(); private isVisible = true; - private userSettings = new UserSettings(); constructor( private readonly game: GameView, private readonly transformHandler: TransformHandler, private readonly eventBus: EventBus, + private readonly userSettings: UserSettings, ) {}Then in
GameRenderer.ts:- new TroopAdvantageLayer(game, transformHandler, eventBus), + new TroopAdvantageLayer(game, transformHandler, eventBus, userSettings),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TroopAdvantageLayer.ts` around lines 16 - 27, The TroopAdvantageLayer currently constructs its own UserSettings (private userSettings = new UserSettings()), breaking DI consistency; modify the TroopAdvantageLayer constructor to accept a UserSettings parameter (e.g., add private readonly userSettings: UserSettings to the constructor signature) and remove the inline new UserSettings() field initialization so the class uses the injected instance; update call sites (like where GameRenderer constructs TroopAdvantageLayer) to pass the shared UserSettings instance consistent with other layers such as TerritoryLayer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/GameRenderer.ts`:
- Line 286: TroopAdvantageLayer is placed between TerritoryLayer and
RailroadLayer but its shouldTransform() returns false while those neighbors
return true; move the instantiation/new TroopAdvantageLayer(game,
transformHandler, eventBus) out of the transform-true group and insert it after
the canvas-based layers (e.g., after RailroadLayer or after the block of layers
that return true) so all layers where shouldTransform() === true remain
contiguous while DOM-overlay TroopAdvantageLayer stays with other non-transform
layers.
In `@src/client/graphics/layers/TroopAdvantageLayer.ts`:
- Around line 16-27: The TroopAdvantageLayer currently constructs its own
UserSettings (private userSettings = new UserSettings()), breaking DI
consistency; modify the TroopAdvantageLayer constructor to accept a UserSettings
parameter (e.g., add private readonly userSettings: UserSettings to the
constructor signature) and remove the inline new UserSettings() field
initialization so the class uses the injected instance; update call sites (like
where GameRenderer constructs TroopAdvantageLayer) to pass the shared
UserSettings instance consistent with other layers such as TerritoryLayer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30c65e8e-0cc0-482c-b7b4-afba78e65864
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/TroopAdvantageLayer.tssrc/core/game/UserSettings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
ralfisalhon
force-pushed
the
feature/troop-advantage-layer
branch
from
757ab00 to
f026a4b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/layers/TroopAdvantageLayer.ts (1)
43-46: Skip tick work while the layer is hidden in terrain view.When alternate view is active, labels are hidden visually, but
tick()still does per-attack work and async position fetches. Add an early return for hidden mode and clear labels once on hide to reduce unnecessary load.♻️ Proposed refactor
this.onAlternateView = (e) => { this.isVisible = !e.alternateView; + if (!this.isVisible) { + this.clearAllLabels(); + } this.container.style.display = this.isVisible ? "" : "none"; }; tick() { + if (!this.isVisible) { + return; + } if (!this.userSettings.troopAdvantageLayer()) { this.clearAllLabels(); return; }Also applies to: 61-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TroopAdvantageLayer.ts` around lines 43 - 46, onAlternateView currently only toggles visibility but doesn't stop background work; update the onAlternateView handler (method onAlternateView / property this.onAlternateView) to clear existing labels when hiding (e.g., remove or empty this.labels / this.container children) and then in the layer's tick method add an early return when the layer is not visible (check this.isVisible) so no per-attack work or async position fetches run while alternate/terrain view is active; ensure the same early-return pattern is applied to any related update routines in this class that perform async fetches or per-attack processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/TroopAdvantageLayer.ts`:
- Around line 43-46: onAlternateView currently only toggles visibility but
doesn't stop background work; update the onAlternateView handler (method
onAlternateView / property this.onAlternateView) to clear existing labels when
hiding (e.g., remove or empty this.labels / this.container children) and then in
the layer's tick method add an early return when the layer is not visible (check
this.isVisible) so no per-attack work or async position fetches run while
alternate/terrain view is active; ensure the same early-return pattern is
applied to any related update routines in this class that perform async fetches
or per-attack processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a46fe232-04b2-441b-a286-476cb4cfb8d8
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/TroopAdvantageLayer.tssrc/core/game/UserSettings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/GameRenderer.ts
- src/core/game/UserSettings.ts
ralfisalhon
changed the title
TroopAdvantageLayer
|
the defender troop count seems a bit redundant, because it's already displayed on their territory? Maybe just show the attacker troops? |
Thanks @evanpelle for taking a look! I've quickly implemented your suggestion so that we have some footage to compare with. Here it is with just the attacker count: Here is another example of island conquering with Attacker vs Defender counts: Let me know your thoughts, thanks! |
|
On second thought, I think your suggestion with just the attackers will be the better way forward. I'll push some changes. |
ralfisalhon
changed the title
|
Thank you for the Staging deployment! @FloPinguin 🙏🏼 |
|
This also gave me a chance to test on Mobile: The attack/defence emojis I used appear differently on different devices, so I'll want to standardize them to using the icons that already exist. |
ralfisalhon
force-pushed
the
feature/troop-advantage-layer
branch
from
843f2ee to
af978a7
Compare
✅ Actions performedReview triggered.
|
…e z-index above names prettier fix Apply AI timeout suggestion Apply AI TroopAdvantageLayer tick suggestion Rename to AttackingTroopsOverlay, label transitions Rename centers to positions
ralfisalhon
force-pushed
the
feature/troop-advantage-layer
branch
from
af978a7 to
7667ef5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/graphics/layers/AttackingTroopsOverlay.ts (1)
264-285: Consider caching icon/span elements to reduce DOM churn.
updateLabelContent()creates 2 new elements (img + span) and callsreplaceChildren()every time. With a 200ms tick and few labels, this is fine for now. If label count grows or updates become more frequent, caching these elements and updating only their attributes would reduce GC pressure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/AttackingTroopsOverlay.ts` around lines 264 - 285, The updateLabelContent function currently creates a new img and span and calls el.replaceChildren on every tick; change it to reuse existing child elements instead: in updateLabelContent (and/or where labels are created), ensure each label container (el) initially contains an img and span (or mark them with data-role attributes), then in updateLabelContent find those children (e.g., el.querySelector('img') and el.querySelector('span') or by data-role), and update img.src, img.style.width/height if needed, span.style.color and span.textContent instead of creating new elements and calling el.replaceChildren; this eliminates repeated DOM creation and reduces GC churn while keeping the same behavior.src/core/game/AttackImpl.ts (1)
176-183: Prefersliceoversplicefor non-mutating extraction.
significant.splice(0, maxClusters)works, butslicebetter signals you only want to take the first N items without side effects. Small readability win.✨ Suggested change
- return significant.splice(0, maxClusters).map((c) => c.tile); + return significant.slice(0, maxClusters).map((c) => c.tile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/AttackImpl.ts` around lines 176 - 183, The code filters clusters into the variable significant and then uses significant.splice(0, maxClusters) which mutates that array; change this to use significant.slice(0, maxClusters) so you take the first N clusters without side effects and then map to .tile as before (references: variables/signals: significant, clusters, maxClusters, minSize in AttackImpl).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/AttackingTroopsOverlay.ts`:
- Around line 264-285: The updateLabelContent function currently creates a new
img and span and calls el.replaceChildren on every tick; change it to reuse
existing child elements instead: in updateLabelContent (and/or where labels are
created), ensure each label container (el) initially contains an img and span
(or mark them with data-role attributes), then in updateLabelContent find those
children (e.g., el.querySelector('img') and el.querySelector('span') or by
data-role), and update img.src, img.style.width/height if needed,
span.style.color and span.textContent instead of creating new elements and
calling el.replaceChildren; this eliminates repeated DOM creation and reduces GC
churn while keeping the same behavior.
In `@src/core/game/AttackImpl.ts`:
- Around line 176-183: The code filters clusters into the variable significant
and then uses significant.splice(0, maxClusters) which mutates that array;
change this to use significant.slice(0, maxClusters) so you take the first N
clusters without side effects and then map to .tile as before (references:
variables/signals: significant, clusters, maxClusters, minSize in AttackImpl).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c599f2c-07ef-4cf8-acd3-1b6d9977b5f3
📒 Files selected for processing (14)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/AttackingTroopsOverlay.tssrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/SettingsModal.tssrc/core/GameRunner.tssrc/core/game/AttackImpl.tssrc/core/game/Game.tssrc/core/game/GameView.tssrc/core/game/UserSettings.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/core/worker/WorkerMessages.tstests/client/graphics/layers/AttackingTroopsOverlay.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/graphics/GameRenderer.ts
- src/core/game/GameView.ts
- src/client/graphics/layers/SettingsModal.ts
- src/client/graphics/layers/AttacksDisplay.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Thank you @evanpelle , pointing me in the direction of using forEachNeighborWithDiag and also suggesting the removal of attack_average_position were both much needed changes. |
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
3 participants
Description:
https://troop-advantage-layer.openfront.dev/
Hey OpenFront dev team, I've been really enjoying the game, and the v0.30 changes have felt great so far. Happy to start contributing!
This PR introduces
AttackingTroopsOverlay, a layer that renders live attacker vs. defender troop counts directly on active front lines. Players can immediately gauge combat strength without leaving the map view.A recent change updates the layer to just the # of attackers and a symbol for attack/defence:
Left: Perspective of Anon 667 (Blue) | Right: Perspective of Anon332 (Red)
How it works:
Settings: An "Attacking Troops Overlay" toggle is added to Settings, enabled by default.
--> the screenshot is old, but the text has been updated
Checklist
Discord
Radyus