Leaderboard update part 2: stat columns#4465
Open
jsshap wants to merge 1 commit into
Open
Conversation
3 tasks
Contributor
WalkthroughAdds a goldPerMinute income stat computed server-side using bucketed gold-income accumulation. The stat is threaded through packed player update buffers (now 5 fields instead of 4), the PlayerUpdate diff/merge contract, client-side PlayerState/PlayerView, and displayed with troops and cities in the leaderboard UI. addGold gains a countAsIncome flag so donations don't count as income. ChangesGold Per Minute Income Stat
Sequence Diagram(s)sequenceDiagram
participant Player
participant PlayerImpl
participant GameUpdateViewData
participant GameView
participant Leaderboard
Player->>PlayerImpl: addGold(amount, tile, countAsIncome=true)
PlayerImpl->>PlayerImpl: recordGoldIncome into time buckets
GameUpdateViewData->>PlayerImpl: toUpdate() / toFullUpdate()
PlayerImpl-->>GameUpdateViewData: goldPerMinute() computed from buckets
GameUpdateViewData->>GameView: packedPlayerUpdates [smallID, tilesOwned, gold, goldPerMinute, troops]
GameView->>GameView: update PlayerState.goldPerMinute
Leaderboard->>GameView: read goldPerMinute via PlayerView
Leaderboard-->>Player: render Gold/min column, sortable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/hud/layers/Leaderboard.ts (1)
86-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache city count once, like
maxTroops, instead of recomputing it repeatedly.
maxTroopsis computed once per player up front and reused during sort and row-building.totalUnitLevels(UnitType.City)isn't cached the same way — it runs inside the sort comparator (once per comparison when sorted by cities) and again in the entry map for every player, every tick. Same idea, just not applied consistently.♻️ Suggested fix: precompute cities alongside maxTroops
interface PlayerViewTroopsCache { pv: PlayerView; maxTroops: number; + cities: number; } const compare = (a: number, b: number) => this._sortOrder === "asc" ? a - b : b - a; const maxTroops = (p: PlayerView) => this.game!.config().maxTroops(p); const sorted: PlayerViewTroopsCache[] = this.game .playerViews() .filter((p) => p.isAlive()) - .map((p) => ({ pv: p, maxTroops: maxTroops(p) })); + .map((p) => ({ + pv: p, + maxTroops: maxTroops(p), + cities: p.totalUnitLevels(UnitType.City), + })); switch (this._sortKey) { ... case "cities": - sorted.sort((a, b) => - compare( - a.pv.totalUnitLevels(UnitType.City), - b.pv.totalUnitLevels(UnitType.City), - ), - ); + sorted.sort((a, b) => compare(a.cities, b.cities)); break;Then reuse
playerCache.citieswhen building eachEntry(line ~150), same asmaxTroopsis reused today.Also applies to: 137-157, 171-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/hud/layers/Leaderboard.ts` around lines 86 - 130, Cache each player’s city count once in the Leaderboard update flow, the same way `maxTroops` is already cached in `PlayerViewTroopsCache`. Add a `cities` field to the per-player cache built from `this.game.playerViews()` and compute it from `totalUnitLevels(UnitType.City)` there, then reuse `playerCache.cities` both in the `"cities"` branch of the sort and when building each leaderboard entry. Keep the change localized around the `compare` helper, the `sorted` cache setup, and the entry-building logic so `totalUnitLevels(UnitType.City)` is not recomputed repeatedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/client/hud/layers/Leaderboard.ts`:
- Around line 86-130: Cache each player’s city count once in the Leaderboard
update flow, the same way `maxTroops` is already cached in
`PlayerViewTroopsCache`. Add a `cities` field to the per-player cache built from
`this.game.playerViews()` and compute it from `totalUnitLevels(UnitType.City)`
there, then reuse `playerCache.cities` both in the `"cities"` branch of the sort
and when building each leaderboard entry. Keep the change localized around the
`compare` helper, the `sorted` cache setup, and the entry-building logic so
`totalUnitLevels(UnitType.City)` is not recomputed repeatedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ab4091e-8319-4943-a69d-49f63cce1864
📒 Files selected for processing (17)
resources/lang/en.jsonsrc/client/hud/layers/Leaderboard.tssrc/client/render/types/Renderer.tssrc/client/view/GameView.tssrc/client/view/PlayerView.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdateUtils.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tstests/GameUpdateUtils.test.tstests/PackedPlayerUpdates.test.tstests/PlayerUpdateDiff.test.tstests/client/render/frame/derive/nuke-telegraphs.test.tstests/client/render/frame/derive/player-status.test.tstests/client/view/GameView.test.tstests/util/viewStubs.ts
Author
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add approved & assigned issue number here:
Resolves #4074
Description:
Leaderboard update part 2. Adds the new leaderboard stat items on a branch based directly on main.
This adds rolling 60 second Gold/min tracking, sends it through packed player updates, and displays Gold/min, current Troops, and Cities in the in-game leaderboard.
The configurable column chooser is split out into #4453.
Testing:
npx vitest tests/PackedPlayerUpdates.test.ts tests/PlayerUpdateDiff.test.ts tests/GameUpdateUtils.test.ts tests/client/view/GameView.test.ts --run;npx tsc --noEmit; targeted ESLint/PrettierPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
jsshap