Skip to content

Fix Segment::fade_out for 2d expansion #4712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
willmmiles merged 3 commits into from
Jul 9, 2025
Merged

Fix Segment::fade_out for 2d expansion #4712

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4abaf13
Fix Segment::fade_out for 2d expansion
willmmiles Jun 7, 2025
94f226a
Fix additional mapping expansion locations
willmmiles Jun 9, 2025
16a8877
Revert changes to blur()
willmmiles Jun 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,8 @@ void Segment::fade_out(uint8_t rate) const {
if (!isActive()) return; // not active
rate = (256-rate) >> 1;
const int mappedRate = 256 / (rate + 1);
for (unsigned j = 0; j < vLength(); j++) {
const size_t length = is2D() ? (vWidth() * vHeight()) : vLength();
Copy link
Contributor

@blazoncek blazoncek Jun 8, 2025

Choose a reason for hiding this comment

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

This shouldn't be necessary as vLength()/virtualLength()takes into account 2D (and its mapping).

Copy link
Member Author

@willmmiles willmmiles Jun 9, 2025

Choose a reason for hiding this comment

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

That would be correct if fade_out was using getPixelColor()/setPixelColor(), which apply the mapping. Right now it's using getPixelColorRaw()/setPixelColorRaw(), so it needs the "unmapped" length.

The tradeoff is between calculating the color fade extra times, vs applying the mapping. Given that the author of the function chose the Raw forms I took this to mean that they intended the former, which I agree is probably faster in most cases.

The bug is pretty clearly apparent with any of the Grav* AR FX with bar expansion -- it doesn't fade out properly.

Copy link
Contributor

@blazoncek blazoncek Jun 9, 2025

Choose a reason for hiding this comment

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

Right. The author would be me.
That only means fade_out() should use regular get/setPixelColor() and skip speed optimisations. This might hold true for all universal functions.
Or re-implement mapping to correctly iterate expanded pixels.

Copy link
Member Author

@willmmiles willmmiles Jun 9, 2025

Choose a reason for hiding this comment

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

OK - I think the optimization is helpful here, but if you feel strongly that we should go the other way, that's OK by me. (Now that you mention it, there are a couple of other locations with the same issue - I should've checked more thoroughly when I spotted the one.)

Long term I think we should move any and all mapping to the blending stage, where we are guaranteed we're only calculating it once per frame. (Yes, I'm aware there are a couple of FX that do special things in certain mapping modes - I'm sure we can find solutions for them.)

Copy link
Contributor

@blazoncek blazoncek Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO the next best thing is to do for (int i=0; i<length(); i++) ... as that will cover all pixels.
I think that will still be faster than mapping.
This is feasible as actual rendering is done in blending function.

Copy link
Member Author

@willmmiles willmmiles Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about length() too; I went with vWidth()*vHeight() as it's smaller for any case with grouping or mirroring.

Copy link
Contributor

@blazoncek blazoncek Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If everything would be properly implemented those 2 functions would be unavailable in WLED_DISABLE_2D.
But I was lazy.

Copy link
Member Author

@willmmiles willmmiles Jun 9, 2025

Choose a reason for hiding this comment

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

Sometimes lazy is good. :)

I'll update with a fix for all cases.

for (unsigned j = 0; j < length; j++) {
uint32_t color = getPixelColorRaw(j);
if (color == colors[1]) continue; // already at target color
for (int i = 0; i < 32; i += 8) {
Expand Down