Addressing PR comments - #76
Conversation
…optimize ISR, extract game loop helper Co-authored-by: 28pins <262898015+28pins@users.noreply.github.com>
Co-authored-by: 28pins <262898015+28pins@users.noreply.github.com>
…ines to 11) Co-authored-by: 28pins <262898015+28pins@users.noreply.github.com>
Claude
AI
changed the title
|
@copilot undo everything in the Gamer lib except the showsScore work |
Done. Reverted all Gamer lib changes (capacitive touch loop, ISR consolidation, updateDisplay optimization) back to original, keeping only the removal of the duplicate |
|
@claude[agent] undo everything in the gamer lib minis the show score bit |
Co-authored-by: 28pins <262898015+28pins@users.noreply.github.com>
Done. Reverted all Gamer library optimizations back to original except for the showScore removal. The three optimizations (ISR consolidation, capTouch loop, and updateDisplay optimization) have been restored to their pre-optimization state. Commit: 15c47a6 |
Claude
AI
changed the title
There was a problem hiding this comment.
Pull request overview
This PR focuses on addressing prior review feedback by reducing duplicated game-loop preamble code across games, while also refactoring parts of Breakout and removing a duplicated score-display API from the Gamer library.
Changes:
- Added a shared
updateGameInput()helper inTWSUGamerPlus.inoand updated multiple games to use it. - Refactored Breakout collision/bounce logic and simplified checkerboard block-propagation.
- Removed
Gamer::showScore(int)from the library, relying on the sketch-level score rendering instead.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
TWSUGamerPlus.ino |
Adds updateGameInput() helper used as a common loop preamble. |
src/games/tetris.h |
Switches loop preamble to updateGameInput(). |
src/games/snake.h |
Switches loop preamble to updateGameInput() and keeps manual display clearing. |
src/games/simon.h |
Switches loop preamble to updateGameInput(). |
src/games/flappy.h |
Switches loop preamble to updateGameInput() (but retains a redundant tone stop). |
src/games/dino.h |
Switches loop preamble to updateGameInput(). |
src/games/conway.h |
Switches loop preamble to updateGameInput(). |
src/games/alien.h |
Uses updateGameInput() inside the movement-delay loop. |
src/games/breakout.h |
Adds isFree() helper and refactors physics + block-propagation logic. |
Gamer.h |
Removes showScore(int) from the public Gamer API. |
Gamer.cpp |
Removes the Gamer::showScore(int) implementation. |
| // Common game loop preamble: check sound toggle, update LED flash, stop any tone | ||
| inline void updateGameInput() { | ||
| checkSoundToggle(); | ||
| updateLEDFlash(); | ||
| if (soundEnabled) gamer.stopTone(); | ||
| } |
There was a problem hiding this comment.
updateGameInput() always calls gamer.stopTone() when sound is enabled. This will immediately cut off any sustained tones, and it breaks Tetris’ background melody logic (tetrisLoop plays a note then relies on it persisting until the next 200ms tick). Consider removing the unconditional stop here, or splitting the helper into two variants / adding a parameter so games like Tetris can opt out of stopping tones each frame.
There was a problem hiding this comment.
Fixed. Added an optional stopTone parameter to updateGameInput() (defaults to true). Tetris now calls updateGameInput(false) to preserve its background melody. Also removed the redundant gamer.stopTone() in Flappy. Commit: d721906
| bool outOfBounds(int xV, int yV) { | ||
| return (xV > 8 || xV < 0 || yV > 8 || yV < 0); | ||
| } | ||
|
|
||
| // Helper: checks if a position is free (LOW) and in bounds | ||
| inline bool isFree(int x, int y) { | ||
| return !outOfBounds(x, y) && gamer.display[x][y] == LOW; | ||
| } |
There was a problem hiding this comment.
outOfBounds() treats x/y == 8 as in-bounds (> 8), but valid indices for an 8×8 grid are 0–7. This can allow gamer.display[8][…] / blocks[8][…] access via helpers like isFree() and new physics calculations. Update the comparisons to use >= 8 (and similarly for y) to avoid out-of-range indexing.
There was a problem hiding this comment.
Fixed. Changed outOfBounds() to use >= 8 (instead of > 8) and reordered the condition in physics() to check bounds before accessing the array. This prevents out-of-range indexing on the 8×8 grid. Commit: a80bbf3
| // Check if we hit something at the next position | ||
| if(gamer.display[nextX][nextY] == HIGH || outOfBounds(nextX, nextY)) { | ||
| // Collision detected! |
There was a problem hiding this comment.
In physics(), gamer.display[nextX][nextY] is evaluated before checking outOfBounds(nextX, nextY). If nextX/nextY are out of range, this reads outside the 8×8 buffer. Reorder the condition to check outOfBounds(...) first (or use isFree()/a safe accessor) before indexing into gamer.display.
There was a problem hiding this comment.
Already fixed in commit a80bbf3. The condition was reordered to check outOfBounds(nextX, nextY) before accessing gamer.display[nextX][nextY], preventing out-of-range reads.
| checkSoundToggle(); | ||
| updateLEDFlash(); | ||
| updateGameInput(); | ||
| if (soundEnabled) gamer.stopTone(); |
There was a problem hiding this comment.
updateGameInput() already stops any tone when soundEnabled is true, so the extra if (soundEnabled) gamer.stopTone(); here is redundant. Removing the duplicate call will keep the loop preamble consistent and slightly reduce per-frame work.
| if (soundEnabled) gamer.stopTone(); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…one in Flappy Co-authored-by: 28pins <262898015+28pins@users.noreply.github.com>
Co-authored-by: 28pins <262898015+28pins@users.noreply.github.com>
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.