UnitLayer: interpolate unit rendering for non stationary units #2462
Conversation
WalkthroughAdds an interpolation rendering pipeline to UnitLayer: offscreen interpolation canvas, tick-based timing tied to replay speed, per-tick alpha interpolation, new draw/render helpers, and moves several fast-moving unit types into the unified interpolated rendering path. Changes
Sequence Diagram(s)sequenceDiagram
participant Replay as ReplaySpeedSystem
participant Tick as TickSystem
participant Render as RenderLoop
participant Layer as UnitLayer
participant Canvas as InterpolationCanvas
Replay->>Layer: ReplaySpeedChangeEvent
Layer->>Layer: onReplaySpeedChange() -> update tickIntervalMs, reset timing
loop Render frames (e.g., 60 FPS)
Render->>Layer: renderLayer()
Layer->>Layer: updateInterpolatedUnits()
Note over Layer: computeTickAlpha(now(), lastTickTimestamp)
Layer->>Canvas: drawSpriteAtPosition(interpolatedPos, alpha)
Layer->>Canvas: renderShellsAndWarheads()
Layer->>Render: composite interpolation canvas onto main display
end
loop Server ticks (~100ms)
Tick->>Layer: tick() (server update)
Layer->>Layer: update unit lastTile / tile, set lastTickTimestamp
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks✅ Passed checks (5 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}
There was a problem hiding this comment.
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/client/graphics/layers/UnitLayer.ts (1)
487-545: Usesprite.heightfor the drawn height to avoid distortionIn
drawSpriteAtPositionyou call:context.drawImage(sprite, offsetX, offsetY, sprite.width, sprite.width);Using
sprite.widthfor both width and height will stretch or squash non‑square sprites.Unless all sprites are guaranteed square, this is safer:
- context.drawImage(sprite, offsetX, offsetY, sprite.width, sprite.width); + context.drawImage( + sprite, + offsetX, + offsetY, + sprite.width, + sprite.height, + );The rest of the refactor (centralizing sprite drawing,
snapToPixel, alternate view color handling) looks good and keeps the logic in one place.
🧹 Nitpick comments (3)
src/client/graphics/layers/UnitLayer.ts (3)
37-38: Interpolation canvas wiring is correct; optional lazy allocation for perfThe extra canvas/context and the draw order (trail → base units → interpolation) look correct and easy to reason about. No functional issues here.
If you later see memory or perf pressure on low-end devices, one small improvement is to create / size
interpolationCanvasonly when there is at least one unit ininterpolatedUnitTypes, and skip thedrawImagewhen it would be empty. That keeps the hot path a bit lighter without changing behavior.Also applies to: 218-243, 259-271
53-67: Avoid hard‑codingtickIntervalMsand double‑check time baseRight now the interpolation factor is tied to a local, hard‑coded
tickIntervalMs = 100and anow()based onperformance.now()/Date.now(). This works, but it couples the client to a magic number.To keep things robust if the server tick ever changes, or if the client wants to adjust tick length, I suggest:
- Pass the tick interval into
UnitLayer(for example fromGameViewor config), instead of hard‑coding100.- Make sure the same time base is used for both
tick()andcomputeTickAlpha()(it looks consistent here, but worth confirming thattick()is called on the same schedule the server uses).A possible sketch:
- private readonly tickIntervalMs: number = 100; + private readonly tickIntervalMs: number; constructor( private game: GameView, private eventBus: EventBus, transformHandler: TransformHandler, + tickIntervalMs: number, ) { this.theme = game.config().theme(); this.transformHandler = transformHandler; + this.tickIntervalMs = tickIntervalMs; this.lastTickTimestamp = this.now(); }Then keep
computeTickAlpha()as is.Also applies to: 75-76, 83-84, 700-716
547-609: Interpolation pipeline is sound; consider clarifying assumptions and tightening hot pathThe overall interpolation pipeline (
interpolatePosition→updateInterpolatedUnits→renderShell/renderWarhead/drawSpriteAtPosition) looks correct and clean. A few focused suggestions:
Clarify
lastTile()vstile()assumption
interpolatePosition()assumesunit.lastTile()is the previous tick andunit.tile()is the current tick. That is important for smooth motion. It would be good to confirm thatUnitViewguarantees this, and maybe leave a short comment:// Assumes lastTile() = previous tick, tile() = current tick.Short‑circuit when there is nothing to interpolate
Right now
updateInterpolatedUnits()runs every render, even whenalphais 0 (just after a tick) or when there are no units of the interpolated types. A small guard can cut work in the hot loop:
- const alpha = this.computeTickAlpha();
- const missiles = this.game.units(...this.interpolatedUnitTypes);
- const alpha = this.computeTickAlpha();
- if (alpha <= 0) {
return;- }
- const units = this.game.units(...this.interpolatedUnitTypes);
and rename `missiles` to something generic like `units` for readability, since the array now includes warships, trains, trade ships, etc. 3. **Theme‑aware highlight color** `getInterpolatedSpriteColor()` hard‑codes `colord("rgb(200,0,0)")` for warships with a target. To keep styling consistent with the theme system, consider getting this color from the theme (or from a small config constant) instead of a magic string, so it can be tuned per skin. 4. **Nice compositional reuse in interpolation helpers** The reuse between `renderShell`/`renderWarhead`, `drawInterpolatedSquare`, `drawInterpolatedSegment` and `resolveInterpolatedColor` is good and idiomatic TypeScript composition over inheritance. If you ever want to add more interpolated shapes, you already have a small, focused “render primitives” API here. Also applies to: 611-639, 641-698, 700-709 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e1db8dc6782370cb1412a2dee1a9329d5f18d048 and e74d1f287bfc959443275b8d97743cbaa7760880. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/client/graphics/layers/UnitLayer.ts` (8 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (3)</summary> <details> <summary>📚 Learning: 2025-10-18T11:00:57.142Z</summary>Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.**Applied to files:** - `src/client/graphics/layers/UnitLayer.ts` </details> <details> <summary>📚 Learning: 2025-05-18T23:36:12.847Z</summary>Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.**Applied to files:** - `src/client/graphics/layers/UnitLayer.ts` </details> <details> <summary>📚 Learning: 2025-05-18T23:36:12.847Z</summary>Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.**Applied to files:** - `src/client/graphics/layers/UnitLayer.ts` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>src/client/graphics/layers/UnitLayer.ts (2)</summary><blockquote> <details> <summary>src/core/game/GameView.ts (3)</summary> * `UnitView` (44-179) * `unit` (658-660) * `targetable` (86-88) </details> <details> <summary>src/core/configuration/Config.ts (1)</summary> * `Theme` (188-211) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>src/client/graphics/layers/UnitLayer.ts (1)</summary><blockquote> `318-320`: **The review comment misunderstands the rendering architecture—this is not a bug.** The codebase intentionally uses separate canvases for different rendering purposes: - **Main canvas** (`this.context`): cleared and redrawn for units handled in `onUnitEvent()` (TransportShip) - **Interpolation canvas** (`this.interpolationContext`): renders all `interpolatedUnitTypes` (Warship, Train, TradeShip, Shell, SAMMissile, and others) with smooth frame-interpolated movement - **Trail canvas**: nuke trails - **Structure canvas**: buildings and static structures All mobile unit types in the game are accounted for: - TransportShip: redrawn via `onUnitEvent()` → `handleBoatEvent()` → `drawSprite()` - Warship, Train, TradeShip, Shell, SAMMissile, MIRVWarhead: redrawn via `updateInterpolatedUnits()` on interpolation canvas - AtomBomb, HydrogenBomb, MIRV: redrawn via `onUnitEvent()` → `handleNuke()` for trails Units like tanks, infantry, and planes do not exist in the game (checked `UnitType` enum). The proposed fix would incorrectly redraw interpolated units on the main canvas where they don't belong—breaking the intended smooth movement rendering design. > Likely an incorrect or invalid review comment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| private renderShell(unit: UnitView, position: { x: number; y: number }) { | ||
| const rel = this.relationship(unit); | ||
| const color = unit.owner().borderColor(); | ||
| this.drawInterpolatedSquare(position, rel, color, 1, 1); | ||
| this.drawInterpolatedSquare(position, rel, color, 2, 0.4); | ||
|
|
||
| const last = { | ||
| x: this.game.x(unit.lastTile()), | ||
| y: this.game.y(unit.lastTile()), | ||
| }; | ||
| if (last.x !== position.x || last.y !== position.y) { | ||
| this.drawInterpolatedSegment(last, position, rel, color, 0.7); | ||
| } | ||
| } | ||
|
|
||
| private renderWarhead(unit: UnitView, position: { x: number; y: number }) { | ||
| const rel = this.relationship(unit); | ||
| const color = unit.owner().borderColor(); | ||
| this.drawInterpolatedSquare(position, rel, color, 1, 1); | ||
| this.drawInterpolatedSquare(position, rel, color, 2, 0.35); | ||
|
|
||
| const last = { | ||
| x: this.game.x(unit.lastTile()), | ||
| y: this.game.y(unit.lastTile()), | ||
| }; | ||
| if (last.x !== position.x || last.y !== position.y) { | ||
| this.drawInterpolatedSegment(last, position, rel, color, 0.5); | ||
| } | ||
| } |
There was a problem hiding this comment.
How are these alphas chosen? Why these numbers specifically?
There was a problem hiding this comment.
Just by visually inspecting. The idea is always to have a solid square, then a somewhat transparent trail. Simply played around with these values until they looked good to me.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/graphics/layers/UnitLayer.ts (1)
635-663: Explain or extract magic alpha values.The alpha values (Shell: 1, 0.4, 0.7; Warhead: 1, 0.35, 0.5) appear arbitrary. Extract these to named constants with comments explaining the visual effect, or document why these specific values were chosen.
+ private readonly SHELL_CORE_ALPHA = 1; // Fully opaque core + private readonly SHELL_GLOW_ALPHA = 0.4; // Semi-transparent outer glow + private readonly SHELL_TRAIL_ALPHA = 0.7; // Trail connecting positions + + private readonly WARHEAD_CORE_ALPHA = 1; + private readonly WARHEAD_GLOW_ALPHA = 0.35; + private readonly WARHEAD_TRAIL_ALPHA = 0.5; private renderShell(unit: UnitView, position: { x: number; y: number }) { const rel = this.relationship(unit); const color = unit.owner().borderColor(); - this.drawInterpolatedSquare(position, rel, color, 1, 1); - this.drawInterpolatedSquare(position, rel, color, 2, 0.4); + this.drawInterpolatedSquare(position, rel, color, 1, this.SHELL_CORE_ALPHA); + this.drawInterpolatedSquare(position, rel, color, 2, this.SHELL_GLOW_ALPHA); const last = { x: this.game.x(unit.lastTile()), y: this.game.y(unit.lastTile()), }; if (last.x !== position.x || last.y !== position.y) { - this.drawInterpolatedSegment(last, position, rel, color, 0.7); + this.drawInterpolatedSegment(last, position, rel, color, this.SHELL_TRAIL_ALPHA); } }
🧹 Nitpick comments (2)
src/client/graphics/layers/UnitLayer.ts (2)
628-633: Consider extracting the hardcoded color.The red color
rgb(200,0,0)is hardcoded for attacking warships. Consider extracting this to a named constant or theme color for maintainability.+ private readonly ATTACKING_WARSHIP_COLOR = colord("rgb(200,0,0)"); + private getInterpolatedSpriteColor(unit: UnitView): Colord | undefined { if (unit.type() === UnitType.Warship && unit.targetUnitId()) { - return colord("rgb(200,0,0)"); + return this.ATTACKING_WARSHIP_COLOR; } return undefined; }
712-722: Make default case explicit for readability.Remove the redundant
case Relationship.Enemy:and let it fall through todefault:for clarity.private getAlternateViewColor(relationship: Relationship): Colord { switch (relationship) { case Relationship.Self: return this.theme.selfColor(); case Relationship.Ally: return this.theme.allyColor(); - case Relationship.Enemy: default: return this.theme.enemyColor(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/UnitLayer.ts(11 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/UnitLayer.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/graphics/layers/UnitLayer.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/client/graphics/layers/UnitLayer.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/UnitLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/UnitLayer.ts (5)
src/core/game/GameView.ts (3)
UnitView(44-179)unit(658-660)targetable(86-88)src/core/game/GameMap.ts (1)
TileRef(3-3)src/client/utilities/ReplaySpeedMultiplier.ts (1)
defaultReplaySpeedMultiplier(8-8)src/client/InputHandler.ts (1)
ReplaySpeedChangeEvent(113-115)src/client/graphics/layers/TerritoryLayer.ts (1)
alternateViewColor(520-535)
🔇 Additional comments (16)
src/client/graphics/layers/UnitLayer.ts (16)
12-12: LGTM!New imports for replay speed handling are appropriate for the interpolation timing system.
Also applies to: 21-24
42-43: LGTM!Offscreen interpolation canvas follows the same pattern as transportShipTrailCanvas above.
58-69: LGTM!The unit types list is clear and focused on fast-moving units that benefit from interpolation.
70-74: LGTM!Timing fields properly initialized; the 100ms default is overwritten by config in the constructor.
83-88: LGTM!Tick interval now correctly sourced from config rather than hardcoded.
95-104: LGTM!Timestamp tracking and runtime config synchronization are correct.
117-119: LGTM!Event subscription for replay speed changes is properly set up.
243-243: LGTM! Consider performance profiling.Interpolation update runs every render frame, which is necessary for smooth animation but could impact performance with many units. If you encounter frame drops, consider optimizing by culling offscreen units or batching draw calls.
Also applies to: 258-266
283-294: LGTM!Interpolation canvas initialization follows the established pattern.
511-569: LGTM!Good refactoring that separates position calculation from rendering. The
snapToPixelparameter correctly handles both pixel-aligned and smooth interpolated rendering.
571-584: LGTM!Linear interpolation implementation is correct.
665-710: LGTM!Helper methods for interpolated rendering are well-structured and correctly handle alternate view mode.
724-733: LGTM!Alpha computation correctly handles edge cases (zero interval, negative elapsed) and properly clamps to [0, 1].
735-748: LGTM!Replay speed handling correctly adjusts interpolation timing and resets the timestamp to prevent visual jumps.
750-755: LGTM!Standard cross-environment timing pattern.
586-626: Critical: TransportShip double-rendering bug.TransportShip is rendered in TWO places:
- Line 469 in
handleBoatEventdraws sprite at current tile on main canvas- Lines 617-623 draw sprite at interpolated position on interpolation canvas
When alpha < 1, these positions differ, so the user sees two TransportShip sprites for the same unit.
Fix: Remove the
drawSprite(unit)call fromhandleBoatEventfor units ininterpolatedUnitTypes, or skip thehandleBoatEventcase entirely for TransportShip since it's now in the interpolation path.Apply this diff to fix the double-rendering:
case UnitType.TransportShip: - this.handleBoatEvent(unit); + // TransportShip is now rendered via interpolation + if (!unit.isActive()) { + this.clearTrail(unit); + } else { + const rel = this.relationship(unit); + if (!this.unitToTrail.has(unit)) { + this.unitToTrail.set(unit, []); + } + const trail = this.unitToTrail.get(unit) ?? []; + trail.push(unit.lastTile()); + this.drawTrail(trail.slice(-1), unit.owner().territoryColor(), rel); + } break;Likely an incorrect or invalid review comment.
|
Can you look at performance cost? |
|
@jrouillard some things about the performance were discussed here too just fyi (possible move of unitlayer to webgl by scamiv/vimacs sounds promising) https://discord.com/channels/1359946986937258015/1359946989046989063/1439565520591650881 |
4 participants
Resolves #803
Description:
Interpolates position for moving units. Functionality is working, want to gather feedback and understand performance implications before moving ahead.
interpolated.unit.rendering.transcoded.mp4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
newyearnewphil / [CYN] super mario