Skip to content

Display gaps in peek, fix segment overflow bug #5105

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+5 −4
Open

Display gaps in peek, fix segment overflow bug #5105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e53a1c
display gaps in peek and fix segment overflow bug
DedeHai Nov 22, 2025
1d4d9f3
move mapping to gPc(), add function to read buffer without mapping
DedeHai Nov 22, 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
2 changes: 1 addition & 1 deletion wled00/FX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ uint16_t mode_copy_segment(void) {
} else { // 1D source, source can be expanded into 2D
for (unsigned i = 0; i < SEGMENT.vLength(); i++) {
if(SEGMENT.check2) {
sourcecolor = strip.getPixelColor(i + sourcesegment.start); // read from global buffer (reads the last rendered frame)
sourcecolor = strip.getPixelColorNoMap(i + sourcesegment.start); // read from global buffer (reads the last rendered frame)
Copy link
Collaborator

@blazoncek blazoncek Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry to open this old wound, but this is so wrong IMO.
None of the effects should depend on strip pixels.

Get segment's pixels using sourcesegment.getPixelColor() instead.

Copy link
Collaborator Author

@DedeHai DedeHai Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

there is currently no other way to get a "full copy" of a segment and I see the copy FX more of a strip function disguised as an FX for UI convenience. Is your concern based a technical (future) difficulty or rather architectural aesthetic?

Copy link
Member

@willmmiles willmmiles Nov 22, 2025

Choose a reason for hiding this comment

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

In a perfect world, I'd ask for all the options - I can see each of them being useful in different contexts.

  1. Re-blend the internal "segment contents" from the source segment, essentially just re-rendering it with different pixel coordinates. Implementation would be like calling blendSegment() with the source segment, but overriding some output properties (start coord, grouping, spacing).
  2. Blend the "rendered but not blended" output of the source segment, ie. respecting source segment grouping/skipping. Implementation would be like calling blendSegment() for the source segment in to a temporary buffer with start coords set to 0, then calling blendSegment() to blend the contents of that buffer to main buffer.
  3. Copy (blit) a block of previously-rendered pixels from one location in the pixel output buffer to another. Useful to capture a blended output.

Copy link
Collaborator Author

@DedeHai DedeHai Nov 22, 2025

Choose a reason for hiding this comment

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

@willmmiles if I understand this right, we currently have options 1 and 3 implemented in copy FX. Option2 might be doable if it were implemented as a usermod, did not think it through. I agree that the ideal implementation would allow all three options. The question I am reading from @blazoncek concern is that it may pollute code in a way that hinders future progress as it should stay backwards compatible.
I dont want to stop discussion about this, but this discussion should be moved to discord or a separate issue as this is bikeshedding.

}
else {
sourcesegment.setDrawDimensions(); // set to source segment dimensions
Expand Down
3 changes: 2 additions & 1 deletion wled00/FX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,8 @@ class WS2812FX {
};

unsigned long now, timebase;
inline uint32_t getPixelColor(unsigned n) const { return (n < getLengthTotal()) ? _pixels[n] : 0; } // returns color of pixel n
inline uint32_t getPixelColor(unsigned n) const { return (getMappedPixelIndex(n) < getLengthTotal()) ? _pixels[n] : 0; } // returns color of pixel n, black if out of (mapped) bounds
inline uint32_t getPixelColorNoMap(unsigned n) const { return (n < getLengthTotal()) ? _pixels[n] : 0; } // ignores mapping table
inline uint32_t getLastShow() const { return _lastShow; } // returns millis() timestamp of last strip.show() call

const char *getModeData(unsigned id = 0) const { return (id && id < _modeCount) ? _modeData[id] : PSTR("Solid"); }
Expand Down
2 changes: 1 addition & 1 deletion wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, ui
return;
}
if (i1 < Segment::maxWidth || (i1 >= Segment::maxWidth*Segment::maxHeight && i1 < strip.getLengthTotal())) start = i1; // Segment::maxWidth equals strip.getLengthTotal() for 1D
stop = i2 > Segment::maxWidth*Segment::maxHeight ? MIN(i2,strip.getLengthTotal()) : constrain(i2, 1, Segment::maxWidth);
stop = i2 > Segment::maxWidth*Segment::maxHeight && i1 >= Segment::maxWidth*Segment::maxHeight ? MIN(i2,strip.getLengthTotal()) : constrain(i2, 1, Segment::maxWidth); // check for 2D trailing strip
startY = 0;
stopY = 1;
#ifndef WLED_DISABLE_2D
Expand Down
2 changes: 1 addition & 1 deletion wled00/ws.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ bool sendLiveLedsWs(uint32_t wsClient)
#ifndef WLED_DISABLE_2D
if (strip.isMatrix && n>1 && (i/Segment::maxWidth)%n) i += Segment::maxWidth * (n-1);
#endif
uint32_t c = strip.getPixelColor(i);
uint32_t c = strip.getPixelColor(i); // note: LEDs mapped outside of valid range are set to black
uint8_t r = R(c);
uint8_t g = G(c);
uint8_t b = B(c);
Expand Down