refactor(pixeloverlay): remove unused colorOrder parameter and update related logic for channel handling

Author: OnlineDynamic

src/channeloutput/PixelString.cpp

@@ -897,20 +897,31 @@ void PixelString::AutoCreateOverlayModels(const std::vector<PixelString*>& strin
             uint32_t strings = vs.size();
             uint32_t strands = 1;
             uint32_t maxChan = 0;
+            uint32_t minChannelsPerNode = UINT32_MAX;
             int8_t rn = -1;
             for (auto& a : vs) {
                 if (a) {
                     startChannel = std::min(startChannel, (uint32_t)a->startChannel);
                     maxChan = std::max(maxChan, (uint32_t)a->startChannel + (a->pixelCount * a->channelsPerNode() / (a->groupCount ? a->groupCount : 1)));
                     channelsPerNode = std::max(channelsPerNode, (uint32_t)a->channelsPerNode());
+                    minChannelsPerNode = std::min(minChannelsPerNode, (uint32_t)a->channelsPerNode());
                     rn = std::max(rn, a->receiverNum);
                 } else {
                     --strings;
                 }
             }
-            // Channel data is always in R,G,B[,W] order; the output driver remaps to the
-            // hardware wire order.  The model's ColorOrder describes the channel data.
-            std::string colorOrder = (channelsPerNode >= 4) ? "RGBW" : "RGB";
+            if (strings && minChannelsPerNode != channelsPerNode) {
+                // One model cannot describe two node widths.  Taking the wider of
+                // them mis-addresses every pixel on the narrower strings, and the
+                // channel order reported for the model is wrong for them too, so
+                // say so rather than creating a silently broken model.
+                LogWarn(VB_CHANNELOUT, "Overlay model %s spans strings of %u and %u channels per node; using %u - pixels on the narrower strings will not line up\n",
+                        name.c_str(), minChannelsPerNode, channelsPerNode, channelsPerNode);
+            }
+            // The model's channel order is derived from channelsPerNode by
+            // PixelOverlayModel - the virtual string's own colour order describes
+            // the wire, which is applied later by prepareOutput(), and must not be
+            // copied onto the model or it would be applied twice.
             int32_t channelCount = maxChan - startChannel;
 
             if (name.find("Tree") != std::string::npos || name.find("TREE") != std::string::npos || name.find("tree") != std::string::npos || name.find("Vert") != std::string::npos || name.find("vert") != std::string::npos) {
@@ -921,7 +932,7 @@ void PixelString::AutoCreateOverlayModels(const std::vector<PixelString*>& strin
             if ((channelCount > 0) && (rn == -1)) {
                 autoModelNames.push_back(name);
                 PixelOverlayManager::INSTANCE.addAutoOverlayModel(name, startChannel, channelCount, channelsPerNode, orientation,
-                                                                  startLocation, strings, strands, colorOrder);
+                                                                  startLocation, strings, strands);
             }
         }
     }

src/overlays/PixelOverlay.cpp

@@ -1888,6 +1888,9 @@ void PixelOverlayManager::RegisterCommands() {
     CommandManager::INSTANCE.addCategorizedCommand(new ApplyEffectOverlayCommand(this), "Pixel Overlay", 1);
 }
 
+// colorOrder is accepted for source compatibility with existing channel output
+// plugins but is no longer used: a model addresses fppd's channel data, which is
+// always R,G,B[,W], so PixelOverlayModel derives the order from the node width.
 void PixelOverlayManager::addAutoOverlayModel(const std::string& name,
                                               uint32_t startChannel, uint32_t channelCount, uint32_t channelPerNode,
                                               const std::string& orientation, const std::string& startLocation,
@@ -1902,7 +1905,6 @@ void PixelOverlayManager::addAutoOverlayModel(const std::string& name,
     val["Orientation"] = orientation;
     val["StartCorner"] = startLocation;
     val["ChannelCountPerNode"] = channelPerNode;
-    val["ColorOrder"] = colorOrder;
     val["autoCreated"] = true;
 
     addModel(val);

src/overlays/PixelOverlay.h

@@ -58,6 +58,8 @@ class PixelOverlayManager {
     bool isAutoCreatePixelOverlayModels() const {
         return autoCreate;
     }
+    // colorOrder is ignored - the model's channel order follows from channelPerNode.
+    // The parameter is retained so plugins built against older headers still link.
     void addAutoOverlayModel(const std::string& name,
                              uint32_t startChannel, uint32_t channelCount, uint32_t channelPerNode,
                              const std::string& orientation, const std::string& startLocation,

src/overlays/PixelOverlayModel.cpp

@@ -56,6 +56,24 @@
 #include "PixelOverlayEffects.h"
 #include "PixelOverlayModel.h"
 
+// The channel order a model reads and writes.  This is the order of fppd's
+// channel data, not of the wire: setPixelValue() stores r, then g, then b,
+// then w into consecutive channels, and the output driver is what permutes
+// those into the hardware's colour order.  It therefore follows entirely from
+// how many channels a node occupies.
+static const char* ChannelDataOrder(int channelsPerNode) {
+    switch (channelsPerNode) {
+    case 1:
+        return "W";
+    case 2:
+        return "RG";
+    case 3:
+        return "RGB";
+    default:
+        return "RGBW";
+    }
+}
+
 static uint8_t* createChannelDataMemory(const std::string& dataName, uint32_t size) {
     mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
     int f = shm_open(dataName.c_str(), O_RDWR | O_CREAT, mode);
@@ -122,11 +140,12 @@ PixelOverlayModel::PixelOverlayModel(const Json::Value& c) :
     }
     bytesPerPixel = (channelsPerNode >= 4) ? 4 : 3;
 
-    // Color order is consumed by the web UI (testing.php) via the model's JSON
-    // config; ensure it always has a value so the API exposes it.
-    if (!config.isMember("ColorOrder")) {
-        config["ColorOrder"] = "RGB";
-    }
+    // A model addresses fppd's channel data, which is always R,G,B[,W] - the
+    // output driver is what applies the hardware wire order.  So the order the
+    // model sees is fully determined by the node width and is derived here
+    // rather than stored: any ColorOrder in the saved config is overwritten.
+    // Exported read-only through the API so the UI can show it.
+    config["ColorOrder"] = ChannelDataOrder(channelsPerNode);
 
     if (config["Type"].asString() != "Channel") {
         if (config.isMember("PixelSize") && config["PixelSize"].asInt() > 1) {            

www/pixeloverlaymodels.php

@@ -220,20 +220,30 @@ function GetOrientationInput(currentValue, attr) {
             return str;
         }
 
-        function GetColorOrderSelect(currentValue, attr) {
-            var options = [
-                "RGB", "RBG", "GRB", "GBR", "BRG", "BGR",
-                "RGBW", "RBGW", "GRBW", "GBRW", "BRGW", "BGRW"
-            ];
-            if (!currentValue) currentValue = "RGB";
-            var str = "<select class='colorOrder'" + attr + ">";
+        // 1, 3 and 4 are the node widths FPP's outputs actually produce - a plain
+        // 1-4 spinner also offers 2, which no output can drive and which silently
+        // discards the blue channel.  A width already saved in the config is kept
+        // as an extra choice rather than quietly rewritten.
+        function GetChannelsPerNodeInput(currentValue, attr) {
+            var options = [1, 3, 4];
+            var current = parseInt(currentValue);
+            if (isNaN(current) || current < 1) {
+                current = 3;
+            }
+            if (options.indexOf(current) == -1) {
+                options.push(current);
+                options.sort(function (a, b) { return a - b; });
+            }
+
+            var str = "<select class='form-select cpn'" + attr + ">";
             for (var i = 0; i < options.length; i++) {
                 str += "<option value='" + options[i] + "'";
-                if (currentValue == options[i])
+                if (current == options[i])
                     str += " selected";
                 str += ">" + options[i] + "</option>";
             }
             str += "</select>";
+
             return str;
         }
 
@@ -334,12 +344,10 @@ function PopulateChannelMemMapTable(data) {
 
                 switch (model.Type) {
                     case "Channel":
-                        var colorOrder = model.ColorOrder || "RGB";
                         postr += "<td><span class='hidden type'>" + model.Type + "</span>" + model.Type + "</td>" +
                             "<td><input class='start' type='text' size='6' maxlength='6' value='" + model.StartChannel + "'" + attr + "></td>" +
                             "<td><input class='cnt' type='text' size='6' maxlength='6' value='" + model.ChannelCount + "'" + attr + "></td>" +
-                            "<td><input class='cpn' type='number' min='1' max='4' value='" + ChannelCountPerNode + "'" + attr + "></td>" +
-                            "<td>" + GetColorOrderSelect(colorOrder, attr) + "</td>" +
+                            "<td>" + GetChannelsPerNodeInput(ChannelCountPerNode, attr) + "</td>" +
                             "<td style=\"white-space: nowrap;\">" + GetOrientationInput(model.Orientation + orientationDetails(model), attr) + "</td>";
                         if (model.Orientation != "custom") {
                             postr += "<td>" + GetStartingCornerInput(model.StartCorner, attr) + "</td>" +
@@ -350,13 +358,21 @@ function PopulateChannelMemMapTable(data) {
                                 "<td><input class='strcnt' type='hidden' value='" + model.StringCount + "'></td>" +
                                 "<td><input class='strands' type='hidden' value='" + model.StrandsPerString + "'></td>";
                         }
-                        var xlchecked = "";
-                        if (model.xLights) {
-                            xlchecked = " checked";
-                        }
-                        postr += "<td><input class='xlights' type='checkbox'" + xlchecked + " disabled>";
-                        if (model.xLights) {
-                            postr += " <i class='fas fa-eye' style='cursor:pointer; color:#5bc0de;' title='Preview model layout' onclick='showModelPreview(" + JSON.stringify(model.Name) + ")'></i>";
+                        // This column is headed "xLights Generated" on the editable
+                        // table and "Submodels" on the auto created one.  The checkbox
+                        // is meaningless for an auto created model - it is never an
+                        // xLights import - but the submodel expander still applies, so
+                        // the cell is always emitted and only its contents differ.
+                        postr += "<td>";
+                        if (!model.autoCreated) {
+                            var xlchecked = "";
+                            if (model.xLights) {
+                                xlchecked = " checked";
+                            }
+                            postr += "<input class='xlights' type='checkbox'" + xlchecked + " disabled>";
+                            if (model.xLights) {
+                                postr += " <i class='fas fa-eye' style='cursor:pointer; color:#5bc0de;' title='Preview model layout' onclick='showModelPreview(" + JSON.stringify(model.Name) + ")'></i>";
+                            }
                         }
                         var subModels = getSubModels(model.Name);
                         if (subModels && subModels.length > 0) {
@@ -579,8 +595,7 @@ function SetChannelMemMaps() {
                             model.StartCorner = $this.find("select.corner").val();
                             model.StringCount = parseInt($this.find("input.strcnt").val());
                             model.StrandsPerString = parseInt($this.find("input.strands").val());
-                            model.ChannelCountPerNode = parseInt($this.find("input.cpn").val());
-                            model.ColorOrder = $this.find("select.colorOrder").val();
+                            model.ChannelCountPerNode = parseInt($this.find(".cpn").val());
                             model.xLights = $this.find("input.xlights").is(':checked');
 
                             if ((model.StartChannel > 0) &&
@@ -656,8 +671,7 @@ function AddNewChannelModel() {
                 "<td><span class='hidden type'>Channel</span>Channel</td>" +
                 "<td><input class='start' type='text' size='6' maxlength='6' value='1'></td>" +
                 "<td><input class='cnt' type='text' size='6' maxlength='6' value='150'></td>" +
-                "<td><input class='cpn' type='number' min='1' max='4' value='3'></td>" +
-                "<td>" + GetColorOrderSelect("RGB", "") + "</td>" +
+                "<td>" + GetChannelsPerNodeInput(3, "") + "</td>" +
                 "<td>" + GetOrientationInput("", "") + "</td>" +
                 "<td>" + GetStartingCornerInput('') + "</td>" +
                 "<td><input class='strcnt' type='text' size='3' maxlength='3' value='1'></td>" +
@@ -987,38 +1001,6 @@ function pageSpecific_PageLoad_PostDOMLoad_ActionsSetup() {
                     DisableButtonClass('btnDelete');
                 }
             });
-
-            // Keep Ch./Node consistent with the selected Color Order: RGBW orders are
-            // 4 channels per node, RGB orders 3.  Leave 1/2-channel models alone when
-            // a plain RGB order is selected since color order doesn't apply to them.
-            $('#channelMemMaps').on('change', 'select.colorOrder', function () {
-                var cpnInput = $(this).closest('tr').find('input.cpn');
-                if (cpnInput.length == 0) {
-                    return;
-                }
-                var isRGBW = $(this).val().indexOf('W') != -1;
-                if (isRGBW) {
-                    cpnInput.val(4);
-                } else if (parseInt(cpnInput.val()) == 4) {
-                    cpnInput.val(3);
-                }
-            });
-
-            // And the reverse: dropping Ch./Node below 4 on an RGBW model reverts the
-            // Color Order to a 3-channel one, raising it to 4 selects RGBW.
-            $('#channelMemMaps').on('change', 'input.cpn', function () {
-                var orderSelect = $(this).closest('tr').find('select.colorOrder');
-                if (orderSelect.length == 0) {
-                    return;
-                }
-                var order = orderSelect.val() || 'RGB';
-                var isRGBW = order.indexOf('W') != -1;
-                if (parseInt($(this).val()) == 4 && !isRGBW) {
-                    orderSelect.val(order + 'W');
-                } else if (parseInt($(this).val()) != 4 && isRGBW) {
-                    orderSelect.val(order.replace('W', ''));
-                }
-            });
         }
 
     </script>
@@ -1073,8 +1055,7 @@ class='buttons btn-success ml-1'>
                                     <th><span title='Type'>Type</span></th>
                                     <th><span title='Start Channel'>Start Ch.</span></th>
                                     <th><span title='Channel Count'>Ch. Count</span></th>
-                                    <th><span title='Chan Per Node'>Ch./Node</span></th>
-                                    <th><span title='Color Order (RGB, RGBW, etc.)'>Color Order</span></th>
+                                    <th><span title='Channels each node occupies. 1, 3 and 4 are the widths FPP&apos;s outputs produce.'>Ch./Node</span></th>
                                     <th><span title='String Orientation'>Orientation</span></th>
                                     <th><span title='Starting Corner'>Start Corner</span></th>
                                     <th><span title='Number of Strings or Width of FB/X11/Sub-Model'>Strings</span></th>
@@ -1108,14 +1089,14 @@ class='buttons btn-success ml-1'>
                                     <th><span title='Type'>Type</span></th>
                                     <th><span title='Start Channel'>Start Ch.</span></th>
                                     <th><span title='Channel Count'>Ch. Count</span></th>
-                                    <th><span title='Chan Per Node'>Ch./Node</span></th>
-                                    <th><span title='Color Order (RGB, RGBW, etc.)'>Color Order</span></th>
+                                    <th><span title='Channels each node occupies. 1, 3 and 4 are the widths FPP&apos;s outputs produce.'>Ch./Node</span></th>
                                     <th><span title='String Orientation'>Orientation</span></th>
                                     <th><span title='Starting Corner'>Start Corner</span></th>
                                     <th><span title='Number of Strings or Width of FB/X11/Sub-Model'>Strings</span></th>
                                     <th><span
                                             title='Number of Strands Per String or Height of FB/X11/Sub-Model'>Strands</span>
                                     </th>
+                                    <th><span title='xLights submodels defined for this model'>Submodels</span></th>
                                     <th><span title='Running Effect'>Running Effect</span></th>
                                 </tr>
                             </thead>