feat: add territory hover highlight setting #3701
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,11 +12,18 @@ import { | |
| import { euclDistFN, TileRef } from "../../../core/game/GameMap"; | ||
| import { GameUpdateType } from "../../../core/game/GameUpdates"; | ||
| import { GameView, PlayerView } from "../../../core/game/GameView"; | ||
| import { | ||
| TERRITORY_HIGHLIGHT_KEY, | ||
| TerritoryHighlightMode, | ||
| USER_SETTINGS_CHANGED_EVENT, | ||
| UserSettings, | ||
| } from "../../../core/game/UserSettings"; | ||
| import { PseudoRandom } from "../../../core/PseudoRandom"; | ||
| import { | ||
| AlternateViewEvent, | ||
| DragEvent, | ||
| MouseOverEvent, | ||
| TerritoryHighlightKeyEvent, | ||
| } from "../../InputHandler"; | ||
| import { FrameProfiler } from "../FrameProfiler"; | ||
| import { TransformHandler } from "../TransformHandler"; | ||
|
|
@@ -47,6 +54,17 @@ export class TerritoryLayer implements Layer { | |
| private highlightedTerritory: PlayerView | null = null; | ||
|
|
||
| private alternativeView = false; | ||
| private highlightMode: TerritoryHighlightMode; | ||
| private highlightKeyHeld = false; | ||
| private onHighlightSettingChanged = ((e: CustomEvent) => { | ||
| const value = e.detail; | ||
| if (value === "always" || value === "onKeyPress") { | ||
| this.highlightMode = value; | ||
| } else { | ||
| this.highlightMode = "never"; | ||
| } | ||
| this.updateHighlightedTerritory(); | ||
| }) as EventListener; | ||
| private lastDragTime = 0; | ||
| private nodrawDragDuration = 200; | ||
| private lastMousePosition: { x: number; y: number } | null = null; | ||
|
|
@@ -63,6 +81,7 @@ export class TerritoryLayer implements Layer { | |
| ) { | ||
| this.theme = game.config().theme(); | ||
| this.cachedTerritoryPatternsEnabled = undefined; | ||
| this.highlightMode = new UserSettings().territoryHighlight(); | ||
| } | ||
|
|
||
| shouldTransform(): boolean { | ||
|
|
@@ -328,11 +347,22 @@ export class TerritoryLayer implements Layer { | |
| this.eventBus.on(MouseOverEvent, (e) => this.onMouseOver(e)); | ||
| this.eventBus.on(AlternateViewEvent, (e) => { | ||
| this.alternativeView = e.alternateView; | ||
| this.updateHighlightedTerritory(); | ||
| }); | ||
| this.eventBus.on(TerritoryHighlightKeyEvent, (e) => { | ||
| this.highlightKeyHeld = e.active; | ||
| this.updateHighlightedTerritory(); | ||
| }); | ||
| this.eventBus.on(DragEvent, (e) => { | ||
| // TODO: consider re-enabling this on mobile or low end devices for smoother dragging. | ||
| // this.lastDragTime = Date.now(); | ||
| }); | ||
|
|
||
| globalThis.addEventListener?.( | ||
| `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}`, | ||
| this.onHighlightSettingChanged, | ||
| ); | ||
|
Comment on lines
+361
to
+364
Contributor
There was a problem hiding this comment. Settings listener added on The listener attached in A small fix is to keep a 🧹 Suggested teardown+ destroy() {
+ globalThis.removeEventListener?.(
+ `${USER_SETTINGS_CHANGED_EVENT}:${TERRITORY_HIGHLIGHT_KEY}`,
+ this.onHighlightSettingChanged,
+ );
+ }🤖 Prompt for AI Agents |
||
|
|
||
| this.redraw(); | ||
| } | ||
|
|
||
|
|
@@ -342,7 +372,17 @@ export class TerritoryLayer implements Layer { | |
| } | ||
|
|
||
| private updateHighlightedTerritory() { | ||
| const shouldHighlight = | ||
| this.highlightMode === "always" || | ||
| (this.highlightMode === "onKeyPress" && this.highlightKeyHeld) || | ||
| this.alternativeView; | ||
|
|
||
| if (!shouldHighlight) { | ||
| if (this.highlightedTerritory) { | ||
| const prev = this.highlightedTerritory; | ||
| this.highlightedTerritory = null; | ||
| this.updateHighlightAlpha(prev, null); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -368,17 +408,31 @@ export class TerritoryLayer implements Layer { | |
| } | ||
|
|
||
| if (previousTerritory?.id() !== this.highlightedTerritory?.id()) { | ||
| this.updateHighlightAlpha(previousTerritory, this.highlightedTerritory); | ||
| } | ||
| } | ||
|
|
||
| private updateHighlightAlpha( | ||
| oldPlayer: PlayerView | null, | ||
| newPlayer: PlayerView | null, | ||
| ) { | ||
| const data = this.imageData.data; | ||
| const oldSmallID = oldPlayer?.smallID() ?? -1; | ||
| const newSmallID = newPlayer?.smallID() ?? -1; | ||
| this.game.forEachTile((tile) => { | ||
| const offset = tile * 4; | ||
| const alpha = data[offset + 3]; | ||
| // Only update non-border territory fill tiles (alpha 150 or 230) | ||
| if (alpha !== 150 && alpha !== 230) return; | ||
| const tileOwner = this.game.ownerID(tile); | ||
| if (tileOwner === newSmallID) { | ||
| data[offset + 3] = 230; | ||
| } else if (alpha === 230 && tileOwner === oldSmallID) { | ||
| data[offset + 3] = 150; | ||
| } | ||
| }); | ||
| } | ||
|
Comment on lines
+415
to
+434
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look for tile-iteration methods on PlayerView and related classes.
rg -nP --type=ts -C2 '\b(tiles|ownedTiles|allTiles|borderTiles)\s*\(' -g 'src/core/game/**'
ast-grep --pattern $'class PlayerView {
$$$
}'Repository: openfrontio/OpenFrontIO Length of output: 25984 🏁 Script executed: cd src/core/game && grep -n "^\s*\(tiles\|ownedTiles\|allTiles\)(" GameView.ts | head -20Repository: openfrontio/OpenFrontIO Length of output: 49
However, To address this, consider:
🤖 Prompt for AI Agents
Author
There was a problem hiding this comment. Addressed in 6b19661. The main bottleneck was calling Replaced with The
Contributor
There was a problem hiding this comment.
const oldSmallID = oldPlayer?.smallID() ?? -1;
const newSmallID = newPlayer?.smallID() ?? -1;
this.game.forEachTile((tile) => {
const offset = tile * 4;
const alpha = data[offset + 3];
if (alpha !== 150 && alpha !== 230) return; // skips ~60% early
const tileOwner = this.game.ownerID(tile); // Uint16Array read + bitmask
if (tileOwner === newSmallID) { ... } // int comparison
else if (alpha === 230 && tileOwner === oldSmallID) { ... }
});The per-tile cost is now just a byte read, an early return for most tiles, a
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
||
|
|
||
| private getTerritoryAtCell(cell: { x: number; y: number }) { | ||
| const tile = this.game.ref(cell.x, cell.y); | ||
| if (!tile) { | ||
|
|
@@ -559,15 +613,12 @@ export class TerritoryLayer implements Layer { | |
| return; | ||
| } | ||
| const owner = this.game.owner(tile) as PlayerView; | ||
| const isHighlighted = | ||
| this.highlightedTerritory && | ||
| this.highlightedTerritory.smallID() === owner.smallID(); | ||
| const myPlayer = this.game.myPlayer(); | ||
|
|
||
| if (this.game.isBorder(tile)) { | ||
| if (myPlayer) { | ||
| const alternativeColor = this.alternateViewColor(owner); | ||
| this.paintTile(this.alternativeImageData, tile, alternativeColor, 255); | ||
|
|
@@ -589,7 +640,8 @@ export class TerritoryLayer implements Layer { | |
| // Alternative view only shows borders. | ||
| this.clearAlternativeTile(tile); | ||
|
|
||
| const alpha = isHighlighted ? 230 : 150; | ||
| this.paintTile(this.imageData, tile, owner.territoryColor(tile), alpha); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "default H" hint will become stale after rebinding.
This label literally says "default H", but the keybind is user-configurable. If someone rebinds
highlightTerritoryto another key, the select option will keep advertising H, which is a small but real source of confusion. Consider dropping the key hint here (the keybinds tab is the source of truth) or passing the current key in as an ICU argument.✏️ Possible simplification
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentionally keeping the "default H" hint. The benefit is that a user can see the default key right in the dropdown without having to navigate to the keybinds tab first. If they rebind it, they already know where to find keybind settings — but for the majority of users who never rebind, this tells them exactly which key to press at a glance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luctrate, that's a fair point — showing the default key inline reduces friction for the majority of users who never touch keybind settings. The trade-off (slightly stale label for the small group who rebind) is reasonable given that those users already know where keybinds live.Makes sense, thanks for the context!
🧠 Learnings used