ofxGui: add setMin() and setMax() to ofxSliderGroup

[woutgg]

This PR implements half of the 'ofParameter <=> slider' coupling, as discussed in #6378.

[roymacdonald]

can you add this to the ofxColorSlider_ class, as it behaves in a similar way and it is actually on the same file?

[woutgg]

Done - also for ofxRectangleSlider, plus they now also have getMin/getMax to match ofxSlider's interface.

With the color slider it is not really useful yet, as the UI widget does not deal with min/max properly:

• Channel sliders maps the parameter range to its full visual range, which makes the slider position meaningless. Improving this would require an extension of ofxSlider, which is probably beyond the scope of this PR.
• The color circle does not (and cannot) take the channel ranges into account. Applying those afterwards in changeValue() works fine, but I did not commit that as I'm not sure if it improves things.

[roymacdonald]

hi. this looks good. Although most of the setMin or setMax use non-const references. this should all be const.
now these are
void setMin(ValueType & min);
but should be
void setMin(const ValueType & min);
same applies for setMax
Can you update this please?

[woutgg]

Changed, that's better indeed. I was a bit thrown off by the equivalent signatures in ofxSlider, which should probably also be const references.

[roymacdonald]

awesome. thanks. Sure, the ofxSlider equivalente should be const too. That needs fixing.

[ofTheo]

@woutgg or @roymacdonald
would you be able to share in this discussion thread a minimal example for testing this functionality?

Thanks!
Theo

[roymacdonald]

I don't have any example code for this. sorry

[woutgg]

I'll try to bake an example from the test code I used.

By the way I considered writing unit tests for at least part of this PR, but a quick look in the tests directory showed that it is almost unused for addons and there is not much information about the role of automated tests either. Is this intentional or this a work in progress?

[arturoc]

unit tests are always welcome, there's not many for addons if at all cause nobody have written them not cause we don't want to have them. tests run with every new PR or commit and let us know if anything has broken with a new addition

[woutgg]

See below for a small example to inspect the changes in this PR. The last commit contains some unit tests as well.

It also shows that setting min/max values for the color widget is not really useful yet because the color circle does not adhere to them. One way to address this would be to clamp the values being set (this line), but that is hardly intuitive.

ofApp.h:

#pragma once

#include "ofMain.h"
#include "ofxGui.h"

class ofApp : public ofBaseApp {
public:
    ofApp();
    void setup();
    void update();
    void draw();
    void limitToggleChanged(bool& state);

    ofParameter<glm::vec2> vec2Param;
    ofxVec2Slider vec2Sl;

    ofParameter<ofRectangle> rectParam;
    ofxRectangleSlider rectSl;

    ofParameter<ofColor> clrParam;
    ofxColorSlider clrSl;

    ofxToggle limitToggle;
    ofxPanel gui;
};

ofApp.cpp:

#include "ofApp.h"

ofApp::ofApp()
: vec2Param("vec2", {50, 50}), rectParam("rectangle", {100, 100, 250, 250}),
  clrParam("color", ofColor(255, 255, 255, 255))
{}

void ofApp::setup() {
    limitToggle.addListener(this, &ofApp::limitToggleChanged);

    gui.setup();
    gui.add(vec2Sl.setup(vec2Param));
    gui.add(rectSl.setup(rectParam));
    gui.add(clrSl.setup(clrParam));
    gui.add(limitToggle.setup("Limit slider min/max", false));
}

void ofApp::update() {}

void ofApp::draw() {
    const auto vec2Info = "vec2 value: (" + ofToString(vec2Param.get()) + ")\n" +
        "     slider min/max: (" + ofToString(vec2Sl.getMin()) + ") / (" + ofToString(vec2Sl.getMax()) + ")\n" +
        "     param min/max: (" + ofToString(vec2Param.getMin()) + ") / (" + ofToString(vec2Param.getMax()) + ")";
    const auto rectInfo = "rect value: (" + ofToString(rectParam.get()) + ")\n" +
        "     slider min/max: (" + ofToString(rectSl.getMin()) + ") / (" + ofToString(rectSl.getMax()) + ")\n" +
        "     param min/max: (" + ofToString(rectParam.getMin()) + ") / (" + ofToString(rectParam.getMax()) + ")";
    const auto clrInfo = " clr value: (" + ofToString(clrParam.get()) + ")\n" +
        "     slider min/max: (" + ofToString(clrSl.getMin()) + ") / (" + ofToString(clrSl.getMax()) + ")\n" +
        "     param min/max: (" + ofToString(clrParam.getMin()) + ") / (" + ofToString(clrParam.getMax()) + ")";

    int line = 1;
    ofDrawBitmapString(vec2Info, 300, 16 * line);
    line += 3;
    ofDrawBitmapString(rectInfo, 300, 16 * line);
    line += 3;
    ofDrawBitmapString(clrInfo, 300, 16 * line);

    gui.draw();
}

void ofApp::limitToggleChanged(bool& state) {
    if (state) {  // Set narrow limits
        vec2Sl.setMin(glm::vec2(10, 10));
        vec2Sl.setMax(glm::vec2(100, 100));
        rectSl.setMin(ofRectangle(10, 10, 10, 10));
        rectSl.setMax(ofRectangle(100, 100, 100, 100));
        clrSl.setMin(ofColor(120, 127, 127, 0));
        clrSl.setMax(ofColor(120, 255, 255, 255));

    } else {  // Set wide limits
        vec2Sl.setMin(glm::vec2(0, 0));
        vec2Sl.setMax(glm::vec2(1000, 1000));
        rectSl.setMin(ofRectangle(0, 0, 0, 0));
        rectSl.setMax(ofRectangle(1000, 1000, 1000, 1000));
        clrSl.setMin(ofColor(0, 0, 0, 0));
        clrSl.setMax(ofColor(255, 255, 255, 255));
    }
}

[woutgg]

Ah, VS project files are required for the tests to work. Attempting to setup a VM now so I can create those.

[roymacdonald]

Thanks!
Does it make any sense to set the min and max for the color picker? I feel that it does not. At most I would restrict the alpha channel.
I'll take a look at the test code you've posted later.
All the best

[woutgg]

Does it make any sense to set the min and max for the color picker? I feel that it does not. At most I would restrict the alpha channel.

I agree, it doesn't really make sense at least without modifying and/or extending the widget itself, which is probably not worth the effort without a concrete use case.
So yes, I'll change the code to only limit the alpha channel then.

Regarding updating the slider limits through their parameter, should I create an issue to address that, or do you prefer leaving it for now?

[roymacdonald]

I agree, it doesn't really make sense at least without modifying and/or extending the widget itself, which is probably not worth the effort without a concrete use case.
So yes, I'll change the code to only limit the alpha channel then.
I think that is should stay as it is. It behaves in the same way as the rest of the group sliders so anyone can infer how to use it.

Regarding updating the slider limits through their parameter, should I create an issue to address that, or do you prefer leaving it for now? I think it should be addressed in this PR as well as it is related to the same issue.