Intermediate color updates while moving around the color wheel#4297
Intermediate color updates while moving around the color wheel#4297WouterGritter wants to merge 2 commits intowled:mainfrom
Conversation
|
thanks for contributing. While I like the idea, the implementation is, as you found yourself, flawed. |
|
Also we've got #4158 in the pipeline for 0.16, which adds a whole bunch of neat new transition modes that definitely won't play nice with rapid FX updates. We could probably figure out some solution in the long term, eg. stacked transitions are permitted if you've got the RAM for it, but there's definitely additional work required. I'm flagging this PR as keep for now; I think it's a nice feature that's worth revisiting when the backend can be improved to support it well. |
|
... now imagine having transition time of 10s. 😉 Also, ESP8266, even with all the bells and whistles @willmmiles added to AWS, this rapid bombardment may bring it down. |
|
Okay, might be worth it to have this be a feature that's off by default then, with warnings about sending too much updates to services like AWS in the settings menu. However, this is similar to just tapping the color wheel once or twice per second manually, which ideally should not be a huge problem as this could be done by a user trying to fine-tune the perfect color. It could be worth adding support for an "intermediate update" in the C code. This type of update would ONLY modify the GPIO pins, and would not make any changes to external services (like mqtt, aws, even udp broadcasting). Because it always expects a final "proper" update when the user has released their thumb off of the color wheel. If we want to go that far, we can implement this system using websockets to allow for even quicker updates without much overhead from separate HTTP requests. |
|
@coderabbitai please review. Create these summary lists in addition to your review:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2351 🏁 Script executed: Length of output: 2082 🏁 Script executed: Length of output: 3512
Code Review — PR
|
| Priority | Item |
|---|---|
| P0 | Fix startTransition() stale-clock bug in wled00/FX_fcn.cpp (C-side) |
| P0 | Override transition: 0 in intermediate updates so they don't fight with ongoing blends |
| P1 | Add a user preference (e.g., in the UI settings panel) to enable/disable live drag updates; default off is safer for users with cloud integrations or long transitions |
| P1 | Replace new Date() with Date.now() — avoids allocating a Date object on every color:change event (which fires on every pixel of drag movement) |
| P2 | Reset lastIntermediateColorUpdate inside the input:end handler to avoid the edge case where color:change fires within the same tick as the final input:end send, potentially triggering two back-to-back requests |
| P2 | Consolidate the two color:change listeners into one to reduce event-handler overhead and improve readability: cpick.on("color:change", () => { updatePSliders(); updateIntermediateColor(); }); |
| P3 | Skip the intermediate send entirely when the WebSocket is not open (i.e., fall back only on HTTP for the final input:end commit), to avoid HTTP-flooding slow devices during drag |
| P3 | Make the throttle interval configurable or at least document why 500 ms was chosen relative to a typical transition duration |
Minimal Code Fix (JS side only)
// Replace the added global var:
var lastIntermediateColorUpdate = 0; // use timestamp number, not Date object
// Replace the two color:change listeners with one:
cpick.on("color:change", () => { updatePSliders(); updateIntermediateColor(); });
// Keep input:end but also reset the throttle timer:
cpick.on("input:end", () => { lastIntermediateColorUpdate = 0; setColor(1); });
// Replace the new function:
function updateIntermediateColor() {
const now = Date.now();
if (now - lastIntermediateColorUpdate > 500) {
lastIntermediateColorUpdate = now;
// Force transition:0 for intermediate updates to avoid fighting ongoing transitions
var cd = gId('csl').children;
let cdd = cd[csel].dataset;
let w = hasWhite ? parseInt(gId('sliderW').value) : 0;
var col = cpick.color.rgb;
let r = hasRGB ? col.r : w, g = hasRGB ? col.g : w, b = hasRGB ? col.b : w;
var obj = {"seg": {"col": [[],[],[]]}, "transition": 0};
obj.seg.col[csel] = [r, g, b, w];
requestJson(obj);
}
}The C-side fix to startTransition() in wled00/FX_fcn.cpp is a separate and required companion change.
🧠 Learnings used
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: 5379
File: wled00/FX.cpp:10836-10846
Timestamp: 2026-03-27T18:44:44.652Z
Learning: WLED Slow Transition FX (mode_slow_transition): The primary target is long transitions (>1 minute). Minor sweep non-uniformity during the 10 s test path (speed=0) is acceptable and not a blocker for this feature.
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: 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: 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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range 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: 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: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T11:41:46.737Z
Learning: In the WLED repository (wled/WLED), trailing-whitespace removals or other pure-whitespace "optimizations" must NOT be intermixed with real code/logic changes in the same PR. Reviewers prefer whitespace cleanup to be submitted in a dedicated, separate PR. Reason: intermixing makes diffs harder to read (obscures actual logic changes) and causes trouble when cherry-picking commits. Always flag PRs that mix whitespace-only edits with functional changes and ask the contributor to separate them.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-12T07:15:56.657Z
Learning: Document changes in pull requests with clear description of what changed and why, including expected impact on user-visible behavior, related issue links, and screenshots for new features
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-22T20:40:46.281Z
Learning: At low brightness (<30), color_blend() suffers from integer rounding that destroys RGB channel ratios, causing visible hue shifts during transitions (e.g., greenish tints). This can be fixed by applying hue preservation logic similar to color_fade()'s video mode: add 1 to channels that are non-zero and > 13% of max channel `(channel << 5) > maxc`. This preserves color ratios and prevents premature channel zeroing at low brightness without affecting performance at normal brightness levels.
Learnt from: softhack007
Repo: wled/WLED PR: 5467
File: usermods/audioreactive/audio_reactive.cpp:2316-2316
Timestamp: 2026-04-09T23:10:03.947Z
Learning: In WLED's AudioReactive usermod (`usermods/audioreactive/audio_reactive.cpp`), `fillAudioPalettes()` is called at variable intervals (0.3ms to 100ms, with peaks up to 1000ms) due to LED count and loop/file-access delays. The `paletteBandAvg[]` IIR EMA filter must NOT use a fixed alpha; instead, compute a time-adaptive alpha per call: `alpha = 1.0f - expf(-(float)dt_ms / PALETTE_TIME_CONSTANT_MS)` where `PALETTE_TIME_CONSTANT_MS ≈ 390.0f` (derived from original α=0.05 at 20ms → τ = -20/ln(0.95) ≈ 390ms). Track `lastPaletteTime` with `millis()` and cap `dt_ms` at ~2000ms to prevent a single catch-up jump after long pauses (OTA, file I/O).
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: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-15T20:25:25.815Z
Learning: The CSS palette preview in wled00/data/index.js genPalPrevCss() function uses raw RGB values in CSS linear-gradient() without applying WLED's gamma correction, while actual LED output goes through NeoGammaWLEDMethod gamma correction. This causes inherent discrepancies between the web UI palette preview and actual LED colors, especially noticeable with different gamma settings.
I think this is a dead end - increasing the number of WebUI JSON messages to the wled core is not a good idea, and 2 color updates per second are definitely not "smooth" . Also the PR is stale since almost two years ==> closing. |
Push color changes when moving your finger/mouse around the color wheel every 500 ms, instead of only when the mouse/finger gets released from the color wheel.
This real-time color changing is similar to how Philips Hue does it in their app, and it makes it feel really nice and responsive.
The implementation almost seems TOO easy, it makes me think someone has tried this before and it did not get merged for some reason. Would love to hear your feedback.
One problem I'm seeing currently is that the C code does not handle it nicely when it gets a new color while it is transition from another color. The transition is smooth for the first update, but for the second update it jumps to the second color. Then for the next update it's smooth again, and it jumps again. Could be fixed by increasing the interval that the JS code sends updates. However it might be better to update the C code.