Fix/stripe direction on quarter change - #28
Conversation
ChessMess
left a comment
ChessMess
left a comment
There was a problem hiding this comment.
🤖 Claude Code Opus — AI Review findings. Generated by AI, reviewed against the actual diff and CI. Sanity-check anything before acting on it.
Verdict: approve ✅ (with follow-ups)
The core math is right. I enumerated all 8 (team, onOffense, offenseMovingRight) combinations and stripeXMultiplier comes out exactly equal to teamSign * directionSign from the movement code (src/game/scenes/BaseGameScene.js:939-940), so facingAngle is identically the player's real movement heading in every case. The QB drop-back case (directionSign = -.01 * endzoneDir) correctly keeps the QB facing forward while moving backward. And changePossession flips possession and offenseMovingRight together, so the multiplier correctly does not change on a turnover — only on a quarter swap. Nice invariant.
Removals verified safe: no remaining references to rotationHandle, setBaseAngle, or initialAngle anywhere in src/ or tests/, and tests/unit/config.test.js:104 iterates config.colors generically, so dropping colors.rotationHandle doesn't break it. Dropping the per-player circle also fixes a small leak — 22 of them were created and never destroyed.
CI: test passing on head 374ae1a.
Follow-ups below. Only #1 is worth doing before this ships, and even that is arguable since it's a pre-existing gap this PR just doesn't close.
1. Resume-from-save boots with stale facing — same bug, different entry path (medium)
stripeXMultiplier is only ever assigned in the Player constructor (default 1) and in resetPosition. Nothing in the boot path calls resetPosition:
init() → loadGame() (BaseGameScene.js:117) → create() → createPlayers() → changeformation() / changeDefensiveFormation() ×2, which route to FormationManager.resetPlayerAngle (FormationManager.js:152) — that resets currentAngle but never touches the multiplier. Both toggles do reassign player.fillColor (setPlayerBallCarrier / defTeamColor), so all 22 get repainted at boot with the stale default of 1.
resetPosition has exactly four callers — PlayStateManager.changePossession, PlayStateManager.nextPlay, and StandardGameScene.endQuarter ×2 — none of which run during init()/create().
Exact trigger: (possession === "Home") !== offenseMovingRight, which is half of all reachable states. Repro: get tackled during Q2 with Home still holding the ball (offenseMovingRight === false, persisted by saveGame), quit, then "Resume Game" — all 22 players and the rotation arrow face backwards until the first nextPlay(). Exactly the bug this PR fixes. The constructor default of 1 happens to be correct only for a fresh Q1 boot.
Caveat: this is from a static call-graph trace, not a reproduction in a running scene. Worth confirming before acting on it.
2. The multiplier isn't per-player at all — it collapses to one scene-level value (low, but it makes #1 a one-liner)
Checked across all four (possession, offenseMovingRight) states: the derivation in Player.resetPosition yields the same value for all 22 players, and reduces exactly to
((possession === "Home") === offenseMovingRight) ? 1 : -1The onOffense / absoluteFacing / per-team flip cancels out. It's also identically teamSign * directionSign from BaseGameScene.js:939-940 — so the same direction rule now has two spellings in two files that have to stay in sync.
That makes the fix for #1 cheap: make stripeXMultiplier a getter on Player (or one scene field set in swapTeamDirection/changePossession) and keep only the repaint in resetPosition. A derived value can't go stale on resume, and there's one copy of the rule instead of three.
3. Dead defensive guards (low)
const stripeXMultiplier = this.stripeXMultiplier ?? 1;in thefillColorsetter — the constructor setsstripeXMultiplierbefore the firstfillColorwrite.if (this._fillColor !== undefined)inresetPosition—_fillColoris always set in the constructor, andresetPositioncan't run before construction.gameObject.facingAngle ?? 0at bothBaseGameScenecall sites — both are already guarded toentityType === "Player"/this.draggedPlayer.
One line each, none load-bearing.
4. The flip expression is re-spelled at the drag site (low)
player.currentAngle = angle - (player.stripeXMultiplier === -1 ? Math.PI : 0);That's the inverse of the facingAngle getter, hand-inlined. A set facingAngle(a) next to the getter would keep the flip in one place and make the call site read player.facingAngle = angle.
5. "Movement is untouched" isn't quite accurate — there's a second fix hiding in here (low)
The drag handler changed from setAngle(player.body, angle) to setAngle(player.body, player.currentAngle). For the flipped cases the resulting movement heading is now angle instead of angle + PI — a hand-rotated player now runs toward the arrow rather than away from it. (Confirmed PlayStateManager.startPlay never resets currentAngle, so a pre-snap rotation does survive into the play.)
That's a real behavior fix — Away players' arrows pointed backwards before — but it is a movement change, and nothing in the new tests covers it. Worth calling out in the PR description, since it's a good fix that currently reads as "no change".
6. The test helper's multiplier assertion restates the implementation (low)
const absoluteFacing = onOffense === scene.offenseMovingRight ? 1 : -1;
const expectedMultiplier = player.team === 'Home' ? absoluteFacing : -absoluteFacing;
expect(player.stripeXMultiplier).toBe(expectedMultiplier);That's a copy of the production formula — if the formula is wrong, the test agrees with it. The expectedFacingAngle assertion right below it is stated independently (offense faces the endzone it attacks: 0 right / PI left) and is the one that actually catches the bug. Suggest dropping the multiplier assertion so there's only one copy of the formula to maintain. The baseAngle assertion is a good independent invariant — keep that one.
Also uncovered: nothing asserts the stripes were repainted. Delete the this.fillColor = this._fillColor line and all three new tests still pass while the visible stripes stay stale. That line is genuinely load-bearing — the Q1→Q2 / Q3→Q4 branch of endQuarter calls only resetPosition, with no resetAllPlayerColors before it, unlike the halftime branch.
7. Nit, pre-existing but adjacent: the actually-dead rotation-handle branch survived
BaseGameScene.js:602-605 is unreachable — the arrow sprite has .player set at line 461, so the dragend early-return at line 585 always fires first. Consequences: draggingRotationHandle is never cleared (it holds a destroyed sprite after deselect) and the arrow stays at alpha 0.7 after you rotate a player. Not introduced here, but the remove dead rotation handler code commit is the natural place for it.
Method: read Player.js, BaseGameScene.js (drag/dragstart/dragend/update/movement), PlayStateManager.js, StandardGameScene.js, FormationManager.js, PlayRecorder.js, saveGame.js and applied the diff by hand; enumerated the 8 direction combinations numerically; grepped every reference to the removed and added symbols across src/ and tests/. Did not run the suite locally — CI covers the PR's own tests.
…er to fix direction after game resume Player facing (front stripes + rotation arrow) is now computed live from currentAngle plus a getPlayerUIDirection() multiplier derived from the scene's (possession, offenseMovingRight, team) state — the same rule movement already uses — instead of a stored value only refreshed in resetPosition. A resume boot no longer comes up facing backwards. Also fix the dragend rotation-handle cleanup that never ran, drop the associated dead code, and hoist veerParams out of the per-player update loop. - stripeXMultiplier is now Player.getPlayerUIDirection(), a getter derived live from (possession, offenseMovingRight, team); it collapses to ((possession === "Home") === offenseMovingRight) ? 1 : -1, identical to the movement code's teamSign * directionSign, so it can never go stale — not even on a resume boot, where the constructor's first paint reads loaded state. - facingAngle getter/setter on Player, derived from currentAngle + the live multiplier; the rotation-arrow handle and drag-to-rotate code now use it, so the flip logic lives in one place (player.facingAngle = angle at the drag site). - resetPosition keeps only the load-bearing fillColor = _fillColor repaint and drops the stored-multiplier derivation. - The rotation-arrow's dragend cleanup never ran (the arrow has .player, so the early-return fired first) — dropping the handle left draggingRotationHandle set and the arrow stuck at alpha 0.7. Cleanup now runs inside the .player branch. - Hand-rotated players now run toward the arrow in the flipped (post-swap) direction too, where they previously ran away from it. - Dead code removed: the unused rotationHandle circle and its config.json color, never-read initialAngle, never-called Player.setBaseAngle, the never-read rotatingPlayer field, the dead dragend cleanup branch, and three dead defensive guards (?? 1, _fillColor !== undefined, facingAngle ?? 0). - Hoisted the frame-constant veerParams object out of the per-player update loop (1 allocation per frame instead of 22). Tests: npm test 118/118, npm run test:e2e 4/4. Added independent quarter-change facing assertions and a resume-from-save boot test.
a8f5202 to
f2de6a1
Compare
|
f2de6a1 commit body has an overview of the changes but it addresses the comments from #28 (review) and fixes the players direction stripes still facing the wrong way when resuming from a save in the 2nd or 4th quarter |
2 participants
Summary
Fixes the player direction indicators — the front stripes on each player capsule and the rotation-arrow handle that appears when a player is selected — not flipping after a quarter change. The visuals stayed fixed per team after Q1→Q2 and Q3→Q4 swapped the attack direction, so the offense looked like it was facing the wrong way while the teams changed endzones.
Root cause
Introduced in #17, which replaced the plain rectangle players with a rounded capsule + front stripes. Player facing was derived from baseAngle, hardcoded at construction (Home = 0, i.e. always facing right; Away = PI, always left). Movement already handled direction flips through directionSign in applyMovementForce, but the visuals read currentAngle/baseAngle directly, which never changed when swapTeamDirection() flipped the endzone at quarter boundaries. So:
Fix
Kept baseAngle/currentAngle team-fixed (movement must not change) and made only the visuals direction-aware:
Movement is untouched — it already used directionSign, so players still run the correct way after every swap.
Testing