Game of Life Rework #4995
Game of Life Rework #4995
Conversation
RAM and speed optimizations. Better repeat detection. Mutation toggle. Blur option added.
WalkthroughReplaces ColorCount with a public Cell bitfield and a contiguous Cell array in the 2D Game of Life; removes CRC-based repetition detection; adds edge/oscillator/spaceship flags; rewrites setup and evolution logic; updates the mode’s metadata string. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
5289-5303: Early break both loops when neighbors > 3 (micro-optim).break exits only the inner loop; the outer i-loop continues. Short‑circuit both loops to save work on dense patterns.
- for (int i = 1; i >= -1; i--) for (int j = 1; j >= -1; j--) if (i || j) { + bool done = false; + for (int i = 1; i >= -1 && !done; i--) for (int j = 1; j >= -1; j--) if (i || j) { int nX = x + j, nY = y + i; if (cell.edgeCell) { nX = (nX + cols) % cols; nY = (nY + rows) % rows; } unsigned nIndex = nX + nY * cols; Cell& neighbor = cells[nIndex]; if (neighbor.alive) { - if (++neighbors > 3) break; + if (++neighbors > 3) { done = true; break; } if (!neighbor.toggleStatus) { // Alive and not dying parentIdx[aliveParents++] = nIndex; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (3)
wled00/FX_fcn.cpp (2)
blur(1071-1097)blur(1071-1071)wled00/fcn_declare.h (3)
hw_random8(433-433)hw_random8(434-434)hw_random8(435-435)wled00/colors.cpp (1)
color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
wled00/FX.cpp (1)
5355-5356: Metadata: surface fits behavior; confirm slider wiring.Blur is mapped from custom1 and mutation from check3 in code; metadata declares "Blur" and "Mutation" accordingly. Looks consistent. Please confirm mobile UI ties custom1/check3 to these controls.
| if (!SEGENV.allocateData(SEGMENT.length() * sizeof(Cell))) return mode_static(); // allocation failed | ||
|
|
||
| Cell *cells = reinterpret_cast<Cell*> (SEGENV.data); | ||
|
|
There was a problem hiding this comment.
Fix allocation size to match iteration domain (cols*rows).
You iterate maxIndex = cols*rows but allocate SEGMENT.length() cells. In some 2D mappings these can diverge. Allocate exactly maxIndex to avoid OOB writes on atypical mappings.
Apply this diff:
- if (!SEGENV.allocateData(SEGMENT.length() * sizeof(Cell))) return mode_static(); // allocation failed
+ if (!SEGENV.allocateData(maxIndex * sizeof(Cell))) return mode_static(); // allocation failed📝 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 (!SEGENV.allocateData(SEGMENT.length() * sizeof(Cell))) return mode_static(); // allocation failed | |
| Cell *cells = reinterpret_cast<Cell*> (SEGENV.data); | |
| if (!SEGENV.allocateData(maxIndex * sizeof(Cell))) return mode_static(); // allocation failed | |
| Cell *cells = reinterpret_cast<Cell*>(SEGENV.data); |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 5209 to 5212, the code allocates SEGMENT.length()
Cell entries but the iteration domain uses maxIndex = cols * rows, which can
differ for 2D mappings and cause out-of-bounds writes; change the allocation to
request maxIndex * sizeof(Cell) (i.e., allocate exactly cols*rows Cells), keep
the existing null-check and fallback to mode_static() on failure, and then cast
SEGENV.data to Cell* as before.
Yes. 2D is built on top of 1D functions. |
Adjust mutation logic. Use 1D get/set. Reduce code size.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (1)
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (3)
wled00/FX_fcn.cpp (2)
blur(1071-1097)blur(1071-1071)wled00/fcn_declare.h (3)
hw_random8(433-433)hw_random8(434-434)hw_random8(435-435)wled00/colors.cpp (1)
color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
| typedef struct Cell { | ||
| uint8_t alive : 1, faded : 1, toggleStatus : 1, edgeCell: 1, oscillatorCheck : 1, spaceshipCheck : 1, unused : 2; | ||
| } Cell; |
There was a problem hiding this comment.
Restore compile-time guard for Cell packing.
We still rely on the bitfield layout being 1 byte, but no longer assert it. Without the static_assert(sizeof(Cell) == 1) we risk miscompilation on other toolchains. Please reintroduce the guard right after the struct definition.
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 5199-5201, the Cell bitfield struct relies on a
1-byte layout but the compile-time check was removed; reintroduce a
static_assert immediately after the struct definition to ensure sizeof(Cell) ==
1 (with a brief message like "Cell must be 1 byte") so builds on other
toolchains fail early if packing changes.
| if (!SEGENV.allocateData(SEGMENT.length() * sizeof(Cell))) return mode_static(); // allocation failed | ||
|
|
There was a problem hiding this comment.
Fix buffer allocation sizing.
We iterate maxIndex = cols * rows, yet still allocate only SEGMENT.length() cells. On non-trivial layouts these differ, leading to OOB writes. Allocate maxIndex * sizeof(Cell) instead.
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 5209-5210, the allocation uses SEGMENT.length()
but the code computes maxIndex = cols * rows and writes up to maxIndex, causing
out-of-bounds writes; change the allocation to allocate maxIndex * sizeof(Cell)
(i.e., allocate space for all cells based on cols*rows) and keep the existing
fallback to mode_static() if allocation fails.
| unsigned a = rows, b = cols; | ||
| while (b) { unsigned t = b; b = a % b; a = t; } | ||
| gliderLength = (cols * rows / a) << 2; | ||
| } |
There was a problem hiding this comment.
Prevent gliderLength overflow.
(cols * rows / a) << 2 is still done in 16-bit space. Larger grids overflow gliderLength. Compute in uint32_t, clamp, then store into the uint16_t.
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 5223 to 5226, the calculation "(cols * rows / a)
<< 2" is performed in 16-bit arithmetic causing overflow for large grids; change
the computation to use a 32-bit unsigned temporary (e.g., uint32_t tmp =
(uint32_t)cols * (uint32_t)rows / a; tmp <<= 2; if (tmp > UINT16_MAX) tmp =
UINT16_MAX;) and then store the clamped result into the uint16_t gliderLength
with an explicit cast.
There was a problem hiding this comment.
Grid sizes would have to be extremely large to overflow. Odds of a glider flying uninterrupted would be insanely low. Glider check is mostly for smaller grids where infinite gliders are more common.
There was a problem hiding this comment.
@Brandon502 Totally fair point — for typical matrices the product/gcd stays small and the glider detection is aimed at those. The only reason I suggested a guard is to avoid silent wrap in the rare case of very large/co-prime dims (e.g. HUB75 chains that creep into 256×N+), where the 32-bit result ends up truncated into the 16-bit aux. That can accidentally disable the spaceship check (gliderLength→0) or make it run at the wrong cadence.
Cost is just a couple instructions, zero behavioral change for small grids. If you’re ok with it, here’s the minimal tweak:
// Calculate glider length LCM(rows,cols)*4 once
unsigned a = rows, b = cols;
while (b) { unsigned t = b; b = a % b; a = t; }
// compute in 32-bit and clamp to uint16_t range
uint32_t lcm = (uint32_t)cols * (uint32_t)rows / (uint32_t)a;
uint32_t tmp = lcm << 2;
if (tmp > UINT16_MAX) tmp = UINT16_MAX;
gliderLength = (uint16_t)tmp;This keeps the small-grid behavior exactly as-is, but prevents corner-case wrap if someone runs very large, relatively prime dimensions.
If you prefer to keep it as-is to save the bytes, I’m fine to drop it. Just wanted to offer the low-risk hardening.
✏️ Learnings added
Learnt from: Brandon502
PR: wled/WLED#4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-15T03:12:37.080Z
Learning: In wled00/FX.cpp Game of Life (mode_2Dgameoflife), spaceship/glider repeat detection is primarily intended for small grids; the maintainer is okay with deprioritizing overflow concerns for very large matrices.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Thanks, swapped to the 1D version and with a few other small changes reduced code size by about 80 bytes. New version now just ~40 bytes larger. |
2 participants
A lightweight version of my MM version to keep code size differences as small as possible (adds ~120 Bytes). Uses 1 Byte per pixel instead of 3. Testing uncapped speeds (esp32dev hub75 build) it ran about 50% faster on 32x32 and 64x32. Improved repeat detection that detects a majority of gliders and oscillators. Not 100% perfect on smaller grids that can produce long repeating patterns due to wrap, I kept mutation as a toggle option for these sizes (8x8 for example). Added blur slider to fade out dying cells.
Previous version had a max update speed of ~10 per second. I increased it to 42 like the old comment suggested.
Videos showing blur:
GoL.64x64.mp4
GoL.Blend.mp4
Patterns that are not detected:
8x8 has oscillators that repeat every 48 and 132 frames. Detection works on patterns <32 for 8x8
24x16 has a spaceship that repeats every 2448 frames. Detection works on patterns <196 for 24x16
Are 2D effects allowed to use 1D version of get/setPixelColor? I didn't include them in this PR but I did test them. They seemed to work fine and reduced code size a bit since in certain places I calculate x and y from an index. For example:
Summary by CodeRabbit