Skip to content

Conversation

@Charming-Lime
Copy link

@Charming-Lime Charming-Lime commented Sep 3, 2025

Adds a new effect called Rainbow Shimmer effect. It takes the Rainbow Cycle effect but adds a white flash that runs across the segment. Adjustable frequency of flash, direction, speed of flash, and size.

Summary by CodeRabbit

  • New Features
    • Added "Rainbow Shimmer" effect: a rainbow base with a moving shimmer overlay for a vivid, dynamic look.
    • Adjustable controls: speed, frequency, size, intensity (length) and a direction toggle.
    • Works with existing rainbow palettes and appears in the effects list for immediate use.
    • Total number of available effects increased.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 3, 2025

Walkthrough

Adds a new FX mode "Shimmer": implements mode_shimmer() producing a rainbow base with a moving, distance-blended shimmer, adds its PROGMEM metadata string _data_FX_MODE_SHIMMER, registers the mode in WS2812FX::setupEffectData(), and defines FX_MODE_SHIMMER with updated MODE_COUNT.

Changes

Cohort / File(s) Summary
New effect implementation & registration
wled00/FX.cpp
Adds uint16_t mode_shimmer() implementing a rainbow base with a distance-blended moving shimmer, adds static const char _data_FX_MODE_SHIMMER[] PROGMEM, and registers the effect via addEffect(FX_MODE_SHIMMER, &mode_shimmer, _data_FX_MODE_SHIMMER) in WS2812FX::setupEffectData.
Effect ID definitions
wled00/FX.h
Adds #define FX_MODE_SHIMMER 218 and increments MODE_COUNT from 218 to 219.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • DedeHai
  • blazoncek

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4f55dc3 and c619a4e.

📒 Files selected for processing (1)
  • wled00/FX.cpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • wled00/FX.cpp
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
wled00/FX.cpp (1)

585-585: Metadata: consider reduced-resolution for Length and set sensible defaults

To avoid huge shimmer lengths with small segments, either:

  • mark “Shimmer Length” as reduced-resolution (0–31) in the metadata, or
  • clamp in code (as suggested). If you keep it reduced, add r-flags and/or set a tighter default via cX/ix.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 666a59f and 62b62d4.

📒 Files selected for processing (2)
  • wled00/FX.cpp (2 hunks)
  • wled00/FX.h (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use spaces (2 per level) for C++ source and header files

Files:

  • wled00/FX.h
  • wled00/FX.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.

Applied to files:

  • wled00/FX.h
  • wled00/FX.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.

Applied to files:

  • wled00/FX.h
  • wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.457Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.457Z
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”.

Applied to files:

  • wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
wled00/colors.cpp (2)
  • color_blend (11-21)
  • color_blend (11-11)
🔇 Additional comments (2)
wled00/FX.h (1)

377-378: Approve MODE_COUNT bump and effect registration
Manual check confirms highest FX_MODE_ ID (218) + 1 == MODE_COUNT (219), and addEffect(FX_MODE_RAINBOW_SHIMMER), its mode_rainbow_shimmer() implementation, and _data_FX_MODE_RAINBOW_SHIMMER are all present.

wled00/FX.cpp (1)

10707-10708: No changes needed: FX_MODE_RAINBOW_SHIMMER and MODE_COUNT are correct
FX_MODE_RAINBOW_SHIMMER is defined as 218 and MODE_COUNT is 219 (highest ID + 1), matching WLED conventions.

wled00/FX.cpp Outdated
Comment on lines 550 to 584
uint16_t mode_rainbow_shimmer() {

unsigned counter = (strip.now * ((SEGMENT.speed >> 2) + 2)) & 0xFFFF;
counter = counter >> 8;

uint32_t shimmerSpeed = 100 + (255 - SEGMENT.custom2) * 40; //ranges from 100-10260ms
uint32_t shimmerSize = (SEGMENT.custom3 * SEGLEN / 2 >> 5) + 1;
uint32_t cycleTime = (255 - SEGMENT.custom1) * 150 + shimmerSpeed;

uint32_t percCycle = strip.now % cycleTime;
float shimmerIndex = (float)percCycle / (float)shimmerSpeed * (SEGLEN + 2*shimmerSize);

shimmerIndex -= shimmerSize;

// reverse direction of shimmer
if(!SEGMENT.check1) {
shimmerIndex = (float)SEGLEN - shimmerIndex;
}


for (unsigned i = 0; i < SEGLEN; i++) {
// Standard rainbow effect.
uint8_t index = (i * (16 << (SEGMENT.intensity / 29)) / SEGLEN) + counter;
SEGMENT.setPixelColor(i, SEGMENT.color_wheel(index));

//shimmer logic
float distFromShimmerCenter = fabsf(shimmerIndex - i);
// Only process pixels that are within the shimmer's range.
if (distFromShimmerCenter < shimmerSize) {
SEGMENT.setPixelColor(i, color_blend(SEGMENT.getPixelColor(i), 0xFFFFFF, (uint8_t)(255 * (1.0f - (distFromShimmerCenter / shimmerSize)))));
}
}

return FRAMETIME;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix shimmer length scaling, avoid floats in hot loop, and make Reverse semantics intuitive

  • shimmerSize scales up to ~4× SEGLEN (at custom3=255), often turning the whole segment white. Clamp to [1..SEGLEN] (or a saner fraction) and compute with integer math.
  • The per‑pixel float math (fabsf, division, mul) inside the loop is costly on ESP8266/ESP32. Use integer math.
  • Reverse currently uses “if (!check1) reverse”, which is counter‑intuitive for a checkbox labeled “Reverse”.

Apply:

-  uint32_t shimmerSpeed = 100 + (255 - SEGMENT.custom2) * 40; //ranges from 100-10260ms
-  uint32_t shimmerSize = (SEGMENT.custom3 * SEGLEN / 2 >> 5) + 1;
-  uint32_t cycleTime = (255 - SEGMENT.custom1) * 150 + shimmerSpeed;
-
-  uint32_t percCycle = strip.now % cycleTime;
-  float shimmerIndex = (float)percCycle / (float)shimmerSpeed * (SEGLEN + 2*shimmerSize);
-  
-  shimmerIndex -= shimmerSize;
-  // reverse direction of shimmer
-  if(!SEGMENT.check1) {
-    shimmerIndex = (float)SEGLEN - shimmerIndex;
-  }
+  // speed: higher custom2 -> faster
+  uint32_t shimmerSpeed = 100u + (255u - SEGMENT.custom2) * 40u; // 100..10260 ms
+  // length: map to [1..SEGLEN] (or change divisor to limit max size, e.g. SEGLEN/3)
+  uint16_t shimmerSize = 1u + ((uint32_t)SEGMENT.custom3 * SEGLEN >> 8); 
+  if (shimmerSize > SEGLEN) shimmerSize = SEGLEN;
+
+  // frequency: gap between flashes
+  uint32_t cycleTime = (255u - SEGMENT.custom1) * 150u + shimmerSpeed;
+  uint32_t percCycle = strip.now % cycleTime;
+
+  // progress within the active flash window [0..shimmerSpeed)
+  uint32_t tInWindow = percCycle % shimmerSpeed;
+  uint32_t travel = SEGLEN + shimmerSize * 2u;           // allow off-edge fade-in/out
+  uint32_t shimmerIndex = (uint64_t)tInWindow * travel / shimmerSpeed;
+  int32_t center = (int32_t)shimmerIndex - shimmerSize;  // center of the flash
+  // Reverse = checked -> flip direction
+  if (SEGMENT.check1) center = (int32_t)SEGLEN - center;
@@
-  for (unsigned i = 0; i < SEGLEN; i++) {
+  for (unsigned i = 0; i < SEGLEN; i++) {
     // Standard rainbow effect.
     uint8_t index = (i * (16 << (SEGMENT.intensity / 29)) / SEGLEN) + counter;
     SEGMENT.setPixelColor(i, SEGMENT.color_wheel(index));
 
     //shimmer logic
-    float distFromShimmerCenter = fabsf(shimmerIndex - i);
-    // Only process pixels that are within the shimmer's range.
-    if (distFromShimmerCenter < shimmerSize) {
-      SEGMENT.setPixelColor(i, color_blend(SEGMENT.getPixelColor(i), 0xFFFFFF, (uint8_t)(255 * (1.0f - (distFromShimmerCenter / shimmerSize)))));
-    }
+    int32_t d = abs(center - (int32_t)i);
+    if (d < (int32_t)shimmerSize) {
+      uint8_t w = (uint8_t)(((uint32_t)(shimmerSize - d) * 255u) / shimmerSize);
+      SEGMENT.setPixelColor(i, color_blend(SEGMENT.getPixelColor(i), ULTRAWHITE, w));
+    }
   }
📝 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.

Suggested change
uint16_t mode_rainbow_shimmer() {
unsigned counter = (strip.now * ((SEGMENT.speed >> 2) + 2)) & 0xFFFF;
counter = counter >> 8;
uint32_t shimmerSpeed = 100 + (255 - SEGMENT.custom2) * 40; //ranges from 100-10260ms
uint32_t shimmerSize = (SEGMENT.custom3 * SEGLEN / 2 >> 5) + 1;
uint32_t cycleTime = (255 - SEGMENT.custom1) * 150 + shimmerSpeed;
uint32_t percCycle = strip.now % cycleTime;
float shimmerIndex = (float)percCycle / (float)shimmerSpeed * (SEGLEN + 2*shimmerSize);
shimmerIndex -= shimmerSize;
// reverse direction of shimmer
if(!SEGMENT.check1) {
shimmerIndex = (float)SEGLEN - shimmerIndex;
}
for (unsigned i = 0; i < SEGLEN; i++) {
// Standard rainbow effect.
uint8_t index = (i * (16 << (SEGMENT.intensity / 29)) / SEGLEN) + counter;
SEGMENT.setPixelColor(i, SEGMENT.color_wheel(index));
//shimmer logic
float distFromShimmerCenter = fabsf(shimmerIndex - i);
// Only process pixels that are within the shimmer's range.
if (distFromShimmerCenter < shimmerSize) {
SEGMENT.setPixelColor(i, color_blend(SEGMENT.getPixelColor(i), 0xFFFFFF, (uint8_t)(255 * (1.0f - (distFromShimmerCenter / shimmerSize)))));
}
}
return FRAMETIME;
}
uint16_t mode_rainbow_shimmer() {
unsigned counter = (strip.now * ((SEGMENT.speed >> 2) + 2)) & 0xFFFF;
counter = counter >> 8;
// speed: higher custom2 -> faster
uint32_t shimmerSpeed = 100u + (255u - SEGMENT.custom2) * 40u; // 100..10260 ms
// length: map to [1..SEGLEN]
uint16_t shimmerSize = 1u + ((uint32_t)SEGMENT.custom3 * SEGLEN >> 8);
if (shimmerSize > SEGLEN) shimmerSize = SEGLEN;
// frequency: gap between flashes
uint32_t cycleTime = (255u - SEGMENT.custom1) * 150u + shimmerSpeed;
uint32_t percCycle = strip.now % cycleTime;
// progress within the active flash window [0..shimmerSpeed)
uint32_t tInWindow = percCycle % shimmerSpeed;
uint32_t travel = SEGLEN + shimmerSize * 2u; // allow off-edge fade-in/out
uint32_t shimmerIndex = (uint64_t)tInWindow * travel / shimmerSpeed;
int32_t center = (int32_t)shimmerIndex - shimmerSize; // center of the flash
// Reverse = checked -> flip direction
if (SEGMENT.check1) center = (int32_t)SEGLEN - center;
for (unsigned i = 0; i < SEGLEN; i++) {
// Standard rainbow effect.
uint8_t index = (i * (16 << (SEGMENT.intensity / 29)) / SEGLEN) + counter;
SEGMENT.setPixelColor(i, SEGMENT.color_wheel(index));
// shimmer logic
int32_t d = abs(center - (int32_t)i);
if (d < (int32_t)shimmerSize) {
uint8_t w = (uint8_t)(((uint32_t)(shimmerSize - d) * 255u) / shimmerSize);
SEGMENT.setPixelColor(i, color_blend(SEGMENT.getPixelColor(i), ULTRAWHITE, w));
}
}
return FRAMETIME;
}

@DedeHai
Copy link
Collaborator

DedeHai commented Sep 3, 2025

Thanks for contributing.
I like the general idea, however, this is not a good way to do it.
First off, do not use floats.
With 0.16 there are elaborate ways to use layers. If you want to add a shimmer effect, it should do so as an overlay so it can be used with any effect. It aso should use palettes instead of just plain white, the default can still be white when doing that. If shimmer is its own independent effect, it could also be a bit more elaborate, like having different shimmer patterns, I am imagining a gradient wave as the base (as you have) but also adding the possibility to modulate that with a cosine to generate "multiple stripes" or maybe you can come up with some other things that look good.

@Charming-Lime
Copy link
Author

I understand the concern about floats; it was a deliberate decision to balance the desire for a smooth transition of the "shimmer" without obfuscating the code with a pseudo implementation of float using bit shifts into an arbitrary index that would be remapped to physical indices. In my testing the performance limitation was vastly dominated by the strip bus speed rather than limits on computational power, a 1000 pixel strip was still achieving 40 FPS, which was the same as the unmodified rainbow cycle. If you would prefer the latter implementation I will refactor.

I originally looked at implementing as an overlay, or how effects could be combined to achieve the same result, but I could not find documentation or effects designed for use that way. Considering 0.16 isn't released yet and there is no documentation outside of the comments on the PR, this would appear to be the first effect dependent on the new layer implementation; I admit I am playing catchup here. If the effect is intended to be secondary to a primary effect, adding color palette support is the natural extension of that line of thought.

On the topic of non-standalone effects, from a user perspective there should be a way in the UI to mark an effect as secondary or describe how effects should be combined with other. Maybe a way to default a primary and secondary overlapping effect? Otherwise, the user might be left wondering the purpose or how to use a secondary effect.

@DedeHai
Copy link
Collaborator

DedeHai commented Sep 3, 2025

floats are only available on ESP32 and S3, not on S2, C3 or ESP8266. So no floats unless absolutely necessary or for things that run only once in a while, it also increases code size.
The thing is, we do not want to increase the number of effects unless its really something new/unique. Otherwise, extending an existing effect is ok. Here there is potential for something unique.
Also there are already a few overlay effects. Scrolling text, sparkle, proably a few others. I would not call them secondary, those also look great on their own, which this one can too if done right.
edit:
documenting how to do composit/overlay effects is something anyone in the community can do, its not hard to figure out.

@Charming-Lime
Copy link
Author

As requested, removed float, added Fx and Bg colors. Removed the rainbow section for use as overlay.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 62b62d4 and 4f55dc3.

📒 Files selected for processing (2)
  • wled00/FX.cpp (2 hunks)
  • wled00/FX.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • wled00/FX.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use spaces (2 per level) for C++ source and header files

Files:

  • wled00/FX.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.

Applied to files:

  • wled00/FX.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.

Applied to files:

  • wled00/FX.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Applied to files:

  • wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#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”.

Applied to files:

  • wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
wled00/colors.cpp (2)
  • color_blend (11-21)
  • color_blend (11-11)
🔇 Additional comments (1)
wled00/FX.cpp (1)

10703-10703: Verify FX id alignment and default placement

Registration looks good next to Rainbow Cycle. Please double-check that:

  • FX_MODE_SHIMMER is defined in FX.h at the intended id and MODE_COUNT covers it (remember MODE_COUNT is highest id + 1).
  • No gap overlaps another effect’s id.

If you want, I can scan FX.h to confirm.

wled00/FX.cpp Outdated
Comment on lines 581 to 582
static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Frequancy,Size,,,Reverse;!!;sx=231,ix=221";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Typo in user-facing metadata and unclear parameter mapping

  • “Frequancy” should be “Frequency”.
  • Labels imply: Speed = speed slider, Frequency = intensity slider, Size = custom1, Reverse = check1. That matches the code. Fix the typo to avoid UI polish regressions.

Apply:

-static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Frequancy,Size,,,Reverse;!!;sx=231,ix=221";
+static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Frequency,Size,,,Reverse;!!;sx=231,ix=221";
📝 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.

Suggested change
static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Frequancy,Size,,,Reverse;!!;sx=231,ix=221";
static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Frequency,Size,,,Reverse;!!;sx=231,ix=221";
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 581-582, the user-facing metadata string contains
a typo: "Frequancy" should be "Frequency"; update the PROGMEM string to correct
the spelling while keeping the existing parameter order and mapping
(Speed,Frequancy,Size,,,Reverse) unchanged so Speed maps to speed slider,
Frequency to intensity slider, Size to custom1 and Reverse to check1.

@DedeHai
Copy link
Collaborator

DedeHai commented Sep 4, 2025

thanks for the update, good progress.
I have a few points:

  • 64bit math is about as bad as float, please read [INFO] Code execution speed considerations for developers #4206
  • if you adjust the speed, there is no need for 64bit
  • your numbers in the code are wrong
  • remove white spaces
  • fix the metadata string (please thoroughly test your code, this is an obvious one)
  • you are doing double-inverse in color_blend, which is unnecessary

also you did not add palettes as requested, please do so and we have the basic structure for this effect. What I would later also like to add is:

  • random interval as a slider, the more random, the further it strays from given interval (or something like that)
  • possibility to add brightness modulators to change the looks, like making it two smaller shimmers instead of one large one for exmple by overlaying multiple gradients on top of the base gradient, or as mentioned, a cosine wave may do it. or even add perlin noise as an option.

with palettes added, a user can create a custom palette to only partially apply the shimmer and with modulators it may look a lot better when not using it as an overlay. Please let me know if (and at what point) this gets beyond the time you are willing to invest.

segmentSize=SEGLEN;
}

uint32_t shimmerSpeed = 100 + (255 - SEGMENT.speed) * 40; // [100,10300]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you change 100 to 200 in this line, no overflow and you do not have to limit the length.

// slightly dangerous, but shimmer index is bounded to be safe
uint32_t distFromShimmerCenter = abs((int32_t)shimmerIndex - ((int32_t)i << 8));
if (distFromShimmerCenter < (shimmerSize<<8)) {
SEGMENT.setPixelColor(i, SEGMENT.color_from_palette(255-(distFromShimmerCenter / shimmerSize),false,PALETTE_SOLID_WRAP,0));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot the blend

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't. It is up to the user to select a color palette that blends (or contrasts if desired) with the selected background color. Any additional blending between layers will happen using #4658 Enhanced Layering facilities. Adding additional blending into the loop could limit the number of ways the effect could be used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without the gradient, there are other effects that do almost the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what exactly should the gradient be with?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it was, plus the suggestions: cosine, sub-gradiens, perlin

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this effect to be a simple, lite occasional flair to a relatively plain and simple back round, as every other effect had too much noise, randomness, and was always visible. Adding all of those gradients are well outside the scope of what I want my effect to be. I think we have very different ideas of what this effect should be so I am going to close this and just maintain my own fork. I'm happy to adapt to make my effect more versatile and flexible, but I am not going to make something I don't want.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you misunderstood.

@Charming-Lime Charming-Lime deleted the effect_rainbow_shimmer branch September 4, 2025 21:12
@DedeHai DedeHai mentioned this pull request Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants