Add flashing effect on line clear in TetrisAI_v2.#5320
Add flashing effect on line clear in TetrisAI_v2.#5320gustebeast wants to merge 3 commits intowled:mainfrom
Conversation
WalkthroughImplements staged line-clearing: full rows are marked as clearing and flashed (flash can be disabled), a clearing timer is tracked per game, after ~750ms lines are marked ready and removed; AI scoring and cleanup use the "ready for removal" signal; usermod exposes and persists the noFlashOnClear setting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧪 Generate unit tests (beta)
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.
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.
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.
|
Flashing effect is now configurable. When disabled, the line will turn white (technically gray) on clear but won't flash on and off. Readme has been updated to highlight the option for those with photosensitivity. uint32_t issue has been addressed by replacing with a vec, matching existing implementation's maximum allowed height. For the width the game already enforces a 32 width maximum with checks in place to ensure it this isn't exceeded. |
|
@gustebeast there is still a warning from the rabbit about bit-shift exceeding 32bit in isLineFull(). |
|
@coderabbitai please re-review this PR, and summarize your remaining findings. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23562
✅ Successfully Addressed
|
gustebeast
force-pushed
the
wled-gtb-line-clear
branch
from
2548bce to
4f14296
Compare
|
Apologies for the force push, I did make sure to keep the change on a new commit though. I hadn't included the fix before since it's an existing issue with the code, but I'm happy to include the fix in this PR. |
Please never do that again. I'm not sure what happened, but we lost at least one comment in a completely unrelated issue ticket. Are you saying that force-push was absolutely necessary just for a single line change? There are other non-destructive methods to update your branch. I can recommend that you look into "git cherry-pick" (copy commit from one branch to another) or "update from main"/ "merge into current branch" / "rebase current branch" features of github desktop. Unless the PR page (bottom part) explicitly says "There are merge conflicts that must be resolved", its not even necessary to keep your PR branch in sync with the target branch. Edit: just to be clear - on next force-push, i'll simply close this PR to prevent further damage, and ask for a new PR. |
@gustebeast I found this in the "minor" section of coderabbit suggestions. Not sure if this is relavant and correct, I'll leave it to your judgement if you want to address this proposal. |
|
Understood on the force push. I was using it for commits that I hadn't made PRs for yet and accidentally carried the habit back over to this PR. Won't do it again, and will look into avoiding it on my personal commits as well. Just need a way to squash my changes into a single commit for local development so my initial PR is a single commit. Having additional commits for changes post PR makes sense. As for the reset comment, the fix for that was squashed into the first commit (another reason not to force push) |
2 participants
See https://imgur.com/1dsNCVd for a demo.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Documentation