Add flashing effect on line clear in TetrisAI_v2. #5320
Conversation
WalkthroughAdds a staged line-clearing flow: full rows are marked as clearing, renderer flashes marked rows for ~750ms, then lines are flagged ready and removed; grid cleanup and AI rating consult the readiness flag to coordinate marking, flashing, and final removal. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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: 4
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/gridbw.h (1)
142-151:reset()should clear the new clearing state variables.The
reset()method doesn't resetclearingRowsorclearedLinesReadyForRemoval, potentially leaving stale state when the grid is reset (e.g., on game restart or segment reconfiguration).Proposed fix
void reset() { if (width > 32) { width = 32; } pixels.clear(); pixels.resize(height); + clearingRows = 0; + clearedLinesReadyForRemoval = false; }
🤖 Fix all issues with AI agents
In `@usermods/TetrisAI_v2/gridbw.h`:
- Around line 109-117: The shift (1 << row) in the clearing logic can overflow
when row >= 32; update the clearingRows handling to use a wider integer and an
unsigned literal: change the type of clearingRows to uint64_t and replace (1 <<
row) with (1ULL << row), and update any code that reads/writes clearingRows
accordingly (e.g., where clearingRows is declared/checked) so rows ≥ 32 are
represented safely; alternatively, if you must keep uint32_t, add a guard if
(row < 32) before performing the shift to avoid UB.
- Around line 29-34: clearingRows is a uint32_t bitmask limited to 32 rows so
rows >=32 won't flash; replace it with a dynamic bit container (e.g.,
std::vector<bool> clearingRows or std::bitset<255>/boost::dynamic_bitset sized
to the grid height) and update all uses that set/test bits to index into the
vector (same semantics as current bit ops), and ensure
clearedLinesReadyForRemoval behavior remains the same; alternatively, if you
prefer a simpler change, document the 32-row limitation in the comment near
clearingRows and enforce/validate grid height ≤32 where the flash effect is
used.
In `@usermods/TetrisAI_v2/TetrisAI_v2.cpp`:
- Around line 196-210: The cleanup branch that runs when strip.now -
tetrisai_data->clearingStartTime > 750 sets clearedLinesReadyForRemoval and
calls cleanupFullLines() but does not redraw the grid, producing a one-frame
flicker; modify the block handling that timeout (the if branch using
tetrisai_data->tetris.grid.gridBW.clearingRows and clearingStartTime) to call
drawGrid(&tetrisai_data->tetris, tetrisai_data) immediately after
cleanupFullLines() (before resetting clearingStartTime) so the post-cleanup grid
is rendered right away.
- Around line 35-37: The current bit-shift (1 << index_y) on
tetris->grid.gridBW.clearingRows (uint32_t) is UB when index_y >= 32; fix by
preventing out-of-range shifts or widening the bitfield: either (A) add a bounds
check where isRowClearing is computed (e.g., replace with (index_y < 32) &&
((tetris->grid.gridBW.clearingRows & (1u << index_y)) != 0)), or (B) change the
bitmask type across the code (gridBW.clearingRows and all uses in
TetrisAI_v2.cpp and gridbw.h) to a wider/better structure (e.g., uint64_t with
1ULL << index_y or std::bitset<MAX_HEIGHT>) and update the corresponding shifts
to use the matching literal; apply the same coordinated change where
clearingRows is referenced in gridbw.h (and any other occurrences).
🧹 Nitpick comments (1)
usermods/TetrisAI_v2/TetrisAI_v2.cpp (1)
35-46: Flash timing creates asymmetric pattern (150ms on / 50ms off).The condition
(strip.now % 200) < 150produces a 150ms Gray, 50ms Black pattern. If a symmetric 100ms/100ms flash was intended, consider adjusting to(strip.now % 200) < 100. The current asymmetry is subtle but visible.
| if (clearedLinesReadyForRemoval) { | ||
| offset++; | ||
| pixels[row] = 0x0; | ||
| doneRemovingClearedLines = true; | ||
| } else { | ||
| // Set bit at position 'row' to 1 | ||
| clearingRows |= (1 << row); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Undefined behavior: bit shift exceeds 31 for tall grids.
At line 115, 1 << row causes undefined behavior when row >= 32. This is the root cause of the issue flagged in TetrisAI_v2.cpp.
Proposed fix (if keeping uint32_t)
} else {
// Set bit at position 'row' to 1
- clearingRows |= (1 << row);
+ if (row < 32) {
+ clearingRows |= (1UL << row);
+ }
}Note: This fix would silently skip the flash effect for rows ≥ 32. A complete solution requires either limiting grid height or using a larger/dynamic data structure.
📝 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.
| if (clearedLinesReadyForRemoval) { | |
| offset++; | |
| pixels[row] = 0x0; | |
| doneRemovingClearedLines = true; | |
| } else { | |
| // Set bit at position 'row' to 1 | |
| clearingRows |= (1 << row); | |
| } | |
| continue; | |
| if (clearedLinesReadyForRemoval) { | |
| offset++; | |
| pixels[row] = 0x0; | |
| doneRemovingClearedLines = true; | |
| } else { | |
| // Set bit at position 'row' to 1 | |
| if (row < 32) { | |
| clearingRows |= (1UL << row); | |
| } | |
| } | |
| continue; |
🤖 Prompt for AI Agents
In `@usermods/TetrisAI_v2/gridbw.h` around lines 109 - 117, The shift (1 << row)
in the clearing logic can overflow when row >= 32; update the clearingRows
handling to use a wider integer and an unsigned literal: change the type of
clearingRows to uint64_t and replace (1 << row) with (1ULL << row), and update
any code that reads/writes clearingRows accordingly (e.g., where clearingRows is
declared/checked) so rows ≥ 32 are represented safely; alternatively, if you
must keep uint32_t, add a guard if (row < 32) before performing the shift to
avoid UB.
gustebeast
force-pushed
the
wled-gtb-line-clear
branch
from
963b45a to
e182841
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/TetrisAI_v2/gridbw.h`:
- Around line 131-134: The isLineFull(uint8_t y) implementation uses (1 <<
width) which is UB when width==32; change the mask computation in isLineFull to
use unsigned 32-bit operations and handle the full-width case explicitly (e.g.,
compute a uint32_t mask using 1U << width with a ternary or branch so when
width==32 you use 0xFFFFFFFFu, otherwise (1U << width) - 1) and then compare
pixels[y] against that mask; update the isLineFull function to build the mask
with unsigned literals and a safe width==32 path.
| bool isLineFull(uint8_t y) | ||
| { | ||
| return pixels[y] == (uint32_t)((1 << width) - 1); | ||
| } |
There was a problem hiding this comment.
Potential UB when width == 32.
The expression (1 << width) at line 133 causes undefined behavior when width == 32 (which is allowed per the constructor). Use an unsigned literal to avoid the overflow.
Proposed fix
bool isLineFull(uint8_t y)
{
- return pixels[y] == (uint32_t)((1 << width) - 1);
+ return pixels[y] == (uint32_t)((1UL << width) - 1);
}🤖 Prompt for AI Agents
In `@usermods/TetrisAI_v2/gridbw.h` around lines 131 - 134, The isLineFull(uint8_t
y) implementation uses (1 << width) which is UB when width==32; change the mask
computation in isLineFull to use unsigned 32-bit operations and handle the
full-width case explicitly (e.g., compute a uint32_t mask using 1U << width with
a ternary or branch so when width==32 you use 0xFFFFFFFFu, otherwise (1U <<
width) - 1) and then compare pixels[y] against that mask; update the isLineFull
function to build the mask with unsigned literals and a safe width==32 path.
e182841 to
8761132
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@usermods/TetrisAI_v2/gridbw.h`:
- Around line 28-33: GridBW::reset currently doesn't clear the new clearing
state, so clearingRows and clearedLinesReadyForRemoval can persist and keep
hasClearingRows() true after a reset; update GridBW::reset() to clear
clearingRows (e.g., clear() or assign empty vector) and set
clearedLinesReadyForRemoval = false so any in-progress flashing is cancelled on
reset, touching the reset() implementation and ensuring any code that relies on
hasClearingRows() observes the cleared state.
In `@usermods/TetrisAI_v2/TetrisAI_v2.cpp`:
- Around line 195-205: Reset the clearingStartTime when reinitializing the game
state so a new TetrisAIGame doesn't inherit an in-progress flash timer: locate
the re-init/reconstruction code that creates or resets tetrisai_data /
TetrisAIGame and explicitly set tetrisai_data->clearingStartTime = 0 (or
initialize it to 0 in the TetrisAIGame constructor) so the 750ms flash window is
always respected before calling gridBW.clearedLinesReadyForRemoval /
cleanupFullLines().
8761132 to
eee4fa0
Compare
|
Caution gustebeast force-pushed the wled-gtb-line-clear branch from 963b45a to e182841 Compare12 hours ago @gustebeast please stop using force-push ! See https://github.com/wled/WLED?tab=contributing-ov-file#updating-your-code Edit: we don't require a single-commit "squashed" PR - its ok if you just keep adding commits to your PR branch. |
There was a problem hiding this comment.
I would like to see two changes:
- make the flashing effect user-configurable, i.e. add a UM setting for "flashing effect" on/off.
maybe a "photosensitive epilepsy warning" should be added to the usermod readme, too. - make sure that the code works reliably with fixtures that are more than 31 pixels wide or high. As the rabbit pointed out, your current implementation, that uses the bits of a
uint32_t, will not work with a 48x56 matrix. Especially HUB75 panels can be 64x64 or even bigger.
For point 2: if you want to keep the bits in an uint32_t, imho its OK if you only support flashing in the lowest 32 rows.
2 participants
See https://imgur.com/1dsNCVd for a demo.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
✏️ Tip: You can customize this high-level summary in your review settings.