PlummersSoftwareLLC
Color Meteor effect does not use first LED in strip
I set up the EFFECTS_ATOMLIGHT configuration and noticed the color meteor effect was not using the first LED in the strip (LED 0).
Making the following changes locally fixed it, but I do not have permissions to create a branch.
meteoreffect.h
if (iPos[i] < meteorSize - 1) { bLeft[i] = false; iPos[i] = meteorSize - 1; }
if ((x <= static_cast<int>(pGFX->GetLEDCount())) && (x >= 0))
I spent unreasonable time staring at this. I didn't like the original (sorry) and I liked the replacement even less. Anytiem I monkey with the same pivot point three times in a single line of code that involve off-by-ones, experience has taught me chan I can often make it work, but iit doesn't spark joy. In one change, we move a variable's range down to zero, added a test <=0 for it, and then still subtracted one from it somewhere else. Those are the things that leave me questioning life choices.
We are on register rich targets with decent optimizers and have opcoes for clamp. We don't have do do some of those crazy test sequences.
virtual void Draw(std::shared_ptr<GFXBase> pGFX)
{
static CHSV hsv;
hsv.val = 255; *// Add this. Keep it live. Think about cases <
0*
hsv.sat = 240; *const int ledCount =
static_cast<int>(pGFX->GetLEDCount());*
for (int j = 0; j<pGFX->GetLEDCount(); j++)
* for (int j = 0; j < ledCount; j++); // One edge to the
other. Period.*
// fade brightness all LEDs one step
{. *// Couuld due with segments. IDC.*
if ((!meteorRandomDecay) || (random_range(0, 10)>2))
// BUGBUG Was 5 for everything before atomlight
{
CRGB c = pGFX->getPixel(j);
c.fadeToBlackBy(meteorTrailDecay);
pGFX->setPixel(j, c);
}
}
for (int i = 0; i < meteorCount; i++)
{
float spd = speed[i];
#if ENABLE_AUDIO
if (g_Analyzer._VURatio > 1.0)
spd *= g_Analyzer._VURatio;
#endif
iPos[i] = (bLeft[i]) ? iPos[i]-spd : iPos[i]+spd;
if (iPos[i]< meteorSize)
{
bLeft[i] = false;
iPos[i] = meteorSize;
}
if (iPos[i] >= pGFX->GetLEDCount())
{
bLeft[i] = true;
iPos[i] = pGFX->GetLEDCount()-1;
}
for (int j = 0; j < meteorSize; j++) // Draw
the meteor head
{
int x = iPos[i] - j;
if ((x <= pGFX->GetLEDCount()) && (x >= 1))
{
CRGB rgb;
On Tue, Dec 9, 2025 at 11:35 PM Cyborg-Squirrel @.***> wrote:
Cyborg-Squirrel created an issue (PlummersSoftwareLLC/NightDriverStrip#791) https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/791
I set up the EFFECTS_ATOMLIGHT configuration and noticed the color meteor effect was not using the first LED in the strip (LED 0).
Making the following changes locally fixed it, but I do not have permissions to create a branch.
meteoreffect.h
if (iPos[i] < meteorSize - 1) { bLeft[i] = false; iPos[i] = meteorSize - 1; }
if ((x <= static_cast
(pGFX->GetLEDCount())) && (x >= 0)) — Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/791, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD3ZAFZRGDM4PS4Q4G4T4A6WKJAVCNFSM6AAAAACOSMTHDGVHI2DSMVQWIX3LMV43ASLTON2WKOZTG4YTGNRWGY3TAOA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
Darn it. Premature send.
Now, precompute some stuff beore looping.
iPos[i] = (bLeft[i]) ? iPos[i]-spd : iPos[i]+spd;
if (iPos[i]< meteorSize)
{
bLeft[i] = false;
iPos[i] = meteorSize;
}
if (iPos[i] >= pGFX->GetLEDCount())
{
bLeft[i] = true;
iPos[i] = pGFX->GetLEDCount()-1;
}
*// Calculate new position*
* float posn = iPos[i] + (bLeft[i] ? -spd : spd);*
-
.. UNambiguous Bounds for head. const
int minPos = meteorSize - 1; const int maxPos =
ledCount - 1; // Clamp and update direction if we
hit a wall iPos[i] = std::clamp
-
// Might be able to simplify this. bLeft[i] = (pos
= maxPos) || (bLeft[i] && pos > minPos); // Draw the meteor int headPos = iPos[i]);*
-
for (int j = 0; j < meteorSize; j++)* -
{* int x =
static_cast
- 1] {* int x = iPos[i] - j;*
-
// ... now use rest.*
On Wed, Dec 10, 2025 at 2:40 AM Robert Lipe ***@***.***> wrote:
> I spent unreasonable time staring at this. I didn't like the original
> (sorry) and I liked the replacement even less. Anytiem I monkey with the
> same pivot point three times in a single line of code that involve
> off-by-ones, experience has taught me chan I can often make it work, but
> iit doesn't spark joy. In one change, we move a variable's range down to
> zero, added a test <=0 for it, and then still subtracted one from it
> somewhere else. Those are the things that leave me questioning life choices.
>
> We are on register rich targets with decent optimizers and have opcoes for
> max and clamp. We don't have do do some of those crazy test sequences.
> where we have to re-read a variable to see if its the max just because we
> just reset it...to the min, fo example. We don't need if/then/else at all
> in most of those places,
>
Start with something like:
> ```
> virtual void Draw(std::shared_ptr<GFXBase> pGFX)
> {
> static CHSV hsv;
> hsv.val = 255; *// Add this. Keep it live. Think about cases
> < 0*
> hsv.sat = 240; *const int ledCount =
> static_cast<int>(pGFX->GetLEDCount());*
>
> for (int j = 0; j<pGFX->GetLEDCount(); j++)
> * for (int j = 0; j < ledCount; j++); // One edge to the
> other. Period.*
> // fade brightness all LEDs one step
> {. *// Couuld due with segments. IDC.*
> if ((!meteorRandomDecay) || (random_range(0, 10)>2))
> // BUGBUG Was 5 for everything before atomlight
> {
> CRGB c = pGFX->getPixel(j);
> c.fadeToBlackBy(meteorTrailDecay);
> pGFX->setPixel(j, c);
> }
> }
>
> for (int i = 0; i < meteorCount; i++)
> {
> float spd = speed[i];
>
> #if ENABLE_AUDIO
> if (g_Analyzer._VURatio > 1.0)
> spd *= g_Analyzer._VURatio;
> #endif
>
> iPos[i] = (bLeft[i]) ? iPos[i]-spd : iPos[i]+spd;
> if (iPos[i]< meteorSize)
> {
> bLeft[i] = false;
> iPos[i] = meteorSize;
> }
> if (iPos[i] >= pGFX->GetLEDCount())
> {
> bLeft[i] = true;
> iPos[i] = pGFX->GetLEDCount()-1;
> }
>
> for (int j = 0; j < meteorSize; j++) //
> Draw the meteor head
>
> {
> int x = iPos[i] - j;
> if ((x <= pGFX->GetLEDCount()) && (x >= 1))
> {
> CRGB rgb;
>
I don't have a strip here in bed to test it, but I think you can make that whole block into simple addition and subtraction that does straight-shot things, calls things what the are, and compares against what they are - :no foo+1 and foo <= foo-1 .... add +1 back to it" kin of changes needed.
The oiriginal author(s) may find that worse, but from a readibility/maintenance/performance case, naming things, populating them once instead of reloading them endlessly, and reducing the number of times you hit the wall of being off by one while trying to fix an off by one just just seems lower.
It may take a few days as I have (more) travel coming up, but if someone would like to take a run at that and wants help pencil whipping it, cc me on a PR for review.
There are lots of right way to write code like this. There are WAY MORE ways that are almost right and those are the ones hou have to learn to recognize and flee. There were just so many sinkk holes here involving off-by-ones and testing things we'd just changed and such that it seemed worth trying to put myself to sleep (some people do Suduko...) with some of those ideas. I think the basic ideas are OK, but they're not tested and may not even compiler.
On Tue, Dec 9, 2025 at 11:35 PM Cyborg-Squirrel @.***> wrote:
Cyborg-Squirrel created an issue (PlummersSoftwareLLC/NightDriverStrip#791) https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/791
I set up the EFFECTS_ATOMLIGHT configuration and noticed the color meteor effect was not using the first LED in the strip (LED 0).
Making the following changes locally fixed it, but I do not have permissions to create a branch.
meteoreffect.h
if (iPos[i] < meteorSize - 1) { bLeft[i] = false; iPos[i] = meteorSize - 1; }
if ((x <= static_cast
(pGFX->GetLEDCount())) && (x >= 0)) — Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/791, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD3ZAFZRGDM4PS4Q4G4T4A6WKJAVCNFSM6AAAAACOSMTHDGVHI2DSMVQWIX3LMV43ASLTON2WKOZTG4YTGNRWGY3TAOA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
Using your changes also solved the bug.
virtual void Draw(LEDStripEffect* owner)
{
auto pGFX = owner->g(0);
const int ledCount = static_cast<int>(pGFX->GetLEDCount());
static CHSV hsv;
hsv.val = 255;
hsv.sat = 255;
// Fade brightness on all channels
for (int j = 0; j < ledCount; j++)
{
if ((!meteorRandomDecay) || (random_range(0, 10) > 2))
{
owner->fadePixelToBlackOnAllChannelsBy(j, meteorTrailDecay);
}
}
for (int i = 0; i < meteorCount; i++)
{
float spd = speed[i];
#if ENABLE_AUDIO
if (g_Analyzer.VURatio() > 1.0f)
spd *= g_Analyzer.VURatio();
#endif
float posn = iPos[i] + (bLeft[i] ? -spd : spd);
const int minPos = meteorSize - 1;
const int maxPos = ledCount - 1;
iPos[i] = std::clamp<float>(posn, minPos, maxPos);
bLeft[i] = (posn <= minPos || posn >= maxPos) ? !bLeft[i] : bLeft[i];
// Draw the meteor head across all channels
for (int j = 0; j < meteorSize; j++)
{
int x = static_cast<int>(iPos[i]) - j;
if (x < ledCount && x >= 0)
{
CRGB rgb;
hue[i] = hue[i] + 0.025f;
if (hue[i] > 255.0f)
hue[i] -= 255.0f;
hsv.hue = static_cast<uint8_t>(hue[i]);
hsv2rgb_rainbow(hsv, rgb);
// Blend with current pixel from primary device, then set on all channels
CRGB c = pGFX->getPixel(x);
nblend(c, rgb, 75);
owner->setPixelOnAllChannels(x, c);
}
}
}
}
I whacked on it even harder. Arrays of Structs > Structs of arrays. Other cleanups applied.
Please confirm that https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/792 keeps your build in a happy place.
It looks the same on my hardware, but it's a configuration that's not checked in.