Copy Segment FX

[DedeHai]

Added an FX that copies the selected source segment:

• 1D and 2D supported
• brightness of segment is relative to source segment
• optionally shifts the color hue
• invert, transpose, mirror etc. work
• if source and target 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

[DedeHai]

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.
Two options:
-this is a feature, i.e. leave it up to the user to make the settings identical
-silently copy the 'mirror' and 'transpose' setting from the source segment (without updating the UI)

[willmmiles]

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 .width(), etc. on the source segment instead of .virtualWidth(). This should essentially copy the rendered output, including invert, mirror, transpose, etc. Continue to use virtualWidth() for sizing the destination segment, though, and let the destination segment options be applied as normal.

I don't have a good setup for testing this but I'll give it a go when I can.

[DedeHai]

I did not know about this distinction, that is actually what I was looking for :)
will change and test.

[DedeHai]

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.
I am not sure what the best way to proceed is.
From a user perspective, I would expect that 'copy' means 'do the same as the source segment' meaning also the mirroring and grouping/spacing should be copied.

[willmmiles]

that does not seem to work as intended.

We are being foiled by getPixelColor()'s internal processing, I think, it's still doing some logical to physical pixel conversion. Possibly we'll need to do our own iteration using the raw segment coordinates (start, stop, spacing, group).

[willmmiles]

This gets copy and pasted below - probably it wants to be a little static function instead.

[DedeHai]

indeed, can you help out? I do not know what the best way to implement it is. just adding a function?

[willmmiles]

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.

[DedeHai]

oh, I though you meant some fancy C++ macro magic ;)
but good point, will add this to color utils

[softhack007]

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.

[DedeHai]

I tested just copying the source settings by doing
SEGMENT.mirror = strip._segments[sourceid].mirror; for example. that works, even UI gets updated (though not consistently)
by doing this I found a nasty bug.
if both x and y mirroring are set in a segment, the flipping is done incorrectly. the error is in line 214 of FX_2Dfcn.cpp (sorry, don't know how to reference that here directly). what I have is a 16x16 matrix, two segments, one x= 0-7, one x=8-15, both have y=0-15. if I set x and y mirroring in the right hand segment (the on going x=8-15) one quarter gets transferred to the first segment. that is independend of my copy function. here is what it looks like (line is the split between segments)

I lack the underlaying logic to fix it.

[blazoncek]

@DedeHai looks like start offsets are missing.

        strip.setPixelColorXY(start + width() - xX - 1, startY + height() - yY - 1, tmpCol);

[DedeHai]

looks like start offsets are missing.

correct, can confirm that adding start fixes it.

[DedeHai]

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.

[willmmiles]

I tested just copying the source settings by doing
SEGMENT.mirror = strip._segments[sourceid].mirror; for example. that works, even UI gets updated (though not consistently)

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.

[DedeHai]

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.

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).
offset is (currently) not copied and seems to work fine.

what I did not test at all yet: what happens if global buffer is disabled.

[willmmiles]

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).

I don't think that's true - your original logic with virtualLength() and getPixelColor() calls operates on an unmirrored/unreversed/ungrouped "virtual pixel" index space, so that the FX code itself doesn't need to know about those flags. getPixelColor knows how to "undo" all of those features, since they're performed in setPixelColor.

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.

[DedeHai]

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).
also if transpose was active, not using virtual did not result in a correct copy.
Anyway: the source buffer will contain the 'mirrored' FX data, so it does not matter whether the user sets the destination to 'unmirrored', 'reverse' will do the same thing (flips one axis).
I also played a little with manual setting of spacing and grouping (which also requires to use virtualLength() to be used) and found that it actually works but did not work if I automatically assigned the value.
I am not sure adding a new 'getPixelColor' will be necessary but I am curios on what that would look like.
I also have a solution for the 'copy source settings': since this is an FX: it has user selectable checkboxes. I added one already but the code to copy the settings is not done yet.
I also found that using fadeToBlackBy() is really slow (and maybe buggy?). It worked in 2D but when I switched to 1D with just 128+128 LEDs the FPS would (sometimes) drop to 8FPS (!). That went away without the fadeToBlackBy() so I am pretty sure that was the cause (I have no explanation on why it would be so slow, and only in 1D).
I also may have fixed the need for that shift_hue() function:
there is no need to distinguish the code between 1D and 2D, a 1D strip can be treated as a nx1 matrix (unless that is really slow for large strips? need to test some more. For WLED_DISABLE_2D there can be an ifdef with different code.

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).

[blazoncek]

we'd need a new "getRenderedPixelColor()"

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.
I'm not fond of copying segments, but that does not matter. I would suggest you rely on regular getPixelColor() and operate from destination segment. If you will exceed coordinates from source segment you'll get black so you'll copy only "active" pixels from source (however many there are).
"active" are what virtualLength, virtualWidth and virtualHeight are returning. Those are the pixels that effect operated on.
mirroring, reversing and transposing are post rendering activities, hidden from effect functions.

[willmmiles]

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).
Anyway: the source buffer will contain the 'mirrored' FX data, so it does not matter whether the user sets the destination to 'unmirrored', 'reverse' will do the same thing (flips one axis).

This is the expected behavior of getPixelColor - it's not accessing the source buffer directly, it's accessing the "virtual pixel" space, described by virtualLength etc. (what I called "FX" space earlier) . Mirrors/reverse/etc. do not apply to getPixelColor, you can think of them as being applied later.

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'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:

Segment.setPixelColor()/Segment.getPixelColor() accept indexes in to "virtual pixel" space for each segment. These are the pixel indexes that are used by FX.

[ 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
[ 1, 2, 3, 4, 4, 3, 2 1]

grouping=2, mirror, reverse - length x2 for mirror, x2 again for grouping
[ 4, 4, 3, 3, 2, 2, 1, 1, 1, 1, 2, 2, 3, 3, 4, 4]

"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 strip.setPixelColor() and strip.getPixelColor(). (Xes are pixel indexes that are not part of the example segment.)

start=3, spacing = 1, offset = 0, mirror
[ X, X, X, 1, X, 2, X, 3, X, 4, X, 4, X, 3, X, 2, X, 1, X]

start=0, spacing = 1, offset = 1, mirror
[ X, 1, X, 2, X, 3, X, 4, X, 4, X, 3, X, 2, X, 1]

start=0, spacing = 1, offset = 0, grouping=2, mirror, reverse - length x2 for mirror, x2 again for grouping
[ 4, 4, X, 3, 3, X, 2, 2, X, 1, 1, X, 1, 1, X, 2, 2, X, 3, 3, X, 4, 4, X]

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.

there is no need to distinguish the code between 1D and 2D, a 1D strip can be treated as a nx1 matrix (unless that is really slow for large strips? need to test some more. For WLED_DISABLE_2D there can be an ifdef with different code.

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; setPixelColor(index) knows how to do the conversion using the standard 2D expansions.

[DedeHai]

Ah get it, if you access outside of 'virtual space' it does not return a valid color, hence getting black.
I agree that operating in 'FX space' is the safe way to do it and all the safety checks are already in place.
Not modifying the properties is probably also a valid point, especially if it does not get properly mapped back to the UI (which is what I saw, sometimes it would, sometimes it would not) it will result in incorrect presets.
a workaround would be to have that checkmark I mentioned and setting the properties just for rendering, then reverting them back. would that work or is data to the UI being processed in parallel (i.e. not just between frames)?

edit:
addendum: I tested to use 1D and 2D (i.e the XY versions) of getPixelColor and setPixelColor on a 2D strip and see no difference in speed (looked at FPS). There may be a tiny advantage on the 1D version but it is negligible.

[softhack007]

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.

🤔 I think @willmmiles has described it quite accurate, maybe just a bit over-compilcated.

• segments are the "drawing canvas" --> reverse, transpose, mirror, grouping and spacing are only applied at the moment when going from segment.setPixelColorXY() to strip.setPixelColorXY() .

• in the 1D case, SEGLEN (aka Segment::virtualLength()) gives you the size of the active canvas.

• In 2D, use Segment::virtualWidth() and Segment::virtualHeight().

• strip is an intermediate layer, which basically performs mapping (ledmaps, gapmaps, 2D->1D) when going to down to busses.setPixelColor() (busses don't have any XY)

• busses manages double buffering, and forwards pixels to the proper bus driver instance

As you want to copy effects ( = segments), I thinks its enough to copy what segment.getPixelColorXY provides - the raw pixels without grouping and spacing.

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 -
you'll want segment::virtualWidth() and segment::virtualHeight() for 2D segments, not segment::width() and segment::height().

[softhack007]

PS: if you need a few nasty testcases

• copy from a 2D segment running "Juggle" with "pinwheel" mapping, into another 2D segment with a different layout
• copy from a 2D segment running "Pacifica" with pixels mapping, into a small 1D segment with grouping=2

[softhack007]

edit:
addendum: I tested to use 1D and 2D (i.e the XY versions) of getPixelColor and setPixelColor on a 2D strip and see no difference in speed (looked at FPS). There may be a tiny advantage on the 1D version but it is negligible

I'd say its better to always use the "right" versions - 1D or 2D - if you are on segment level. The "not XY" getPixelColor and setPixelColor versions will perform 1D->2D mapping, which is maybe not what you want.

Actually this function is a good example how to handle 1D and 2D

WLED/wled00/FX_fcn.cpp

Lines 1101 to 1111 in b9080f9

	void Segment::fadeToBlackBy(uint8_t fadeBy) {
	if (!isActive() || fadeBy == 0) return; // optimization - no scaling to apply
	const int cols = is2D() ? virtualWidth() : virtualLength();
	const int rows = virtualHeight(); // will be 1 for 1D
	for (int y = 0; y < rows; y++) for (int x = 0; x < cols; x++) {
	if (is2D()) setPixelColorXY(x, y, color_fade(getPixelColorXY(x,y), 255-fadeBy));
	else setPixelColor(x, color_fade(getPixelColor(x), 255-fadeBy));
	}
	}

(just except for the if (is2D()) inside the for loop, which costs performance)

[willmmiles]

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.

[DedeHai]

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).
When settings in the target are changed, sometimes (with unlucky timing) they get reverted but that shows in the UI. There may be cases where it does not show in the UI and when changing the FX to some other, it may show wrong (unless the UI also sends out a segment setting update when FX is changed).
while not perfect, this is a 'workaround' that would enable users to change the source segment settings and have all targets do the same (I imagin someone having many slave segments, it would be really annoying to have to manually change each one every time).
Making it 'perfect' would require a semaphore or some other rework of code, which I personally think is way overkill for the edge cases.
As for copying the source including the mirror part, that could be done by a new 'getRenderedPixelXY' function or it could be directly handled in the FX (I am not sure which one is the better approach).

Edit:
I thought about this on my way to work and realized that if a getRenderedPixelXY() function is implemented, there is no need to copy the source settings (except maybe for grouping and spacing, those could still be copied the way I mentioned above or completely left to the user. So I am now in favour of using a getRenderedPixelXY(), question is just how robust does it have to be (and how much code we want to spend on edge cases).

[willmmiles]

I thought about this on my way to work and realized that if a getRenderedPixelXY() function is implemented, there is no need to copy the source settings (except maybe for grouping and spacing, those could still be copied the way I mentioned above or completely left to the user. So I am now in favour of using a getRenderedPixelXY(), question is just how robust does it have to be (and how much code we want to spend on edge cases).

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?

[DedeHai]

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.
One thing that took me quite a while to pinpoint: 2D was all messed up until I figured out that seg.offset is not zero in my 2D config although that is not even an option to set in the UI (it is 6 for some reason). Before I commit my fixes: how is this to be handled? could do an if case (if there is any reason it is not zero) or find out why it is non zero and enforce it there?

[willmmiles]

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 getRenderedPixelColor functions, to mirror the 1D and 2D getPixelColor; or (b) make sure that Segment::offset is always set to zero for 2D segments, instead of carrying around an old value. I lean towards the latter - I don't like unused values being copied around in the config - but the more experienced WLED devs might have a different opinion.

[DedeHai]

latest commit now seems to handle all test cases (even the nasty ones). There are a few things that do not work properly though:

• 2D to 1D copies only one row. since there is not really any good way to map it that all users will like (or makes sense) I left it at that
• when enabling source grouping / spacing + mirroring, the target is not a 1:1 copy, even if settings are adjusted
• when using spacing, 'in between' pixels are not always set to black (may be an underlying bug/feature as that also happens in the source segment if spacing is enabled and afterwards 'reverse' or 'mirror')

edit:
forgot to change the FX parameter string to 1D so the UI enables 2D expansion. fixed it locally, will be in the next commit.

edit2:
tested spacing and grouping with mirror again and it is correct if target is set to same settings (apart from the not cleared pixels issue)

[willmmiles]

when using spacing, 'in between' pixels are not always set to black (may be an underlying bug/feature as that also happens in the source segment if spacing is enabled and afterwards 'reverse' or 'mirror')

The pixels omitted by spacing are not part of the segment at all -- they're expected to belong to some other segment. Spacing+Offset lets you set up "interleaved" segments with different FX, eg with (spacing = 1, seg1: offset=0, seg2: offset=1) you can arrange [1 2 1 2 1 2 1 2] where you can set different FX on 1s and 2s.

[willmmiles]

Following on by that: I intentionally omitted source grouping and spacing in the getRenderedPixel() sketch. Grouping could perhaps be included, but spacing doesn't make sense -- the pixels skipped over by "spacing" are (meant to be) part of some other segment; including pixels from Segment 2 in a copy of Segment 1 seems wrong to me.

In the end it was a judgement call - mirror, reverse, transpose all feel to me like they're "part of" the segment FX rendering; while grouping and spacing feel more like they're part of the rendered FX->output strip mapping. YMMV, though.

[DedeHai]

I have it mostly done now (if there are no objections to latest changes).
What remains though: where should the uint32_t getRenderedPixelXY() be put? I think it should go in FX_fcn.cpp but should it be part of the WS2812FX class or just a standalone function? Or just keep it in FX.h?

[willmmiles]

I have it mostly done now (if there are no objections to latest changes). What remains though: where should the uint32_t getRenderedPixelXY() be put? I think it should go in FX_fcn.cpp but should it be part of the WS2812FX class or just a standalone function? Or just keep it in FX.h?

If we're committed to this approach, I think it should be defined as a member of struct Segment, and implemented in FX_fcn.cpp beside getPixelColor -- as any future changes to getPixelColor or getPixelColorXY might also need to be applied to getRenderedPixelXY.

[blazoncek]

This will get a pixel from another segment if you are overlapping segments and use spacing.

[willmmiles]

The groupLength() call above should include the spacing?

[blazoncek]

Correct. Needed to check sources.
Still it will not handle transposed segments correctly. I mean, logically.

[willmmiles]

It's supposed to copy the data post-transposition. But I think you're correct that there's a bug here - if transpose is set, we need to swap virtualWidth() and virtualHeight() in the copy fx function.

[DedeHai]

I tested transposed source segment (among many other things) it copies right. But maybe I missed a case, there are soo many variants.

[willmmiles]

I think it might run in to trouble if the source segment x/y sizes aren't multiples of each other. Eg, source = 3x8 transpose.

[DedeHai]

So far I could not find a case where transpose does not work, tested various sizes (3x16, 5x16, 3x7) and its always correct. If segments are overlapping, the getRenderedPixelXY() will of course read whatever has been written to the buffer, so if a segment overwrites data, the copy function will still copy that. This can not be avoided and depends on order of segments (as does overlapping segments in general)