Copy Segment FX #4124
Copy Segment FX #4124
Conversation
- copies the source segment - brightness of segment is relative to source segment - optionally shifts the color hue - invert, transpose, mirror work - if source or targets do not match in size, smallest size is copied - unused pixels fade to black (allows overlapping segments) - if invalid source ID is set, segment just fades to black - added a rgb2hsv conversion function as the fastled variant is inaccurate and buggy note: 1D to 2D and vice versa is not supported
|
just realised: if the source segment is inverted, the targed does not invert (which is ok) but if the source is mirrored, only half of the pixels are copied unless the mirror settings in the target segment are the same. |
My quick suggestion would be to use I don't have a good setup for testing this but I'll give it a go when I can. |
|
I did not know about this distinction, that is actually what I was looking for :) |
|
that does not seem to work as intended. the numbers are correct (i.e. the width is not halfed if mirrored) but one half still stays black. |
We are being foiled by |
wled00/FX.cpp
Outdated
| if(SEGMENT.custom2 > 0) // color shifting enabled | ||
| { | ||
| CHSV pxHSV = rgb2hsv(sourcecolor); //convert to HSV | ||
| pxHSV.h += SEGMENT.custom2; // shift hue | ||
| hsv2rgb_spectrum(pxHSV, sourcecolor); // convert back to RGB | ||
|
|
||
| } |
There was a problem hiding this comment.
This gets copy and pasted below - probably it wants to be a little static function instead.
There was a problem hiding this comment.
indeed, can you help out? I do not know what the best way to implement it is. just adding a function?
There was a problem hiding this comment.
Yup, pretty much. Something like:
static CRGB shift_hue(CRGB sourcecolor, uint8_t amount) {
if (amount > 0) {
CHSV pxHSV = rgb2hsv(sourcecolor); //convert to HSV
pxHSV.h += amount; // shift hue
hsv2rgb_spectrum(pxHSV, sourcecolor); // convert back to RGB
}
return sourcecolor;
}My inclination would be to leave it here in FX.cpp (hence static), to allow the compiler to inline it if it wants to. Organizationally though it might be a better fit for colors.cpp.
There was a problem hiding this comment.
oh, I though you meant some fancy C++ macro magic ;)
but good point, will add this to color utils
There was a problem hiding this comment.
if the shift_hue function is on a critical path (like pixel copy), then it would be smarter to put the static function into fx.cpp so the compiler can inline. Inlined functions are faster (no overhead due to call-and-return).
But the compiler will not inline things that are defined in a different .cpp file.
|
I tested just copying the source settings by doing |
|
@DedeHai looks like start offsets are missing. strip.setPixelColorXY(start + width() - xX - 1, startY + height() - yY - 1, tmpCol); |
correct, can confirm that adding |
target segment now also copies the source settings (mirror, grouping, spacing, transpose) so the target looks exactly like the source. inverting can still be set independently. note: the bug in line 214 of FX_2Dfcn.cpp missing `start` needs to be fixed for it to work properly in 2D.
|
did some testing on the latest commit. it works fine with one exception: setting spacing other than 0 seems to screw things up. the target segment does weird things and I have no idea how to fix it. |
This feels wrong to me - I want to set my own mirroring or spacing settings on the destination segment and they should be respected, not overridden. However I expect the 'source data' to be the rendered output, including the source's mirroring/reversing/grouping etc. Spacing is where things get tough, though. Spacing+offset is for creating interleaved segments. My gut feeling is to pretend the gaps don't exist when copying -- leave it to the destination segment to decide how to render those. I think this might be a bit tricky; I'll sketch some code. |
it does feel wrong, but you cannot copy data that just is not there. like if source is mirrored, target cannot be 'unmirrored' without re-calculating the FX. 'reverse' is still possible (and is not copied). what I did not test at all yet: what happens if global buffer is disabled. |
I don't think that's true - your original logic with That was the original concern that spawned this discussion, I thought; we wanted the copy to operate on the "rendered" mirrored/reversed/grouped FX output, so if the destination segment had no flags set, it would look completely identical to the source segment. I would then expect that any flags set on the destination are applied normally to the rendered copy. To implement this, it looks like we'd need a new "getRenderedPixelColor()" that has slightly different index calculation logic. I have a working 1D version at home, sorry I didn't get time to publish it last night. That said: if we don't want to go that way, I think the reasonable alternative is to ask the user to manually harmonize the segment flags they want. For example, if the source segment is 8x16 (mirrored, so the FX calculates 8x8), I think it's reasonable for the copy operation to treat that as an 8x8 source (eg. exactly your original code); and then it's up to the user to set the mirror flag on the destination segment if they also want it mirrored. To sum up: I think either we should copy from either the "rendered" space, for which we need new getPixelColor flavors; or from the "FX" space, ignoring the source segment flags, as your original code does. In either case the destination segment render flags should be set by the user, I really don't think we should override them. Note I've completely passed over "spacing" and "offset" here -- IMO these options should be ignored by the copy operation as their purpose is to specify which pixels are /not/ part of the segment, and thus shouldn't be considered for the copy. |
|
I tried using .length() instead of virtual and confirmed the loop went over the full index range but if mirror of source is active, it sill only gets half of the data, the rest is still black (which is weird). when you say 'rendered space' that is the buffer that is sent out and 'FX space' is that same buffer but with 'mapped' access right? I still don't fully grasp on what is mapped how as in the code it is really hard to grasp where a pixel color actually gets written to (or read from). |
I did implement "raw" get/setPixelColor (in one of my POCs) that did no translations but it proved wrong and there were always ways that it did not work properly. |
This is the expected behavior of
I'd defined "render space" as "the pixels that are actually written to" without any gaps. This might just be my idiosyncratic way of of thinking about it though... The way I think of it is:
[ 1, 2, 3, 4 ] FX space is mapped to a "render space" via mirror, transpose, reverse, grouping. (There are currently no functions to access this; I had proposed it as new concept to use as a copy source.) mirror - note length is doubled from FX space grouping=2, mirror, reverse - length x2 for mirror, x2 again for grouping "Render space" is then mapped to "strip space" with start, stop, spacing and offset. Note that spacing is applied in between groups. "strip space" can be accessed via start=3, spacing = 1, offset = 0, mirror start=0, spacing = 1, offset = 1, mirror start=0, spacing = 1, offset = 0, grouping=2, mirror, reverse - length x2 for mirror, x2 again for grouping Finally "strip space" is mapped to actual LED addresses using ledmap.json. (Not shown, but I mention for completeness.) Anyways it sounds like the right thing to copy is FX space. I still feel strongly that we should not be editing the segment properties in the FX function -- the software expects those to be inputs, not outputs, and you'll have a bunch of weird side effects on the UI and config management as the software doesn't expect those to be writable by FX.
Yeah I'd had a feeling we could get away with something there, but I hadn't worked through all the implications yet. There's also the 1D->2D conversion to consider; |
|
Ah get it, if you access outside of 'virtual space' it does not return a valid color, hence getting black. edit: |
🤔 I think @willmmiles has described it quite accurate, maybe just a bit over-compilcated.
As you want to copy effects ( = segments), I thinks its enough to copy what The target segment might not be the same size as the source segment -> so maybe implement your own version of "mirror" and "mirrorY", to preserve the overall look. But don't try to duplicate segments settings because the result will not look the same any way. Dimensions might be completely different, so the mirror axis will usually not match with the source segment. Edit: sorry made a mistake im my original comment - |
|
PS: if you need a few nasty testcases
|
I'd say its better to always use the "right" versions - 1D or 2D - if you are on Actually this function is a good example how to handle 1D and 2D (just except for the |
|
I think a lot of the challenge here comes from some of the corner cases. If I have a segment 1 set as1x8, with mirror enabled, I get virtualLength() == 4, values [1 2 3 4], and what is drawn on segment 1 is [1 2 3 4 4 3 2 1]. If I then set "Copy" on Segment 2 (also 1x8) - what should I get? The simple implementation (434ba3f) will give me [1 2 3 4 X X X X], where X is off. I have to set 'mirror' on segment 2 to make it actually look like a copy of segment 1. This seems counterintuitive, why don't I get a "whole" copy? Supposing we accept that logic - if segment 1 is mirrored, we should render the mirrored output on segment 2. Mirror, reverse, and transpose all seem reasonable to copy from -- though as @softhack007 notes, copying the flags won't do what we're expecting if the segment sizes don't match. Grouping? Maybe - this is somewhat less clear. Spacing? Definitely not, weird stuff will happen. What if segment 2 is 1x16, and has the mirror flag set on it? With the naiive code, I'd get [ 1 2 3 4 X X X X X X X X 4 3 2 1]? Or should I expect [1 2 3 4 4 3 2 1 1 2 3 4 4 3 2 1]? This is why I added that idea of "render space" above - to capture what I think we want as the "copy source". Anyhow, I suggest https://github.com/willmmiles/WLED/tree/pr/DedeHai/4124 as an example implementation - it's lame but I think it should copy the "obvious" way without disrespecting the target segment settings. Note I've only tested on 1D, I don't have a 2D system on my bench right now. :( Note that we don't need to check the source segment sizes - getRenderedPixelXY checks the coordinate validity and returns black if it's out of range - so we can also omit the fadetoblack, too. |
|
I tested temporarily setting the target segment settings to the sorce settings (which can be enabled in the FX by a check). This works but is not perfect (does not solve the things mentioned about non-equal sizes, mirroring will leave a gap in the center but that is a separate issue). Edit: |
I dug out a 2D panel and tested the implementation I sketched last night: https://github.com/willmmiles/WLED/tree/pr/DedeHai/4124 -- it seems like it handled every case I tried reasonably well, even 1D->2D conversion; though it's still missing the 2D->1D conversion case. Give it a go and let me know what you think? |
|
I tested your version, there is a few bugs which I fixed and now it works amazingly well and does exactly what we discussed. There is still some cases I want to run but generally this is the way to go. |
|
Apparently I don't do enough with 2D. ;) Possibly the offset field is carrying some historical value, as it's not used in the standard 2D calculations. I can think of two ways to approach fixing that: either (a) separate the 1D and 2D |
- added color adjust function with support for hue, lightness and brightness - colors can now be adjusted in hue, saturation and brightness - added `getRenderedPixelXY()` function (credit @willmmiles) - source is now accurately copied, even 1D->2D is possible - fix for segment offset not being zero in 2D in `getRenderedPixelXY()` - added checkmark to copy source grouping/spacing (may remove again) - tested many different scenarios, everything seems to work
|
latest commit now seems to handle all test cases (even the nasty ones). There are a few things that do not work properly though:
edit: edit2: |
|
@DedeHai please merge blending styles branch (to a new branch) to see how different blending affects your new effect. |
|
sorry for messing up the blending styles branch... |
There was a problem hiding this comment.
I am not in favour of this PR as in my opinion copying segments (as different as they may be) is not the correct approach. It may also lead to user frustration in the long run.
As the underlying issue is "synchronised" segments (i.e. segments that mimic each other) a correct approach would be to execute effect function original segment uses on the destination segment with parameters (and timers and PRNG) of the original segment.
that would indeed be the best approach BUT: how do you control it? are FX with the same parameters always in sync (if started with the same timebase)? This whole endevour is mostly to sync particle effects, which are frame based (and I have no intention to change that as it makes collisions and speeds way more complex to calculate and therefore a lot slower, the only workaround would be to have fixed frame rates but that poses new issues). |
|
Consider the complexity of overlapping segments. |
|
overlapping segments work fine, but you are correct that it is a bit complex i.e. the segment layering has to be done correctly. This is already the case for overlapping segments, adding copy FX on top does not add much to complexity. |
|
I disagree. Consider the following valid example: Segment 2 will show artifacts from underlying Segment 0 which is unwanted. I would consider this a flaw and would report a bug as I only want to copy Segment 1 and not the underlying segment. If it is to be done, it has to be done correctly or not at all. |
|
I think the implementation of "copy the rendered buffer including all underlays at that segment layer" is probably the correct behavior. It's true there might be some counterintuitive cases, but it offers a lot of opportunity for clever compositions. With some easy extensions, there are some really cool tricks we could use it for.
As with all powerful features -- like segment overlays themselves! -- there's going to be some sharp edges and cases where it takes a little planning to compose the FX you want to see. Still, I think this feature offers a surprisingly large amount of new capability for a minimum of code. My $0.02, though! |
Yes if that was optional. Now it is not. The solution is to optionally enable individual segment buffer for rendered pixels. So that user has an option if he/she wants underlying segments copied or not.
I do not disagree with that. To answer @DedeHai regarding "effect blending styles": Yes, not all styles will look great with every effect. I particularly dislike "push" variants as the outcome may be weird to say the least. Yet, they are there to be in line with #3669 . Still, the user has an option to select them or not. The code is also largely untested and there may be bugs still present. |
|
I fully agree with @willmmiles: while there are some limits in the current state of things, this PR adds a lot of value for the average use case.
I see no problem in adding this later when (or if) individual segment buffers are implemented
same goes for the copy FX :) edit: |
- Bonus: saves over 1.2kB of flash -also added a struct to handle HSV with 16bit hue better (including some conversions, can be extended easily) - the functions are optimized for speed and flash use. They are faster and more accurate than what fastled offers (and use much less flash). - replaced colorHStoRGB() with a call to the new hsv2rgb() function, saving even more flash - the 16bit hue calculations result in an almost perfect conversion from RGB to HSV and back, the maximum error was 1/255 in the cases I tested.
|
just FYI: with the latest commit the code size shrinks by 1.2kB, making it 200bytes smaller in total than vanilla 0.15 :) |
|
You have started to mix two different things into this PR. |
|
that is a good point. the changes are kind of 'required' to improve this PR, but keeping optimizations separated is cleaner. Will move it over to the optimization PR and revert the changes here. |
IMO no need as this PR will be merged after the optimisations one. If you keep source files identical no ill should come out of it. |
WalkthroughThis change introduces a new LED effect mode, "Copy Segment", to the WS2812FX library. It extends the WS2812FX class by adding methods for copying segment data and accessing rendered pixel colors using 2D coordinates. In addition, a new function for performing HSV-based color adjustments has been added along with its declaration in the corresponding header, expanding the library’s color manipulation capabilities. Changes
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wled00/FX_fcn.cpp (1)
1634-1644: Consider caching group length calculation.The
groupLength()call could be cached if this method is called frequently in tight loops, as it may perform multiple calculations internally.uint32_t WS2812FX::getRenderedPixelXY(uint8_t segid, unsigned x, unsigned y) const { + unsigned groupLen = _segments[segid].groupLength(); // cache group length - x *= _segments[segid].groupLength(); // expand to physical pixels - y *= _segments[segid].groupLength(); // expand to physical pixels + x *= groupLen; // expand to physical pixels + y *= groupLen; // expand to physical pixels if (x >= _segments[segid].width() || y >= _segments[segid].height()) return 0; uint32_t offset = _segments[segid].is2D() ? 0 : _segments[segid].offset; return strip.getPixelColorXY(_segments[segid].start + offset + x, _segments[segid].startY + y); }wled00/FX.cpp (1)
151-165: Document 2D to 1D copy behavior.The comment indicates that 2D to 1D copy only uses the first row/column, but this important limitation should be more prominently documented in the effect metadata.
Consider updating the effect metadata to clearly indicate this limitation:
-static const char _data_FX_MODE_COPY[] PROGMEM = "Copy Segment@,Color shift,Lighten,Brighten,ID,,Axis(2D),Overlay;;;12;ix=0,c1=0,c2=0,c3=0"; +static const char _data_FX_MODE_COPY[] PROGMEM = "Copy Segment@,Color shift,Lighten,Brighten,ID,,Axis(2D),Overlay;Note: 2D to 1D copy only uses first row/column;;12;ix=0,c1=0,c2=0,c3=0";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/FX.cpp(3 hunks)wled00/FX.h(2 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/colors.cpp(1 hunks)wled00/fcn_declare.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (lolin_s2_mini)
🔇 Additional comments (9)
wled00/fcn_declare.h (1)
171-171: LGTM! Function declaration is well-optimized.The function declaration uses appropriate attributes and parameter types for optimal performance.
wled00/FX.h (2)
212-213: LGTM! Effect mode is correctly positioned.The new effect mode is added at an appropriate position, replacing a previously commented-out mode.
937-937: Function placement is appropriate.The function is correctly placed in the
WS2812FXclass as suggested in past reviews.wled00/FX_fcn.cpp (2)
1634-1644: LGTM! The implementation looks correct and well-documented.The new
getRenderedPixelXY()method properly:
- Handles coordinate expansion for grouped pixels
- Validates bounds before accessing pixels
- Sets offset to 0 for 2D segments as documented
- Returns black (0) for out-of-range pixels
1634-1644:✅ Verification successful
Verify usage in Copy Segment effect.
Let's confirm this method is used correctly in the copy segment effect implementation.
🏁 Script executed:
#!/bin/bash # Search for usage of getRenderedPixelXY in effects rg "getRenderedPixelXY" -A 5Length of output: 2175
Usage Verified: Copy Segment FX is Correctly Utilizing
getRenderedPixelXYThe search results confirm that the method is used as expected in the Copy Segment effect within both single-axis and transposed (flipped) contexts. The boundary checks and coordinate adjustments are consistent with the intended behavior for 1D and 2D segments.
- FX.cpp: The function is invoked with both standard and swapped coordinate parameters to handle different segment configurations.
- FX.h & FX_fcn.cpp: The declaration and implementation correctly adjust pixel indices by group length and validate boundaries.
No changes are required in this part of the code.
wled00/FX.cpp (4)
133-138: Use API methods for segment access.Based on past discussions, while direct access to segment properties is more performant, the codebase prefers using API methods for better maintainability.
Apply this diff to use API methods consistently:
- uint32_t sourceid = SEGMENT.custom3; + uint32_t sourceid = SEGMENT.getCustom3();
141-151: Verify handling of different segment sizes in 1D copy.The current implementation copies up to the target segment's virtual length, but there's no explicit handling when source and target segments have different sizes.
Please verify:
- What happens when source is longer than target?
- What happens when target is longer than source?
- Should we add bounds checking to prevent potential buffer overflows?
169-169: LGTM: Effect metadata is well-structured.The effect metadata provides clear parameter descriptions and appropriate default values.
10202-10202: LGTM: Effect registration follows established pattern.The new effect is properly registered with its implementation and metadata.
|
ready to merge from my side. It will need adjustments once segment layering is added. |
|
Might be worth waiting for layering. Once that's in, "copy" becomes truly trival - it could point right at the source FX segment buffer. |
There was a problem hiding this comment.
I am not very fond of "segment copy" idea as it brings more challenges that we want to take on IMO.
What I do like is the color adjustment function added in this PR, though I would embed it into CHSV32 class (and perhaps RGBW32).
And as @willmmiles pointed out my new segment blending modifications could make segment copying trivial (just copy one buffer to another) if exact copy is desired. However, I'm guessing no-one will stop at that.
| */ | ||
| uint32_t adjust_color(uint32_t rgb, uint32_t hueShift, uint32_t lighten, uint32_t brighten) { | ||
| if(rgb == 0 | hueShift + lighten + brighten == 0) return rgb; // black or no change | ||
| CHSV32 hsv; |
There was a problem hiding this comment.
Since CHSV32 is custom HSV class, I would suggest to incorporate constructor from RGBW32 and uint32_t which should hide explicit calls to HSV conversion.
Added an FX that copies the selected source segment:
note: 1D to 2D and vice versa is not supported
Summary by CodeRabbit