Reduce flash size of TetrisAI_V2 by 97% #5308
Conversation
WalkthroughRemoved unused includes, refactored piece-queue logic and public API, added cleared-pixel tracking to grid color state, and adjusted drawGrid pixel-color selection to prioritize cleared pixels and use local pixel variables. Total changes are header cleanup, queue shift refactor, rendering tweak, and added clearedPixels storage/accessor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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)
usermods/TetrisAI_v2/tetrisbag.h (1)
19-19: Self-include should be removed.This header includes itself, which is unnecessary and likely a copy-paste error. The include guards prevent infinite recursion, but the self-include serves no purpose.
Proposed fix
-#include "tetrisbag.h"
🧹 Nitpick comments (1)
usermods/TetrisAI_v2/tetrisbag.h (1)
89-91: Manual left-shift loop is correct; minor edge-case note.The replacement of
std::rotatewith a manual loop is functionally equivalent and achieves the flash size reduction goal. However, ifpiecesQueue.size()were ever 0, the expressionpiecesQueue.size() - 1would underflow (sincesize()returns unsignedsize_t), causing the loop to iterate with out-of-bounds access.In practice,
queueLengthshould never be 0 for a Tetris game, so this is a low-risk edge case. If you want to be defensive:Optional defensive check
void queuePiece() { //move vector to left + if (piecesQueue.empty()) return; for (uint8_t i = 0; i < piecesQueue.size() - 1; i++) { piecesQueue[i] = piecesQueue[i + 1]; } piecesQueue[piecesQueue.size() - 1] = getNextPiece(); }
usermods/TetrisAI_v2/tetrisbag.h
Outdated
| { | ||
| //move vector to left | ||
| std::rotate(piecesQueue.begin(), piecesQueue.begin() + 1, piecesQueue.end()); | ||
| for (uint8_t i = 0; i < piecesQueue.size() - 1; i++) { |
There was a problem hiding this comment.
Safer and technically "correct":
for (uint8_t i = 1; i < piecesQueue.size(); i++) {
piecesQueue[i-1] = piecesQueue[i];
}Will also guard against size of 0.
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)
usermods/TetrisAI_v2/gridcolor.h (1)
81-107: Removingoffset++breaks line-clear synchronization between visual and occupancy grids.With the increment commented out,
offsetstays 0, so pixel rows never shift down. Meanwhile,gridBW.cleanupFullLines()at line 107 still shifts the occupancy bits. This desyncs the color grid from the collision grid. Worse, the UINT8_MAX value set at line 90 is non-zero, so ifsync()runs afterward (which rebuilds gridBW from the color pixels), those marked rows will reappear in gridBW sincesync()checksif (*getPixel(x, y) != 0)at line 123.Restore the
offset++increment to shift color pixels in sync with the occupancy grid, or ensure marked rows are cleared before a later sync pass.Fix: restore offset++ to shift pixels with occupancy
if (gridBW.isLineFull(y)) { - // offset++; + offset++; for (uint8_t x = 0; x < width; x++) { pixels[y * width + x] = UINT8_MAX; }
gustebeast
force-pushed
the
wled-gtb
branch
3 times, most recently
from
c19abe9 to
2578f7d
Compare
Main branch without Tetris Flash: [======== ] 79.8% (used 1255301 bytes from 1572864 bytes) Main branch with Tetris (+196kb) Flash: [========= ] 92.3% (used 1452049 bytes from 1572864 bytes) This commit with Tetris (+6kb, 97% less flash) Flash: [======== ] 80.2% (used 1261625 bytes from 1572864 bytes)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
usermods/TetrisAI_v2/gridcolor.h (2)
19-19: Remove self-include.The file
gridcolor.hincludes itself. This is redundant since the header guards prevent actual issues, but it's clearly unintentional and should be removed.-#include "gridcolor.h"
84-114: Critical bug:offsetis never incremented, breaking row-shift logic.The
offset++on line 92 is commented out, sooffsetremains 0 throughout the loop. This means the conditionif (offset > 0)on line 101 is never true, and the row-shifting logic (lines 105-109) that moves color pixels down after a line is cleared will never execute.While
gridBW.cleanupFullLines()handles the black/white grid, thepixelsarray will become desynchronized—cleared rows won't collapse, causing visual corruption.🐛 Proposed fix
if (gridBW.isLineFull(y)) { - // offset++; + offset++; for (uint8_t x = 0; x < width; x++) { pixels[y * width + x] = 0; clearedPixels[y * width + x] = 1; } continue; }
🤖 Fix all issues with AI agents
In `@usermods/TetrisAI_v2/gridcolor.h`:
- Around line 93-97: The flash state in clearedPixels is never reset so flashed
pixels stay white; to fix, reset the clearedPixels buffer at the start of
cleanupFullLines() (or alternatively decrement/clear entries in drawGrid() after
rendering the flash), e.g., iterate the clearedPixels array and set each element
to 0 before processing line clears so subsequent frames no longer treat those
pixels as flashed; locate the clearedPixels and pixels arrays and the
cleanupFullLines(), drawGrid(), and clear() functions to implement the reset.
| for (uint8_t x = 0; x < width; x++) | ||
| { | ||
| pixels[y * width + x] = 0; | ||
| clearedPixels[y * width + x] = 1; | ||
| } |
There was a problem hiding this comment.
Flash effect persists indefinitely—clearedPixels are never reset.
The clearedPixels are set to 1 when a line is cleared but are only reset to 0 in clear() (called on full reset). After the flash animation frame, these pixels will continue rendering as white forever because there's no code to decrement or clear them.
Consider either:
- Decrementing
clearedPixelsindrawGrid()after rendering the flash, or - Adding a separate method to clear the flash state after a frame, or
- Resetting
clearedPixelsat the start ofcleanupFullLines()before processing.
🐛 Proposed fix (option: reset in cleanupFullLines before processing)
void cleanupFullLines()
{
uint8_t offset = 0;
+ // Reset flash state from previous frame
+ for (uint8_t i = 0; i < width * height; i++)
+ {
+ clearedPixels[i] = 0;
+ }
//from "height - 1" to "0", so from bottom row to top
for (uint8_t y = height; y-- > 0; )
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (uint8_t x = 0; x < width; x++) | |
| { | |
| pixels[y * width + x] = 0; | |
| clearedPixels[y * width + x] = 1; | |
| } | |
| void cleanupFullLines() | |
| { | |
| uint8_t offset = 0; | |
| // Reset flash state from previous frame | |
| for (uint8_t i = 0; i < width * height; i++) | |
| { | |
| clearedPixels[i] = 0; | |
| } | |
| //from "height - 1" to "0", so from bottom row to top | |
| for (uint8_t y = height; y-- > 0; ) | |
| { | |
| // ... rest of the function | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@usermods/TetrisAI_v2/gridcolor.h` around lines 93 - 97, The flash state in
clearedPixels is never reset so flashed pixels stay white; to fix, reset the
clearedPixels buffer at the start of cleanupFullLines() (or alternatively
decrement/clear entries in drawGrid() after rendering the flash), e.g., iterate
the clearedPixels array and set each element to 0 before processing line clears
so subsequent frames no longer treat those pixels as flashed; locate the
clearedPixels and pixels arrays and the cleanupFullLines(), drawGrid(), and
clear() functions to implement the reset.
2 participants
Main branch without Tetris
Flash: [======== ] 79.8% (used 1255301 bytes from 1572864 bytes)
Main branch with Tetris (+196kb)
Flash: [========= ] 92.3% (used 1452049 bytes from 1572864 bytes)
This commit with Tetris (+6kb, 97% less flash)
Flash: [======== ] 80.2% (used 1261625 bytes from 1572864 bytes)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.