-
Notifications
You must be signed in to change notification settings - Fork 70
described matrix channelOrder in more detail #4133
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the naming here isn't terribly clear. "Per ___" isn't how I would describe either of these alternatives. To me, "per" implies quantity, not order. Also, this feature is describing channels already, so it sounds like it's using "channel" to mean both "DMX channel" and "color channel" at the same time.
We've got 714 cases of perPixel
in the database, and 0 cases of perChannel
. It wouldn't be the first time I saw an OFL feature which looked useless, but I wonder what the reasoning was for this particular one.
I have no objection to this documentation clarification.
Co-authored-by: Ken Harris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it makes sense to document this more thoroughly.
* `"perPixel"`: For the above [matrix structure](#matrix-structure) example, this results in | ||
- `["Red Inner ring", "Green Inner ring", "Blue Inner ring"]` | ||
- `["Red Middle ring", "Green Middle ring", "Blue Middle ring"]` | ||
- `["Red Outer ring", "Green Outer ring", "Blue Outer ring"]` | ||
* `"perChannel"`: For the above [matrix structure](#matrix-structure) example, this results in | ||
- `["Red Inner ring", "Red Middle ring", "Red Outer ring"]` | ||
- `["Green Inner ring", "Green Middle ring", "Green Outer ring"]` | ||
- `["Blue Inner ring", "Blue Middle ring", "Blue Outer ring"]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, it always results in a flat list, so maybe better replace it with this?
* `"perPixel"`: For the above [matrix structure](#matrix-structure) example, this results in | |
- `["Red Inner ring", "Green Inner ring", "Blue Inner ring"]` | |
- `["Red Middle ring", "Green Middle ring", "Blue Middle ring"]` | |
- `["Red Outer ring", "Green Outer ring", "Blue Outer ring"]` | |
* `"perChannel"`: For the above [matrix structure](#matrix-structure) example, this results in | |
- `["Red Inner ring", "Red Middle ring", "Red Outer ring"]` | |
- `["Green Inner ring", "Green Middle ring", "Green Outer ring"]` | |
- `["Blue Inner ring", "Blue Middle ring", "Blue Outer ring"]` | |
* `"perPixel"`: For the above [matrix structure](#matrix-structure) example, this results in | |
- `"Red Inner ring"` | |
- `"Green Inner ring"` | |
- `"Blue Inner ring"` | |
- `"Red Middle ring"` | |
- `"Green Middle ring"` | |
- `"Blue Middle ring"` | |
- `"Red Outer ring"` | |
- `"Green Outer ring"` | |
- `"Blue Outer ring"` | |
* `"perChannel"`: For the above [matrix structure](#matrix-structure) example, this results in | |
- `"Red Inner ring"` | |
- `"Red Middle ring"` | |
- `"Red Outer ring"` | |
- `"Green Inner ring"` | |
- `"Green Middle ring"` | |
- `"Green Outer ring"` | |
- `"Blue Inner ring"` | |
- `"Blue Middle ring"` | |
- `"Blue Outer ring"` |
However, this is quite long. Better suggestions?
The reasoning was that there might be a fixture with this channel order out there and we wanted the syntax to be future-proof and readable even in that case.
Do you have better naming suggestions? (Should be a separate issue/MR of course.) |
I suppose so, but there are an infinite number of possible features. To my mind, the benefit of a common interface is that it doesn't support every possible hypothetical feature. It supports the common features -- which means implementers only need to implement those features. (There are actual features, which lots of fixtures use today, that I want to use, and which OFL doesn't yet support -- like color beyond RGB. I'm totally fine with accepting a documentation clarification here, or whatever else you want to do with it, but I'm not going to spend any more time on this myself because there are a grand total of 0 fixtures which depend on it.)
I'd hate to change the interface (schema) at this point, for no benefit, but if we were starting from scratch today, I might suggest something like:
Of course, this is all academic until someone can find a pixel array anywhere which has a different channel ordering. How much effort do we want to put into being completely generic, for a feature which nobody has yet found a use for? Changing the schema today would force all software using OFL to be updated, and it wouldn't make anyone's lighting software any better. |
Based on the documentation, it was not clear to me, what the channelOrder does exactly. I found some more information here and tried to describe it a little bit more in detail in the docs:
open-fixture-library/tests/fixture-valid.js
Line 789 in 715e90c
However, I'm not sure, whether it should be the other way round. Based on the code, it should look like this, but based on the naming, I think it should be the other way round.
I found no fixture already using
perChannel
currently.