Animartrix plus: HUE shift, contrast & brightness boost#344
Animartrix plus: HUE shift, contrast & brightness boost#344softhack007 wants to merge 28 commits intomdevfrom
Conversation
thanks @DedeHai, works perfectly
* convert pixel color from RGBW32 to CHSV32, * shift HUE value, * convert back to RGBW32 HUE shift is either static (slider), or continuous based on elapsed time.
* slider for HUE shift (static shift, or dynamic speed of change) * checkbox for HUE cycling (dynamic) * checkboxes for brightness and contrast boost * remove checkbox for gamma correction, need to move this into usermod global settings.
📝 WalkthroughWalkthroughAdds audio-reactive features and an AudioMon mode, expands color processing (gamma/contrast/brightness, 16-bit hue via CHSV32), introduces color utilities header, global audio/gamma state and mapping helpers, and exposes config/UI hooks for gamma and audio detector settings. Changes
Sequence Diagram(s)sequenceDiagram
participant AudioDetector as Audio Detector
participant Animartrix as ANIMartRIXMod
participant ColorUtils as Color Utils (CHSV32/gamma)
participant LEDStrip as LED Strip
AudioDetector->>Animartrix: provide sample / detector reading
Animartrix->>Animartrix: getAudioDataOrSim() / processAudio()
Animartrix->>Animartrix: handleAudioHUE() -> compute hueshift / audioHUE
Animartrix->>ColorUtils: processColor(rgb) (apply hue shift, contrast, brightness, gamma)
ColorUtils-->>Animartrix: processed RGB
Animartrix->>LEDStrip: write pixel data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
With strip.now in milliseconds, the fast-path overflows after ~7.5 hours and the slow-path after ~54.2 hours, causing visible hue jumps in continuous uptime scenarios. => Cast the multiplication operands to uint64_t before division.
avoid losing accuracy in hue shift calculation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
explains rationale behind enhanceBrightness fixed-point math using integer sqrt()
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AnimarTrix effects were designed with hardware-level gamma correction in mind => change default to apply gamma. Note: this may create an unexpected / different look for older presets. If old behaviour is needed, disable gamma correction in UM settings (option will be added soon)
improves accuracy by embedding "<<4" into the main multiplication
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Ideas and ToDOs
oops, forgot half of the code.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
audio extraction and filtering
gravimeter-like 1D effect for audio debugging
|
|
||
| if ((deltaSample > 0.0f) || (decayMS < 1)) { | ||
| // fast attack/decay with minimal filtering | ||
| lastAudioData[detectorID] += 0.9f * audioSmooth * deltaSample; // 0.9 for damping of jumps |
There was a problem hiding this comment.
tinkering help: adjust "0.9" if you still find effects too "flash-like" (smaller = smoother)
| audioSample = 255.0f * (logf(audioSample) - M_LOG_3) / M_LOG_256; // log scaling - amplifies lower frequencies, reduces noise | ||
| else audioSample = 0.0f; | ||
| break; | ||
| case DET_BASS: |
There was a problem hiding this comment.
tinkering help: the FFT based ones below looked sufficiently "distinct" in my own tests. comments/feedback welcome 😀
| //======================================================================================================================== | ||
|
|
||
| #define ANIMARTRIX_UI_MONITOR "Speed(nothing),HUE Change(nothing),Audio Strength,Audio Decay,Audio Detector ID,Amplify,Amplify,;!;1v;c1=255,c2=64,c3=0,o1=0,o2=0,m12=7,si=0" | ||
| static const char _data_FX_mode_AudioMon1D[] PROGMEM = "Y💡AudioMonitor 1D ☾@" ANIMARTRIX_UI_MONITOR; |
There was a problem hiding this comment.
tinkering help: this is your friend if you want to see the audio-based hue shift as a bar.
not needed, as AR core already does some filtering
correct missing filter 4 add input validation
correct metadata errors, change default for c2 from 64 to 32
"anim" is one global object - create separate object for Monitor, to prevent interferences between effects wtih different filter options
* use different state object when running monitor with detectorID == 0 * monitor takes over main effect parameters when detectorID == 0 * minor tinkering * adding rounding in monitor effect drawing routine
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds extended color processing controls to Animartrix effects (static/dynamic HUE shift, brightness/contrast boost, audio-reactive HUE) and moves gamma correction to a global usermod setting.
Changes:
- Introduces
CHSV32+ fast HSV/RGB conversion helpers for higher-precision HUE operations. - Extends Animartrix effect UI controls (cycle HUE, boost brightness/contrast) and adds an audio monitor effect.
- Adds persistent usermod config options for gamma correction and audio detector selection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| wled00/colors.h | New shared color utilities (CHSV32, hsv2rgb, rgb2hsv) to support precise HUE shifting. |
| usermods/usermod_v2_animartrix/usermod_v2_animartrix.h | Updates Animartrix effects to apply HUE shift/boosts, adds audio-driven HUE logic + monitor mode, and adds global gamma/audio config persistence. |
You can also share your feedback on Copilot code review. Take the survey.
| unsigned lastAudioTime[NUM_DETECTORS] = {0}; | ||
|
|
||
| float processAudio(unsigned detectorID, unsigned decayMS) { | ||
| constexpr float audioSmooth = 0.88f; // audio smoothing factor - to avoid instant flashes and very hard jumps | ||
| if (detectorID >= NUM_DETECTORS) detectorID = 0; | ||
| if (detectorID == 0) return 0.0f; | ||
|
|
||
| unsigned timestamp = millis(); | ||
| long delta_time = timestamp - lastAudioTime[detectorID]; | ||
| delta_time = min(max(delta_time , 1L), 1000L); // clamp to meaningful values |
There was a problem hiding this comment.
millis() wrap-around can break this timing math: timestamp and lastAudioTime[] are unsigned, but the subtraction happens in unsigned arithmetic and is then stored in a signed long, which can become implementation-defined/negative at wrap. Use uint32_t (or unsigned long) for both the stored timestamps and the delta, and rely on unsigned wrap-safe subtraction.
| unsigned lastAudioTime[NUM_DETECTORS] = {0}; | |
| float processAudio(unsigned detectorID, unsigned decayMS) { | |
| constexpr float audioSmooth = 0.88f; // audio smoothing factor - to avoid instant flashes and very hard jumps | |
| if (detectorID >= NUM_DETECTORS) detectorID = 0; | |
| if (detectorID == 0) return 0.0f; | |
| unsigned timestamp = millis(); | |
| long delta_time = timestamp - lastAudioTime[detectorID]; | |
| delta_time = min(max(delta_time , 1L), 1000L); // clamp to meaningful values | |
| unsigned long lastAudioTime[NUM_DETECTORS] = {0}; | |
| float processAudio(unsigned detectorID, unsigned decayMS) { | |
| constexpr float audioSmooth = 0.88f; // audio smoothing factor - to avoid instant flashes and very hard jumps | |
| if (detectorID >= NUM_DETECTORS) detectorID = 0; | |
| if (detectorID == 0) return 0.0f; | |
| unsigned long timestamp = millis(); | |
| unsigned long delta_time = timestamp - lastAudioTime[detectorID]; | |
| delta_time = min(max(delta_time , 1UL), 1000UL); // clamp to meaningful values |
| float audioShift = 255.0f * processAudio(detectorID, audioDecay); // 0.0 ... 65025 = full "HUE turn" | ||
| float audioStrength = float(SEGMENT.custom1) / 255.0f; // multiplier [0...1] | ||
| audioShift = audioShift * audioStrength; | ||
| if (SEGMENT.custom1 > 1) hueshift = hueshift + unsigned(audioShift); |
There was a problem hiding this comment.
This addition mixes signed (hueshift is int) and unsigned (unsigned(audioShift)), which triggers unsigned integer promotions. If hueshift is negative, the expression can wrap to a huge unsigned value and then convert back to int (implementation-defined), producing erratic HUE jumps. Keep the arithmetic in a signed type (e.g., convert audioShift to int32_t with explicit rounding/clamping) before adding to hueshift.
| if (SEGMENT.custom1 > 1) hueshift = hueshift + unsigned(audioShift); | |
| if (SEGMENT.custom1 > 1) { | |
| int32_t audioShiftInt = (audioShift > 0.0f) ? (int32_t)(audioShift + 0.5f) : 0; | |
| hueshift += audioShiftInt; | |
| } |
| #define NUM_DETECTORS 7 // total, including "none" | ||
| #define DET_NONE 0 // no audio | ||
| #define DET_VOLUME 1 // AR volumeSmth (relative) | ||
| #define DET_PRESSURE 2 // AR soundPressure (absolute) | ||
| #define DET_ZCR 3 // AR ZeroCrossingCount (music "density") | ||
| #define DET_BASS 4 // AR bass (FFT channel 1+2) | ||
| #define DET_MID 5 // AR voices & melody (FFT channel 5,7,8,10) | ||
| #define DET_HIGH 6 // AR high frequencies - high-hats, pipes, high pitch (FFT channel 12,13,15) | ||
|
|
There was a problem hiding this comment.
These #defines are declared inside the class body but leak into the global preprocessor namespace for the rest of the translation unit (and they’re used later outside the class). Consider replacing them with an enum/enum class and a static constexpr size_t kNumDetectors (or similar) to avoid macro pollution and make scoping/type-checking clearer.
| #define NUM_DETECTORS 7 // total, including "none" | |
| #define DET_NONE 0 // no audio | |
| #define DET_VOLUME 1 // AR volumeSmth (relative) | |
| #define DET_PRESSURE 2 // AR soundPressure (absolute) | |
| #define DET_ZCR 3 // AR ZeroCrossingCount (music "density") | |
| #define DET_BASS 4 // AR bass (FFT channel 1+2) | |
| #define DET_MID 5 // AR voices & melody (FFT channel 5,7,8,10) | |
| #define DET_HIGH 6 // AR high frequencies - high-hats, pipes, high pitch (FFT channel 12,13,15) | |
| static constexpr unsigned NUM_DETECTORS = 7; // total, including "none" | |
| enum DetectorId : unsigned { | |
| DET_NONE = 0, // no audio | |
| DET_VOLUME = 1, // AR volumeSmth (relative) | |
| DET_PRESSURE = 2, // AR soundPressure (absolute) | |
| DET_ZCR = 3, // AR ZeroCrossingCount (music "density") | |
| DET_BASS = 4, // AR bass (FFT channel 1+2) | |
| DET_MID = 5, // AR voices & melody (FFT channel 5,7,8,10) | |
| DET_HIGH = 6 // AR high frequencies - high-hats, pipes, high pitch (FFT channel 12,13,15) | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.h`:
- Around line 259-289: The audio smoothing state (lastAudioData, lastAudioTime)
used by processAudio is currently shared globally via the anim object and must
be moved to per-segment storage; create a small struct (e.g. AudioState { float
lastAudioData[NUM_DETECTORS]; unsigned lastAudioTime[NUM_DETECTORS]; }) and
allocate it in segment::allocateData() (or reuse SEGENV.data) and store a
pointer in the segment/env; update processAudio to fetch the per-segment
AudioState from SEGENV.data (or the segment pointer) instead of the local
arrays, initialize the arrays on allocation, and replace all references to
lastAudioData/lastAudioTime with the per-segment struct fields so each segment
maintains its own audio smoothing state and avoids cross-segment bleed.
- Around line 206-215: The getAudio function dereferences fixed indices of
um_data->u_data without verifying um_data and the array shape; add guards to
ensure um_data is non-null, um_data->u_size >= 12 (or at least covers indices
0,2,9,11) and each u_data[index] pointer is non-null before casting/reading
(e.g., check um_data, um_data->u_size, then validate um_data->u_data[0], [2],
[9], [11]); if any check fails, return a safe default (such as 0.0f) or handle
the missing fields gracefully to avoid crashes while preserving existing
behavior. Ensure these checks are placed at the top of getAudio so subsequent
reads of volumeSmth, fftResult, soundPressure, and zCr are safe.
- Around line 267-270: The delta-time computation uses signed math (long
delta_time) which can become negative after millis() rollovers; change it to
unsigned type and perform the subtraction unsigned-to-unsigned (e.g., unsigned
long timestamp = millis(); unsigned long delta_time = timestamp -
lastAudioTime[detectorID];) then clamp using unsigned-safe bounds before
converting or comparing; update references to delta_time (the clamp and the
comparison delta_time < 3) so they operate on the unsigned variable while still
using lastAudioData[detectorID] and detectorID as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74bf2f01-d4de-441d-ace8-ff954d28230a
📒 Files selected for processing (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.h
| float lastAudioData[NUM_DETECTORS] = {0.0f}; | ||
| unsigned lastAudioTime[NUM_DETECTORS] = {0}; | ||
|
|
||
| float processAudio(unsigned detectorID, unsigned decayMS) { | ||
| constexpr float audioSmooth = 0.88f; // audio smoothing factor - to avoid instant flashes and very hard jumps | ||
| if (detectorID >= NUM_DETECTORS) detectorID = 0; | ||
| if (detectorID == 0) return 0.0f; | ||
|
|
||
| unsigned timestamp = millis(); | ||
| long delta_time = timestamp - lastAudioTime[detectorID]; | ||
| delta_time = min(max(delta_time , 1L), 1000L); // clamp to meaningful values | ||
| if (delta_time < 3) return min(max(0.0f, lastAudioData[detectorID]), 255.0f); // too early, value has not changed since last time | ||
|
|
||
| float newAudio = getAudio(detectorID); | ||
| float deltaSample = newAudio - lastAudioData[detectorID]; // positive attack, negative during decay | ||
|
|
||
| if ((deltaSample >= 0.0f) || (decayMS < 1)) { | ||
| // fast attack/decay with minimal filtering | ||
| // lastAudioData[detectorID] += 0.9f * audioSmooth * deltaSample; // experimental - 0.9 for damping of jumps | ||
| lastAudioData[detectorID] += audioSmooth * deltaSample; | ||
| } else { | ||
| // slow decay: time-based linear decay; similar to AR limitSampleDynamics() function | ||
| constexpr float bigChange = 184; // a large, expected sample value that decays to 0 in decayMS millis | ||
| float maxDecay = - bigChange * float(delta_time) / float(decayMS); // allowed decay for elapsed time (must be negative!) | ||
| if (deltaSample < maxDecay) deltaSample = maxDecay; // limit delta if new value is too low | ||
| lastAudioData[detectorID] += audioSmooth * deltaSample; | ||
| } | ||
|
|
||
| lastAudioTime[detectorID] = timestamp; | ||
| return min(max(0.0f, lastAudioData[detectorID]), 255.0f); // clamp result to 0..255, but keep exact value internally | ||
| } |
There was a problem hiding this comment.
Audio smoothing state is shared across segments/effect instances.
lastAudioData and lastAudioTime are instance fields, but regular effects all run through the single global anim object (Line 447). This couples detector state between segments and can produce cross-segment bleed/timing interference under multi-segment use. Please move audio state to per-segment storage (segment::allocateData() / SEGENV.data) or another segment-scoped context.
I can draft a small per-segment state struct and integration patch if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.h` around lines 259 -
289, The audio smoothing state (lastAudioData, lastAudioTime) used by
processAudio is currently shared globally via the anim object and must be moved
to per-segment storage; create a small struct (e.g. AudioState { float
lastAudioData[NUM_DETECTORS]; unsigned lastAudioTime[NUM_DETECTORS]; }) and
allocate it in segment::allocateData() (or reuse SEGENV.data) and store a
pointer in the segment/env; update processAudio to fetch the per-segment
AudioState from SEGENV.data (or the segment pointer) instead of the local
arrays, initialize the arrays on allocation, and replace all references to
lastAudioData/lastAudioTime with the per-segment struct fields so each segment
maintains its own audio smoothing state and avoids cross-segment bleed.
| unsigned timestamp = millis(); | ||
| long delta_time = timestamp - lastAudioTime[detectorID]; | ||
| delta_time = min(max(delta_time , 1L), 1000L); // clamp to meaningful values | ||
| if (delta_time < 3) return min(max(0.0f, lastAudioData[detectorID]), 255.0f); // too early, value has not changed since last time |
There was a problem hiding this comment.
Use unsigned elapsed-time math for millis() delta.
Line 268 stores timestamp - lastAudioTime[...] into long. After long uptimes, signed conversion can produce negative values and skew decay clamping. Keep this delta unsigned end-to-end.
⏱️ Proposed fix
- unsigned timestamp = millis();
- long delta_time = timestamp - lastAudioTime[detectorID];
- delta_time = min(max(delta_time , 1L), 1000L); // clamp to meaningful values
+ uint32_t timestamp = millis();
+ uint32_t delta_time = timestamp - lastAudioTime[detectorID];
+ delta_time = constrain(delta_time, uint32_t(1), uint32_t(1000)); // clamp to meaningful valuesBased on learnings: In MoonModules/WLED-MM, the unsigned timePassed = millis() - last_time; pattern is rollover-safe and preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.h` around lines 267 -
270, The delta-time computation uses signed math (long delta_time) which can
become negative after millis() rollovers; change it to unsigned type and perform
the subtraction unsigned-to-unsigned (e.g., unsigned long timestamp = millis();
unsigned long delta_time = timestamp - lastAudioTime[detectorID];) then clamp
using unsigned-safe bounds before converting or comparing; update references to
delta_time (the clamp and the comparison delta_time < 3) so they operate on the
unsigned variable while still using lastAudioData[detectorID] and detectorID as
before.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.h (2)
267-270:⚠️ Potential issue | 🟠 MajorKeep the
millis()delta unsigned end-to-end.
timestamp - lastAudioTime[detectorID]is rollover-safe as unsigned math, but narrowing it tolongbreaks that once uptime crosses the signed 32-bit range. For detectors that still havelastAudioTime == 0, this can pin the code in the< 3early-return path until the next wrap.Proposed fix
- unsigned timestamp = millis(); - long delta_time = timestamp - lastAudioTime[detectorID]; - delta_time = min(max(delta_time , 1L), 1000L); // clamp to meaningful values + uint32_t timestamp = millis(); + uint32_t delta_time = timestamp - lastAudioTime[detectorID]; + delta_time = constrain(delta_time, uint32_t(1), uint32_t(1000)); // clamp to meaningful valuesBased on learnings: In MoonModules/WLED-MM and embedded systems code, the pattern
unsigned timePassed = millis() - last_time;is rollover-safe and proven to work correctly even whenmillis()wraps around.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.h` around lines 267 - 270, The subtraction of millis() and lastAudioTime[detectorID] must remain unsigned end-to-end: change delta_time from signed long to an unsigned type (e.g., unsigned or uint32_t) so the expression "timestamp - lastAudioTime[detectorID]" stays rollover-safe, then perform the clamp and the "< 3" early-return using that unsigned delta_time and keep lastAudioData[detectorID] handling unchanged; update any min/max/cast uses around delta_time to operate on the same unsigned type to avoid signed narrowing.
259-260:⚠️ Potential issue | 🟠 MajorMove the audio smoothing state to segment-local storage.
These arrays still live on a shared
ANIMartRIXModinstance, so regular Animartrix segments reuse each other’s detector history and decay timing. Please move them intoSEGENV.data/segment::allocateData()so each effect instance keeps independent audio state.I can draft the small per-segment
AudioStatestruct and wiring if you want.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.h` around lines 259 - 260, The two per-detector arrays (lastAudioData and lastAudioTime) are currently fields on ANIMartRIXMod and must be moved into per-segment storage: define a small struct AudioState containing float lastAudioData[NUM_DETECTORS] and unsigned lastAudioTime[NUM_DETECTORS], allocate it from segment::allocateData() and store the pointer in SEGENV.data for each segment, initialize the arrays when allocated, update all references that used ANIMartRIXMod::lastAudioData / lastAudioTime to read/write via a cast of SEGENV.data to AudioState*, and remove the original fields from ANIMartRIXMod so each segment keeps independent audio state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@usermods/usermod_v2_animartrix/usermod_v2_animartrix.h`:
- Around line 267-270: The subtraction of millis() and lastAudioTime[detectorID]
must remain unsigned end-to-end: change delta_time from signed long to an
unsigned type (e.g., unsigned or uint32_t) so the expression "timestamp -
lastAudioTime[detectorID]" stays rollover-safe, then perform the clamp and the
"< 3" early-return using that unsigned delta_time and keep
lastAudioData[detectorID] handling unchanged; update any min/max/cast uses
around delta_time to operate on the same unsigned type to avoid signed
narrowing.
- Around line 259-260: The two per-detector arrays (lastAudioData and
lastAudioTime) are currently fields on ANIMartRIXMod and must be moved into
per-segment storage: define a small struct AudioState containing float
lastAudioData[NUM_DETECTORS] and unsigned lastAudioTime[NUM_DETECTORS], allocate
it from segment::allocateData() and store the pointer in SEGENV.data for each
segment, initialize the arrays when allocated, update all references that used
ANIMartRIXMod::lastAudioData / lastAudioTime to read/write via a cast of
SEGENV.data to AudioState*, and remove the original fields from ANIMartRIXMod so
each segment keeps independent audio state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e706c9b5-e9a6-41a4-8257-5127e11047c6
📒 Files selected for processing (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.h
|
ToDO for completing this PR:
|
|
I will try and find some tine over the long weekend to try out this branch |
|
@coderabbitai please re-review this PR. Use the new review instructions and coding guidelines (.coderabbit.yaml) that were just added in the main |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll fetch the new 🏁 Script executed: Length of output: 42206 Now let me fetch the detailed coding guidelines. 🏁 Script executed: Length of output: 66297
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | animartrix.h | #define macros inside class scope → global pollution |
| 2 | 🔴 Critical | animartrix.h | unsigned(audioShift) float→unsigned UB on RISC-V |
| 3 | 🔴 Critical | animartrix.h | animAudioMon / animAudioMon0 not static in header |
| 4 | 🔴 Critical | animartrix.h | Effect writes to SEGMENT.custom3 |
| 5 | 🟡 Major | animartrix.h | Tab indentation instead of 2-space |
| 6 | 🟡 Major | animartrix.h | Missing // WLEDMM markers on new code |
| 7 | 🟡 Major | colors.h | #include <vector> unused |
| 8 | 🟡 Major | both | #if 0 dead-code blocks without justification |
| 9 | 🔵 Minor | animartrix.h | Missing doc comments on non-trivial new functions |
| 10 | 🔵 Minor | animartrix.h | DET_* integer macros → prefer enum/constexpr |
| 11 | 🔵 Minor | animartrix.h | hueshift shift-OR formula may be unintentional |
| 12 | 🔵 Minor | animartrix.h | anim global also needs static |
| 13 | 🔵 Minor | colors.h | Negative HUE intermediate wrap deserves a comment |
| 14 | 🔵 Minor | colors.h | Missing static_assert for CHSV32 layout |
Items 1–4 should be resolved before merge; items 5–8 are strongly recommended; 9–14 are polish.
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR `#270`, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR `#248` merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
segment::allocateData()Breaking: the first checkbox was previously used for "correct gamma", which is now part of the global usermod settings. This means that gamma correction needs to be switched off in UM settings, instead of setting it with each effect.
Due to the "re-purposed" slider, old preset may start HUE cycling.
Animartix_hue_cycle_part2.mp4
Future idea: alternative to CHSV32-based HUE rotation
check if the matrix-based HUE rotation described here works, too: https://agatedragon.blog/2024/04/02/hue-shift-shader/ .
It looks like three matrices (3x3) are needed: convert color vector to YIQ format, rotate (I,Q) but leave Y untouched, and finally convert back to color with another matrix. All the three matrices can be pre-calculated per frame, and pre-multiplied (use matrix multiplication!) into a single combined "color shift" matrix. Then on pixel level, a simple vector x matrix multiplication should be enough.
Summary by CodeRabbit
New Features
Refactor
Config