Adding overlay support for Strobe and Blink Effects#5508
Closed
mic159 wants to merge 2 commits into
Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Collaborator
|
overlay is no longer needed in 16.0 it has segment layering making this PR obsolete. |
Member
@DedeHai you're right about the principle- segment layering / blending should be able to achieve the same. I've checked, and we still have like ~20 effects with an "Overlay" option - 90% of them to remove non-black background, like a background palette or user-defined background color. In a very few cases, setPixelColor is replaced by blending (unnecessary indeed), or it disables a "blur" or "fade-out" step. To free one custom checkbox, we could actually implement a new segment level flag (like |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
wled00/FX.cpp (2)
1301-1311:⚠️ Potential issue | 🟠 MajorKeep the fireworks decay path active in overlay mode.
Line 1301 and Line 1311 now skip
fade_out()andblur()whenSEGMENT.check2is set, but those calls are the effect’s own decay/state progression, not just background painting. In overlay mode this causes sparks to stop dissipating, somode_fireworks()— and any wrapper that reuses it — will accumulate stale pixels instead of animating cleanly.Suggested fix
- if (!SEGMENT.check2) SEGMENT.fade_out(128); + SEGMENT.fade_out(128); @@ - if (!SEGMENT.check2) SEGMENT.blur(16); // used in mode_rain() + SEGMENT.blur(16); // used in mode_rain()Based on learnings: post-commit ee9ac94 the rendering pipeline uses per-segment buffers and per-pixel bus updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 1301 - 1311, The decay calls are being skipped when SEGMENT.check2 is set which prevents sparks from dissipating in overlay/wrapper use; always perform the effect's own decay/state progression by invoking SEGMENT.fade_out(...) and SEGMENT.blur(...) regardless of SEGMENT.check2 so mode_fireworks (and callers that reuse it) continue to decay; modify the block around SEGENV.step (referencing SEGMENT.check2, SEGMENT.fade_out, SEGMENT.blur, mode_fireworks, and SEGENV.step) to separate background painting gating from the effect decay — keep painting conditional on check2 but unconditionally run the fade_out and blur calls.
1337-1360:⚠️ Potential issue | 🟠 MajorDon’t disable the rain shift/update step for overlay.
Line 1337 makes the core move/shift block conditional on
!SEGMENT.check2. That removes the falling motion entirely, so overlay “Rain” degrades into stationary spark spawning rather than rain. If overlay should only avoid repainting the background, the state update still needs to run.Suggested fix
- if (!SEGMENT.check2 && SEGENV.call && SEGENV.step > SPEED_FORMULA_L) { + if (SEGENV.call && SEGENV.step > SPEED_FORMULA_L) {Based on learnings: post-commit ee9ac94 the rendering pipeline uses per-segment buffers and per-pixel bus updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 1337 - 1360, The motion/shift and spark index updates must run regardless of SEGMENT.check2 so rain physics continue even when overlay-only rendering is active; change the logic in the block guarded by "!SEGMENT.check2 && SEGENV.call && SEGENV.step > SPEED_FORMULA_L" to always perform the state updates (the 2D SEGMENT.move(...) branch and the 1D shift using SEGMENT.getPixelColor / setPixelColor plus SEGENV.aux0/aux1 increments), and only skip the actual background repainting when SEGMENT.check2 is true; also fix the typo that resets the wrong aux on zero (the line "if (SEGENV.aux1 == 0) SEGENV.aux0 = UINT16_MAX;" should reset SEGENV.aux1), and keep the existing bounds checks (SEGENV.aux0/aux1 >= width*height) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX.cpp`:
- Around line 4137-4142: The overlay blending is reading from the segment's own
buffer (SEGMENT.getPixelColor(i)) instead of the already-rendered lower layer;
in the overlay branch (the path controlled by SEGMENT.check2) change the source
pixel read to the global/strip buffer (use strip.getPixelColor with the correct
absolute index, e.g. strip.getPixelColor(i + SEGMENT.start)) so color_blend(...)
mixes the overlay SEGCOLOR(1) with the underlying rendered pixel rather than the
segment's stale self-pixel; keep the other branch (palette path) as-is.
---
Outside diff comments:
In `@wled00/FX.cpp`:
- Around line 1301-1311: The decay calls are being skipped when SEGMENT.check2
is set which prevents sparks from dissipating in overlay/wrapper use; always
perform the effect's own decay/state progression by invoking
SEGMENT.fade_out(...) and SEGMENT.blur(...) regardless of SEGMENT.check2 so
mode_fireworks (and callers that reuse it) continue to decay; modify the block
around SEGENV.step (referencing SEGMENT.check2, SEGMENT.fade_out, SEGMENT.blur,
mode_fireworks, and SEGENV.step) to separate background painting gating from the
effect decay — keep painting conditional on check2 but unconditionally run the
fade_out and blur calls.
- Around line 1337-1360: The motion/shift and spark index updates must run
regardless of SEGMENT.check2 so rain physics continue even when overlay-only
rendering is active; change the logic in the block guarded by "!SEGMENT.check2
&& SEGENV.call && SEGENV.step > SPEED_FORMULA_L" to always perform the state
updates (the 2D SEGMENT.move(...) branch and the 1D shift using
SEGMENT.getPixelColor / setPixelColor plus SEGENV.aux0/aux1 increments), and
only skip the actual background repainting when SEGMENT.check2 is true; also fix
the typo that resets the wrong aux on zero (the line "if (SEGENV.aux1 == 0)
SEGENV.aux0 = UINT16_MAX;" should reset SEGENV.aux1), and keep the existing
bounds checks (SEGENV.aux0/aux1 >= width*height) as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6339af31-9660-4871-a987-265116028be3
📒 Files selected for processing (1)
wled00/FX.cpp
Comment on lines
+4137
to
+4142
| uint8_t blendAmount = uint8_t(255 - (SEGENV.aux1 >> 8)); | ||
| if (SEGMENT.check2) { | ||
| SEGMENT.setPixelColor(i, color_blend(SEGMENT.getPixelColor(i), SEGCOLOR(1), blendAmount)); | ||
| } else { | ||
| SEGMENT.setPixelColor(i, color_blend(SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0), SEGCOLOR(1), blendAmount)); | ||
| } |
Contributor
There was a problem hiding this comment.
Heartbeat overlay is blending with the wrong source buffer.
Line 4139 blends against SEGMENT.getPixelColor(i), which is this segment’s own buffer, not the already-rendered lower layer the PR is trying to stack over. That means the overlay path will blend with stale/self pixels instead of the effect underneath. This file already shows the distinction in mode_copy_segment(), where global-frame reads use strip.getPixelColor... and segment-buffer reads use getPixelColor....
Based on learnings: post-commit ee9ac94 the rendering pipeline uses per-segment buffers and per-pixel bus updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 4137 - 4142, The overlay blending is reading from
the segment's own buffer (SEGMENT.getPixelColor(i)) instead of the
already-rendered lower layer; in the overlay branch (the path controlled by
SEGMENT.check2) change the source pixel read to the global/strip buffer (use
strip.getPixelColor with the correct absolute index, e.g. strip.getPixelColor(i
+ SEGMENT.start)) so color_blend(...) mixes the overlay SEGCOLOR(1) with the
underlying rendered pixel rather than the segment's stale self-pixel; keep the
other branch (palette path) as-is.
Member
There was a problem hiding this comment.
The effect uses SEGMENT.getPixelColor(i) which is the correct way to retrieve the last pixel(i) painted by the effect.
It is indeed unclear what is the intention here - "blending" with a lower layer pixel, or just to merge a background color into the pixel being painted.
Contributor
There was a problem hiding this comment.
@softhack007 Thanks — agreed that SEGMENT.getPixelColor(i) is the segment’s current-frame buffer, so it’s correct if the goal is “merge into this segment’s background.”
Because the PR description says “allow these effects to be layered on top of other effects,” we should pick one of these two exact behaviors for Heartbeat’s Overlay path (Line 4139):
A) True overlay over lower layers (preferred for stacking semantics)
- Don’t sample anything; just paint the pulse with alpha and let segment layering/compositor handle the stacking.
- Minimal change:
// Line 4137–4142
uint8_t blendAmount = uint8_t(255 - (SEGENV.aux1 >> 8));
if (SEGMENT.check2) {
// Overlay: draw pulse with alpha; compositor stacks this segment above lower layers
SEGMENT.blendPixelColor(i, SEGCOLOR(1), blendAmount);
} else {
SEGMENT.setPixelColor(i, color_blend(
SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0),
SEGCOLOR(1), blendAmount));
}B) Tint what’s already “underneath” (visual mix with the layer below)
- Read the underlay from the global frame. Note this path uses the last rendered frame (1-frame latency), consistent with the comment used in mode_copy_segment().
- Change only the overlay branch:
uint8_t blendAmount = uint8_t(255 - (SEGENV.aux1 >> 8));
if (SEGMENT.check2) {
// “Underlay tint” variant (reads global/last frame)
uint32_t under = strip.getPixelColorNoMap(i + SEGMENT.start);
SEGMENT.setPixelColor(i, color_blend(under, SEGCOLOR(1), blendAmount));
} else {
SEGMENT.setPixelColor(i, color_blend(
SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0),
SEGCOLOR(1), blendAmount));
}Given the rest of this PR generally avoids clearing the background in overlay mode (instead of sampling), I lean toward option A for consistency and to avoid the last-frame read. If you prefer the “tint underlay” effect, I’ll switch to option B and note the 1‑frame lag in the commit message.
Which behavior would you like for Heartbeat’s Overlay?
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FX.cpp:10824-10824
Timestamp: 2026-03-29T06:08:02.547Z
Learning: WLED: In wled00/FX.cpp::mode_slow_transition(), the change-detection logic intentionally compares data->currentCCT to SEGMENT.cct (not data->endCCT). SEGMENT.cct is set to currentCCT at the end of each call; comparing to endCCT would re-initialize the transition on each frame and stall CCT blending. Do not propose changing this.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:09.740Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/FX_fcn.cpp:1380-1399
Timestamp: 2026-03-21T17:47:53.826Z
Learning: In WLED's stencil blend mode (case 16 in `WS2812FX::blendSegment()`, `wled00/FX_fcn.cpp`), the transparency key is intentionally hardcoded to black (`t ? t : b`), not the segment's background color (`colors[1]`). This is a deliberate design choice to avoid additional parameters and overhead. It is the user's responsibility to not use a non-black background in combination with stencil mode. Do not flag this as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:09.816Z
Learning: In WLED’s WLED code, if a pixel/buffer allocation uses `BFRALLOC_NOBYTEACCESS` (and especially on classic ESP32 where byte-level access to IRAM-resident buffers is unsafe), avoid using byte-wise operations like `memset`/`memcpy` on that buffer. Specifically, do not combine `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` and do not perform `memcpy`/`memset` over `Segment::pixels` (e.g., in `setGeometry()`/`finalizeInit()` or copy ctor/assignment). Instead, use element-wise 32-bit access (loop over `uint32_t*` and assign/copy per element) to ensure 32-bit access only.
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:13:21.670Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1307-1327
Timestamp: 2026-03-24T07:59:28.661Z
Learning: In WLED's Dissolve Plus effect (usermods/user_fx/user_fx.cpp, mode_dissolveplus), when the "Last one" checkbox (check3/lastOneMode) is turned off while the effect is in PHASE_PAUSE_SURVIVOR, the phase is intentionally NOT immediately reset. The pause expires naturally via the normal delay timing (a few hundred ticks), after which the filling phase begins. PHASE_DISSOLVE_SURVIVOR is reset to PHASE_DISSOLVE and PHASE_FILL_SURVIVOR is reset to PHASE_FILL on toggle-off, but PHASE_PAUSE_SURVIVOR is left to expire on its own. Do not flag this as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 5379
File: wled00/FX.cpp:10865-10865
Timestamp: 2026-03-27T18:33:33.813Z
Learning: Repo: wled/WLED
File: wled00/FX.cpp
Context: Slow Transition effect (mode_slow_transition) UI metadata string _data_FX_MODE_SLOW_TRANSITION
Learning: The “Time (min)” control intentionally does not advertise the special case “0 = 10s” in the label; this shortcut is meant to be discovered by users. Do not suggest adding “0=10s” (or similar) to the label in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:29.218Z
Learning: In WLED, when reviewing ESP32 code that uses pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (e.g., `WS2812FX::_pixels` and `Segment::pixels`), treat `memset`/`memcpy` on those buffers as an intentionally low-risk, accepted pattern. These buffers are designed to be IRAM-safe for 32-bit-only access, and the Xtensa newlib assembly implementations perform 32-bit word stores for word-aligned, word-multiple-sized buffers (so the IRAM constraint is satisfied in practice). Do not flag `memset`/`memcpy` on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug; if any concern arises, downgrade severity to low and focus on potential future regressions rather than an immediate correctness violation.
Member
There was a problem hiding this comment.
@coderabbitai you seem to use wrong assumptions, let me correct a few things:
- effects should always use SEGMENT level pixel functions, the "copy segment" effects family (post-processing of other effects) is an exception to the rule
- WLED does not (yet) support RGBA (with alpha). We use RGBW where W stands for "white LED"
- effect blending already happens in WS2812FX::blendSegment().
- to be usable as overlay, effects just need to skip painting pixels that should be kept from the lower layers, or paint them BLACK and use an appropriate blending mode, or use other blending modes to mix colors.
See also #5508 (comment)
Contributor
There was a problem hiding this comment.
@softhack007 Thanks for the clarifications — I stand corrected.
- Agreed: effects should use SEGMENT.* read/write (copy_* family is the exception).
- No alpha channel; final layer compositing is handled centrally in WS2812FX::blendSegment().
- For overlay behavior, we should not sample any “underlay.” Instead, only draw the pulse and otherwise leave pixels untouched or clear to BLACK as needed.
Concretely for Heartbeat, a minimal overlay path that respects this looks like:
// inside mode_heartbeat()
uint8_t blendAmount = uint8_t(255 - (SEGENV.aux1 >> 8));
if (SEGMENT.check2) { // Overlay: draw pulse only, no background
if (blendAmount) {
// scale the pulse color by the envelope
SEGMENT.setPixelColor(i, color_blend(BLACK, SEGCOLOR(1), blendAmount));
} else {
// ensure we don't leave stale self-pixels around between beats
SEGMENT.setPixelColor(i, BLACK);
}
} else {
// original non-overlay path (build background from palette, then tint with pulse)
SEGMENT.setPixelColor(i, color_blend(
SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0),
SEGCOLOR(1), blendAmount));
}Why this aligns with your guidance:
- Only SEGMENT.* APIs are used.
- No attempt to read a lower layer or use alpha — we simply emit the pulse color (or clear) in overlay mode.
- The composer/blendSegment() then stacks this segment over the lower one according to the selected blend mode. If a mode that keys black-as-transparent is chosen, the off-beat BLACK keeps the underlay visible; if not, at least the overlay path isn’t repainting a background.
If you prefer keeping last-frame self-pixel decay instead of explicit BLACK on the “off” phase, we can switch the off branch to “do nothing” — I chose BLACK to avoid ghosting and keep frame-to-frame state deterministic.
I can update Heartbeat accordingly if that approach works for you.
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5508
File: wled00/FX.cpp:4137-4142
Timestamp: 2026-04-16T09:49:58.587Z
Learning: WLED overlay behavior: Effects must operate via SEGMENT.* APIs (reads/writes) — global frame reads (strip.getPixelColor*) are reserved for copy_* style post‑processing. Blending between segments is performed later in WS2812FX::blendSegment(); effects should not try to composite with lower layers themselves.
Learnt from: softhack007
Repo: wled/WLED PR: 5508
File: wled00/FX.cpp:4137-4142
Timestamp: 2026-04-16T09:49:58.588Z
Learning: WLED uses RGBW (no alpha). To make an effect usable as an overlay, either skip painting pixels you want to preserve from lower layers or write BLACK and rely on the selected segment blend mode to treat black appropriately (e.g., stencil). Avoid sampling “underlay” pixels within the effect.
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.
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FX.cpp:10824-10824
Timestamp: 2026-03-29T06:08:02.547Z
Learning: WLED: In wled00/FX.cpp::mode_slow_transition(), the change-detection logic intentionally compares data->currentCCT to SEGMENT.cct (not data->endCCT). SEGMENT.cct is set to currentCCT at the end of each call; comparing to endCCT would re-initialize the transition on each frame and stall CCT blending. Do not propose changing this.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:09.740Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/FX_fcn.cpp:1380-1399
Timestamp: 2026-03-21T17:47:53.826Z
Learning: In WLED's stencil blend mode (case 16 in `WS2812FX::blendSegment()`, `wled00/FX_fcn.cpp`), the transparency key is intentionally hardcoded to black (`t ? t : b`), not the segment's background color (`colors[1]`). This is a deliberate design choice to avoid additional parameters and overhead. It is the user's responsibility to not use a non-black background in combination with stencil mode. Do not flag this as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:09.816Z
Learning: In WLED’s WLED code, if a pixel/buffer allocation uses `BFRALLOC_NOBYTEACCESS` (and especially on classic ESP32 where byte-level access to IRAM-resident buffers is unsafe), avoid using byte-wise operations like `memset`/`memcpy` on that buffer. Specifically, do not combine `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` and do not perform `memcpy`/`memset` over `Segment::pixels` (e.g., in `setGeometry()`/`finalizeInit()` or copy ctor/assignment). Instead, use element-wise 32-bit access (loop over `uint32_t*` and assign/copy per element) to ensure 32-bit access only.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:29.218Z
Learning: In WLED, when reviewing ESP32 code that uses pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (e.g., `WS2812FX::_pixels` and `Segment::pixels`), treat `memset`/`memcpy` on those buffers as an intentionally low-risk, accepted pattern. These buffers are designed to be IRAM-safe for 32-bit-only access, and the Xtensa newlib assembly implementations perform 32-bit word stores for word-aligned, word-multiple-sized buffers (so the IRAM constraint is satisfied in practice). Do not flag `memset`/`memcpy` on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug; if any concern arises, downgrade severity to low and focus on potential future regressions rather than an immediate correctness violation.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:13:21.670Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FXparticleSystem.cpp:598-606
Timestamp: 2026-03-09T19:16:30.985Z
Learning: In WLED's FXparticleSystem.cpp, the particle system (both 1D and 2D) is intentionally RGB-only and does not process or output the white channel. The `fast_color_scaleAdd()` function discards the W byte by design. Even though `CRGBW` is used internally for color representation (e.g., `baseRGB`, palette lookups via `ColorFromPalette`), the W channel is never rendered. Do not flag the missing W channel handling in particle system rendering paths as a bug.
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1307-1327
Timestamp: 2026-03-24T07:59:28.661Z
Learning: In WLED's Dissolve Plus effect (usermods/user_fx/user_fx.cpp, mode_dissolveplus), when the "Last one" checkbox (check3/lastOneMode) is turned off while the effect is in PHASE_PAUSE_SURVIVOR, the phase is intentionally NOT immediately reset. The pause expires naturally via the normal delay timing (a few hundred ticks), after which the filling phase begins. PHASE_DISSOLVE_SURVIVOR is reset to PHASE_DISSOLVE and PHASE_FILL_SURVIVOR is reset to PHASE_FILL on toggle-off, but PHASE_PAUSE_SURVIVOR is left to expire on its own. Do not flag this as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 5379
File: wled00/FX.cpp:10865-10865
Timestamp: 2026-03-27T18:33:33.813Z
Learning: Repo: wled/WLED
File: wled00/FX.cpp
Context: Slow Transition effect (mode_slow_transition) UI metadata string _data_FX_MODE_SLOW_TRANSITION
Learning: The “Time (min)” control intentionally does not advertise the special case “0 = 10s” in the label; this shortcut is meant to be discovered by users. Do not suggest adding “0=10s” (or similar) to the label in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
Member
There was a problem hiding this comment.
@mic159 as you are the contributor:
- what was your intention with this part of the code?
- does the last suggestion from the rabbit make sense for you?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
softhack007
added
the
waiting for feedback
Author
|
I was after layering effects that could be synced to BPM ontop of a base effect. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull request is to add Overlay support to a few more effects where it might be useful:
This allows you to stack them ontop of other effects.
Summary by CodeRabbit
Release Notes
New Features
Documentation