Feat: Transport boats now show arrival ETA#3381
Feat: Transport boats now show arrival ETA#3381camclark wants to merge 4 commits intoopenfrontio:mainfrom
Conversation
|
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds client-side ETA calculation and display for boats in transit: exports Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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/graphics/layers/AttacksDisplay.ts (1)
27-33: ExtractestimateBoatEtaSecondsinto a plain helper module.
tests/client/graphics/layers/BoatEta.test.tsnow has to import the whole Lit element module just to test a pure function, which also pulls in custom-element registration and asset imports. A small utility file would keep this test simpler and side-effect free.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 27 - 33, Move the pure function estimateBoatEtaSeconds out of the Lit element file into a small plain helper module that exports the same named function; in AttacksDisplay.ts replace the local implementation with an import of estimateBoatEtaSeconds from the new helper (or re-export it) so external callers keep working, and update tests to import the helper module directly instead of importing the whole Lit element module.
🤖 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/AttacksDisplay.ts`:
- Around line 363-370: In getBoatEtaSeconds, replace the Manhattan-distance
based ETA with one computed from the boat's actual remaining route length:
instead of calling this.game.manhattanDist(boat.tile(), targetTile), obtain the
boat's remaining path/route length (e.g., via a method on UnitView like
boat.remainingRouteLength() or boat.route()/boat.path() that returns remaining
steps) or ask the game's pathfinder for the current path length from boat.tile()
to boat.targetTile(), then pass that step count into estimateBoatEtaSeconds so
ETA reflects route steps around land/chokepoints (update references in
getBoatEtaSeconds and remove the manhattanDist usage).
---
Nitpick comments:
In `@src/client/graphics/layers/AttacksDisplay.ts`:
- Around line 27-33: Move the pure function estimateBoatEtaSeconds out of the
Lit element file into a small plain helper module that exports the same named
function; in AttacksDisplay.ts replace the local implementation with an import
of estimateBoatEtaSeconds from the new helper (or re-export it) so external
callers keep working, and update tests to import the helper module directly
instead of importing the whole Lit element module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80c72ee4-2eb1-4bf8-83a9-ec856cdd2177
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/graphics/layers/AttacksDisplay.tstests/client/graphics/layers/BoatEta.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/AttacksDisplay.ts (1)
387-425:⚠️ Potential issue | 🟠 MajorDrive the ETA refresh from a 1s wall-clock tick.
This renders the new countdown, but the layer still does not opt into a 1-second wall-clock schedule. As a result, both outgoing and incoming boat ETAs will follow the default layer tick cadence instead of the PR requirement of updating once per second.
Suggested fix
export class AttacksDisplay extends LitElement implements Layer { public eventBus: EventBus; public game: GameView; public uiState: UIState; @@ tick() { this.active = true; @@ this.requestUpdate(); } + + getTickIntervalMs(): number { + return 1000; + } shouldTransform(): boolean { return false; }Based on learnings, UI/layer updates should be wall-clock throttled independently of simulation speed, and layers should expose
getTickIntervalMs()for that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 387 - 425, The layer currently renders ETA countdowns but doesn't opt into a 1s wall-clock schedule; add/override getTickIntervalMs() on the AttacksDisplay class to return 1000 (ms) so the renderer uses a 1-second wall-clock tick and the values from getBoatEtaSeconds(boat) refresh once per second for both outgoingBoats and incoming boats; ensure this method exists on the class (same class that defines renderButton, getBoatEtaSeconds, outgoingBoats mapping) so the layer infrastructure will throttle updates independently of simulation speed.
🤖 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/AttacksDisplay.ts`:
- Around line 27-35: The estimateBoatEtaSeconds function validates distance but
not turnIntervalMs, so invalid or non-positive intervals can produce bad ETAs;
update estimateBoatEtaSeconds to also return null when turnIntervalMs is not a
finite number or is <= 0 (e.g., use Number.isFinite(turnIntervalMs) &&
turnIntervalMs > 0), then compute secondsPerTick and the ceil as before;
reference the function estimateBoatEtaSeconds and the parameter turnIntervalMs
when making this change.
---
Outside diff comments:
In `@src/client/graphics/layers/AttacksDisplay.ts`:
- Around line 387-425: The layer currently renders ETA countdowns but doesn't
opt into a 1s wall-clock schedule; add/override getTickIntervalMs() on the
AttacksDisplay class to return 1000 (ms) so the renderer uses a 1-second
wall-clock tick and the values from getBoatEtaSeconds(boat) refresh once per
second for both outgoingBoats and incoming boats; ensure this method exists on
the class (same class that defines renderButton, getBoatEtaSeconds,
outgoingBoats mapping) so the layer infrastructure will throttle updates
independently of simulation speed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aed723fc-04da-435d-8114-962bc361a05a
📒 Files selected for processing (1)
src/client/graphics/layers/AttacksDisplay.ts
|
Regarding the
Adding a 1s interval would actually make the ETA update less frequently than the betrayal timer, which would be inconsistent. Leaving it as-is. |
| if (!Number.isFinite(distance) || distance < 0) return null; | ||
| if (!Number.isFinite(turnIntervalMs) || turnIntervalMs <= 0) return null; |
There was a problem hiding this comment.
do we need these checks? I don't think this should ever happen.
Maybe just throw an error instead
| } from "./Leaderboard"; | ||
| import swordIcon from "/images/SwordIcon.svg?url"; | ||
|
|
||
| /** Estimates boat arrival time in seconds from Manhattan distance and server tick interval. */ |
There was a problem hiding this comment.
manhattan distance is a bad heuristic. could be way off if boat is going around a peninsula. Instead we should use actual distance
3 participants
Description:
Adds a real-time ETA countdown (in seconds) to transport boats in the attacks display panel. Shows estimated arrival time for both outgoing and incoming boats, giving players at-a-glance information for strategic planning.
Resolves #1793
Image showing both outgoing and incoming boats:
Gif of the same:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
cool_clarky