Added V-Mapper:Z for flipped orientation panels chains #1014

marcmerlin · 2020-03-21T22:58:15Z

This allows using every other panel in a vertical chain, upside down
so as to be able to use the shortest HUB75 cables that come with
panels.
Instead of --led-pixel-mapper=V-Mapper use --led-pixel-mapper=V-Mapper:Z

pull upstream

…into hzeller-master

This allows using every other panel in a vertical chain, upside down so as to be able to use the shortest HUB75 cables that come with panels. Instead of --led-pixel-mapper=V-Mapper use --led-pixel-mapper=V-Mapper:Z

marcmerlin · 2020-03-21T23:00:16Z

Vmapper

Vmapper:Z

marcmerlin · 2020-03-21T23:01:59Z

My apologies for the git history, normally I'd blow my tree and start over fresh from yours to avoid all these merges with no file changes but CL entries, but I can't do that because I still have 2 outstanding pull requests that would get lost if I did that.

Make Henner happy :) (also show pixel count of 2 kinds of panels)

hzeller

The code looks like it evolved 'until wit worked' :) Could you reformulate it so that it is a bit more understandable what is happening ?

lib/pixel-mapper.cc

hzeller · 2020-03-22T01:11:49Z

lib/pixel-mapper.cc

@@ -250,11 +258,29 @@ class VerticalMapper : public PixelMapper {
                                  int *matrix_x, int *matrix_y) const {
    int panel_width  = matrix_width  / chain_;
    int panel_height = matrix_height / parallel_;
+
    *matrix_x = (x % panel_width) +  int(y/panel_height)* panel_width;
    *matrix_y = (y % panel_height) + int(x/panel_width) * panel_height;


Wouldn't it be easier and more readable to figure out if we need flipping first, and then do the one or the other ?

It is very hard to follow to first do the assignment, and then additional modifications later.

So start with an expression that figures out if we should do flipping

const bool needs_flipping = z_ && y / panel_height % 2 == 0; // or something like that.

And then either some

if (needs_flipping) { *matrix_ = ... // } else { *matrix_x = (x % panel_width) + needs_flipping ? ... : int(y/panel_height)* panel_width; // ... }

or, maybe even more readable

*matrix_x = (x % panel_width) + needs_flipping ? ... : ...

So, I'm a bit torn. I'll be honest that I had a hard time figuring out what U-mapper was doing, code that was doing smart things with virtually no comments :)
There are 2 issues maybe in conflict:

efficiency

readability / easy to follow the algorithm flow

For #1: I'll wager that the transform table is computed once at startup and that loosing maybe a millisecond (just made that up) to a few double assigns, is not a prime concern.

For #2:
The function does 3 things
a) normal Vmapper
b) Z Vmapper on a panel that isn't flipped
c) Z Vmapper on a panel that is flipped

I wrote the codeflow to make it very clear which one of a b or c is happening.
By the time you make single line more complex assignments that do a b and/or c at the same time, it's pretty darn hard to figure out what's going on later.

I kind of like the code the way it is because it's pretty simple to follow.
The other part that would be confusing is that x_panel_offset_cnt is computed on the transposed value of X, after it's been mapped to the virtual array (i.e. the first assignment already happened). Sure, you can do smart math that would work before the assignment, but it seemed harder to write, and as a result even harder to re-read.

Keep in mind that we're not top SWEs, so keeping it a bit easier to read, is a plus IMO.
What do you think?

Yes, efficientcy we don't care, it only happens once.

But readability. Multiple assignments to things require you to keep track of it when reading. You do and undo things, while all we need to choose is if we need flipping or not, and formulate the expression accordingly.

What happens now is

do one thing.

oh wait, we are actually doing the other thing .. maybe.

we actually need to do the other thing and overwrite which we did first

This is really hard to follow.
So this is why I suggest to change this to

do we need to do the other thing ?

Yep: do other thing

No: do one thing.

And once you've done this, you'll notice, that the ?: version of the same is even more readable.

Mmmh, so please don't take this as me trying to argue with you :)
The way I wrote it/meant it was:
a) transform from Hmapping to Vmapping
b) if V:Z, see if the resulting panel requires inverting
c) if so, deconstruct the X coordinate to figure out if it ends up on an odd or even panel. If it's an even panel, invert X and Y

a) needs to be done regardless, it's not a) or c), nor does c) undo a)
Combining a and c in one step can be done, but it definitely felt harder to write, and likely not understandable anymore when read.
That said, keep in mind that you're talking to a programmer who isn't as smart as you are (actually I probably barely meet the definition of programmer, and it sure isn't my job description unless you compare with me with java factory factory folks :) ).

If you do see an obvious way to rewrite this in a short and yet still readable way, would you be ok merging this and modifying it the way you had in mind?
(I'll be happy to test your patch before you submit)

(and yes, calculating from the already transposed value is particularly mind-bending)

I disagree that a) needs to be done regardless. We either do a) or we do the 'flipped a'. Right now you have to take the one a) and have to do gymnastics to then flip it.

I don't try to make it more complicated or 'smart', I actually suggest this to make it more simple that even I would understand it (right now, it twists my brain).

We can merge it and I can have a look later.

"and yes, calculating from the already transposed value is particularly mind-bending" => you'll hate me, I found it easier to vizualize after the transposition than before :)
"We can merge it and I can have a look later." =>
that would be great. I am curious to see how it can be best written.

lib/pixel-mapper.cc

hzeller#1014

marcmerlin · 2020-03-22T02:22:45Z

Thanks for the review. It feels like being at work :)

"The code looks like it evolved 'until wit worked' :)" => not really, I wrote it like this the first time, but maybe you mean that first the original transpose calculation happens, and then if there is a need to flip pixels, it does that afterwards.
I wrote more details in the review reply on what my thinking was with a/b/c

lib/pixel-mapper.cc

examples-api-use/README.md

marcmerlin · 2020-03-22T04:27:39Z

If you prefer this picture or would like both, I can replace or put both.
Plus: no power wires, so it's easier to show the Z and the panel orienation
Minus: doesn't show how power is distributed, given that you can't use the rails.

hzeller · 2020-03-22T04:46:16Z

Given that we want to illustrate the way the zig-zag wiring goes, this is a totally fine picture, no need to worry about power-connection (img/chained-64x64 doesn't have power connectors either).

Make sure to scale it down (~1024 wide ?) so that it is sufficiently small - because it will be part of the github repo that people check out on their Pi with possibly a slow connection.

If you want to see how the final rendered markdown looks like while editing it, I can recommend the grip tool.

marcmerlin · 2020-03-22T04:57:48Z

Sure, done.
Actually you can see the rendered version here
https://github.com/marcmerlin/rpi-rgb-led-matrix/blob/Vmapper_Z/examples-api-use/README.md
but thanks for the grip link, didn't know about it. Normally I edit README.md files directly in the github interface which can render it online before you commit.
I also replaced the picture with one that was downsized as you requested.

hzeller · 2020-03-22T05:00:42Z

Cool, how can you render a markdown in github ?

hzeller · 2020-03-22T05:12:46Z

(edit, I mean)

marcmerlin · 2020-03-22T05:19:42Z

go to https://github.com/hzeller/rpi-rgb-led-matrix/blob/master/README.md
click edit
click preview changes

hzeller · 2020-03-22T05:29:01Z

Thanks, merged!

hzeller · 2020-03-22T05:41:37Z

I've simplified the expressions to be just single assignment ( 66fc98d ). This improves readability tremendously for me as it is now clear what happens and why.

Can you verify with your panel that it indeed behaves the same ?

marcmerlin added 7 commits March 17, 2020 20:49

Merge pull request #1 from hzeller/master

1c51f89

pull upstream

Added V-mapper.

5a10b55

Merge branch 'master' of https://github.com/hzeller/rpi-rgb-led-matrix …

d79903c

…into hzeller-master

Merge branch 'hzeller-master'

d6c8ffb

Added V-Mapper:Z for flipped orientation panels chains.

51a4b03

This allows using every other panel in a vertical chain, upside down so as to be able to use the shortest HUB75 cables that come with panels. Instead of --led-pixel-mapper=V-Mapper use --led-pixel-mapper=V-Mapper:Z

Removed debugging in Vmapper:Z

28e12fb

VmapperZ: fixed debugging removal.

b6f6dbd

marcmerlin mentioned this pull request Mar 21, 2020

No support for panels stacked up high instead of wide? #1003

Closed

marcmerlin added 2 commits March 21, 2020 16:25

Added Vmapper:Z documentation.

f4b9eec

Orient panels the same way in pixel-mapper.cc.

308a2b6

Make Henner happy :) (also show pixel count of 2 kinds of panels)

hzeller requested changes Mar 22, 2020

View reviewed changes

Vmapper: cleanups#1 after review (bool, whitespace...)

af718b0

hzeller#1014

Collapse calculations for *matrix_x/y.

6df947b

hzeller reviewed Mar 22, 2020

View reviewed changes

lib/pixel-mapper.cc Outdated Show resolved Hide resolved

hzeller reviewed Mar 22, 2020

View reviewed changes

examples-api-use/README.md Outdated Show resolved Hide resolved

marcmerlin added 2 commits March 21, 2020 20:34

V-mapper: Collapse 2 levels of ifs into one.

9869469

Added link to internal copy of Vmapper wiring picture.

7ad630d

V-Mapper:Z: Better picture for wiring illustration

60ae39d

hzeller merged commit 7bcd89d into hzeller:master Mar 22, 2020

btownshend mentioned this pull request May 13, 2022

Another flicker issue #483

Closed

marcmerlin mentioned this pull request Jun 11, 2024

Vmapper:Z requires physical re-mapping and rewiring of panels when switching from odd to even number in height #1663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added V-Mapper:Z for flipped orientation panels chains #1014

Added V-Mapper:Z for flipped orientation panels chains #1014

marcmerlin commented Mar 21, 2020

marcmerlin commented Mar 21, 2020 •

edited

Loading

marcmerlin commented Mar 21, 2020

hzeller left a comment

hzeller Mar 22, 2020

marcmerlin Mar 22, 2020

hzeller Mar 22, 2020

marcmerlin Mar 22, 2020 •

edited

Loading

hzeller Mar 22, 2020

hzeller Mar 22, 2020

marcmerlin Mar 22, 2020

marcmerlin commented Mar 22, 2020 •

edited

Loading

marcmerlin commented Mar 22, 2020

hzeller commented Mar 22, 2020

marcmerlin commented Mar 22, 2020 •

edited

Loading

hzeller commented Mar 22, 2020

hzeller commented Mar 22, 2020

marcmerlin commented Mar 22, 2020

hzeller commented Mar 22, 2020

hzeller commented Mar 22, 2020

Added V-Mapper:Z for flipped orientation panels chains #1014

Added V-Mapper:Z for flipped orientation panels chains #1014

Conversation

marcmerlin commented Mar 21, 2020

marcmerlin commented Mar 21, 2020 • edited Loading

marcmerlin commented Mar 21, 2020

hzeller left a comment

Choose a reason for hiding this comment

hzeller Mar 22, 2020

Choose a reason for hiding this comment

marcmerlin Mar 22, 2020

Choose a reason for hiding this comment

hzeller Mar 22, 2020

Choose a reason for hiding this comment

marcmerlin Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

hzeller Mar 22, 2020

Choose a reason for hiding this comment

hzeller Mar 22, 2020

Choose a reason for hiding this comment

marcmerlin Mar 22, 2020

Choose a reason for hiding this comment

marcmerlin commented Mar 22, 2020 • edited Loading

marcmerlin commented Mar 22, 2020

hzeller commented Mar 22, 2020

marcmerlin commented Mar 22, 2020 • edited Loading

hzeller commented Mar 22, 2020

hzeller commented Mar 22, 2020

marcmerlin commented Mar 22, 2020

hzeller commented Mar 22, 2020

hzeller commented Mar 22, 2020

marcmerlin commented Mar 21, 2020 •

edited

Loading

marcmerlin Mar 22, 2020 •

edited

Loading

marcmerlin commented Mar 22, 2020 •

edited

Loading

marcmerlin commented Mar 22, 2020 •

edited

Loading