-
Notifications
You must be signed in to change notification settings - Fork 714
Feature: Improve Spawn Color Highlighting #2271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Improve Spawn Color Highlighting #2271
Conversation
…e.g. from { r: #, g: #, b: # } to 'rgb(#,#,#)'
… ally, red for enemy, and standard for enemy in FFA. Updated Theme interface with new methods. Updated color selection in spawnHighligh()
… others in FFA are default yellow.
…lor, Humans in FFA are default yellow
…describes current spawn color determination
…er). Added a semi-transparent permanent ring around the starting position. This ring has a solid inner radius and gradients to transparent at the outer radius. On top of the transparent ring, draw a breathing ring with outer radius a function of 'time'. The breathing ring is solid throughout. Both rings use the 'spawnHighlightSelfColor' and both rings have a fully transparent inner circle where the starting territory is drawn.
… b: # }' to prevent significant code changes.
WalkthroughThis PR implements improved spawn highlighting with team-based and self-specific colors. It refactors the TerritoryLayer spawn highlight invocation timing and breathing ring rendering geometry, expands the fallback color palette, and adds new spawn highlight color properties to the Theme interface and PastelTheme implementation. Changes
Sequence Diagram(s)sequenceDiagram
participant tick as tick()
participant spawn as Spawn Phase
participant highlight as Spawn Highlight
participant render as Render
tick->>spawn: Check if spawn phase
alt Spawn Phase Active
spawn->>highlight: Invoke spawn highlight
alt FFA (team() === null)
highlight->>highlight: Use default spawn color
else Team Game
highlight->>highlight: Use highlighted player's team color
end
highlight->>render: Draw breathing ring with team color
else Not Spawn Phase
tick->>render: Standard rendering
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple files with mixed complexity: the breathing ring refactor requires careful verification of timing and geometry logic, color logic branches need validation across FFA and team scenarios, and the palette changes demand review for visual coherence. However, the changes follow consistent patterns and are primarily additive with clear separation of concerns. Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 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 |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/configuration/PastelTheme.ts (1)
54-66: Probable method mismatch: assignTeamPlayerColor may not existterritoryColor calls this.teamColorAllocator.assignTeamPlayerColor(team, player.id()). Per past learnings, ColorAllocator exposes assignColor(id: string) and assignTeamColor(team: Team), not assignTeamPlayerColor. This would cause a compile error unless you added that method in this PR.
If per‑player variation inside a team is not required, return the team color:
- if (team !== null) { - return this.teamColorAllocator.assignTeamPlayerColor(team, player.id()); - } + if (team !== null) { + return this.teamColor(team); + }If you do want per‑player team shades, either add assignTeamPlayerColor to ColorAllocator or derive a shade deterministically from player.id().
Based on learnings
🧹 Nitpick comments (4)
src/core/configuration/PastelTheme.ts (1)
41-48: New spawn highlight getters are added but currently unused outside selfspawnHighlightTeamColor() and spawnHighlightEnemyColor() are exposed but not used in TerritoryLayer (team mode uses theme.teamColor(team) instead). Either wire these in the layer logic or drop them for now to keep the interface lean. If you keep them, consider a single function with a typed union parameter:
- spawnHighlightColorFor(role: "self" | "team" | "enemy"): Colord
Also applies to: 158-169
src/client/graphics/layers/TerritoryLayer.ts (3)
187-195: Team color choice is fine; consider using theme’s new spawn getters or keep API minimalCurrent logic uses theme.teamColor(team) in team games, which matches the PR summary. If you intend to keep the new Theme getters, consider using them here (friendly vs enemy) to justify the API:
- } else if (myPlayer !== null && myPlayer !== human) { - // In Team games, the spawn highlight color becomes that player's team color - const team = human.team(); - if (team !== null) color = this.theme.teamColor(team); + } else if (myPlayer !== null && myPlayer !== human) { + const team = human.team(); + if (team !== null && myPlayer.team() !== null) { + color = human.isFriendly(myPlayer) + ? this.theme.spawnHighlightTeamColor() + : this.theme.spawnHighlightEnemyColor(); + } }Optional, non‑blocking.
219-233: Naming nit: “self” color used for focused player, which may not be “me”drawFocusedPlayerHighlight uses spawnHighlightSelfColor() for whichever player is focused. Consider renaming to spawnHighlightFocusedColor() or pass myPlayer check if you want “self” to mean my player only. Behavior is OK as is; this is just naming clarity.
563-609: Canvas gradient transparency: prefer rgba(...) over 8‑digit hex for broader compatibilitySome canvases don’t accept #RRGGBBAA everywhere. Use rgba strings from colord to avoid browser quirks.
- const transparent = color.toHex() + "00"; - const c = color.toHex(); + const transparent = color.alpha(0).toRgbString(); + const c = color.toRgbString();Optional: make the “~1px solid” part independent of ring thickness:
- radGrad.addColorStop(0.01, c); - radGrad.addColorStop(0.1, c); + const solidFrac = Math.max(1 / Math.max(1, maxRad - minRad), 0.05); // ~1px or 5% + radGrad.addColorStop(solidFrac, c); + radGrad.addColorStop(Math.min(0.1, solidFrac * 2), c);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)src/client/graphics/layers/TerritoryLayer.ts(4 hunks)src/core/configuration/Colors.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/PastelTheme.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/core/configuration/Config.tssrc/core/configuration/PastelTheme.ts
📚 Learning: 2025-06-06T15:42:21.903Z
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:20-21
Timestamp: 2025-06-06T15:42:21.903Z
Learning: In ColorAllocator class, all methods (assignBotColor, assignPlayerColor, assignTeamColor) use the constructor colors from this.availableColors. Only the predefined team cases in assignTeamColor use hardcoded global colors, while the default case uses constructor colors.
Applied to files:
src/core/configuration/PastelTheme.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/TerritoryLayer.ts (2)
src/core/game/GameView.ts (2)
myPlayer(585-587)team(333-335)src/core/game/PlayerImpl.ts (1)
team(740-742)
🔇 Additional comments (4)
.gitignore (1)
13-13: LGTM!Adding
.idea/to gitignore is a standard best practice for JetBrains IDE projects. This prevents IDE-specific metadata from being committed to the repository.src/client/graphics/layers/TerritoryLayer.ts (1)
76-80: LGTM: move spawnHighlight to start of tick()Clearer control flow and only runs in spawn phase.
src/core/configuration/Config.ts (1)
201-203: No breaking change concern—methods already implemented and in useThe review comment's premise is incorrect. Only one class implements Theme (PastelTheme), and it already has all three methods implemented (lines 159, 163, 167 in PastelTheme.ts). Additionally,
spawnHighlightSelfColor()is actively used in TerritoryLayer.ts:232, so the API addition already pays off.The suggested refactor to a typed union (
spawnHighlightColorFor(role: "self" | "team" | "enemy"): Colord) is a valid optional improvement for cleaner API design, but not required.Likely an incorrect or invalid review comment.
src/core/configuration/Colors.ts (1)
263-570: Update comment count: 523 colors, not 100The comment at line 263 states "Currently 100 colors" but the array contains 523
colord()elements. Update this count or remove the number to keep documentation accurate.Verify contrast handling for very dark fallback colors
The code uses fixed dark text colors (black for humans, dark gray otherwise) in
PastelTheme.textColor(). IffallbackColorsentries with very low RGB values (like{r: 35, g: 0, b: 0}) are applied as backgrounds, they may create poor contrast with the dark text. Confirm how these fallback colors are used in the UI and whether contrast validation is needed.Consider programmatic generation (optional)
The large hardcoded array could be generated from ranges and steps, then exported as a frozen constant. This would reduce file size and maintenance burden, though it is optional.
evanpelle
left a comment
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.
Thanks!
This PR improves spawn highlighting to identify self and friend/foe.
Suggested Label: Feature
Suggested Milestone: v26 or v27
Description:
This PR changes the behavior for spawn color highlighting and addresses issue #2270.
Currently, the user's spawn highlight color is the same as all other players in FFA and the same as all enemies in Team games. Although "breathing" rings were added recently for the user's spawn highlight, this can still be difficult to find on the map, especially with large player counts.
This PR modifies how spawn highlights are drawn for the user and for allies and enemies in Team games.
User Spawn Color Updates
First, an additional spawn highlight color was added for the user in
src/core/configuration/PastelTheme.ts:_spawnHighlightSelfColor. This is defined to be white (0xFFFFFF).Second, the breathing ring was modified to improve visibility (all in
src/client/graphics/layers/TerritoryLayer.ts) :radius = 8pixels from the player's center point8to24pixels from the player's center point24pixelsradius = 8pixels from the center pointOther Player Spawn Color Updates
In FFA games, no change is made to the spawn color highlight of other players. It remains
rgb(255,213,79).In team games, the spawn color highlight of other players is updated to use their team colors. For example, a player on the red team will have a red spawn highlight, while a player on the purple team will have a purple spawn highlight.
Both of these changes are handled with a simple update in
src/client/graphics/layers/TerritoryLayer.tswithin thespawnHighlight()method:Attached Images
Three images have been attached. The three images show a progression of the "breathing" user highlight. They also show the team-specific highlights of players on other teams. (Note that Nations in the private game were assigned teams but do not have spawn highlights).
Misc.
.idea/to.gitignorebecause I use JetBrains IDEsfallbackColorslist. Right now it has a lot of green colors, so I added equivalents for red, blue, cyan, magenta, and yellow.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
GlacialDrift