RobinSchmidt / RS-MET

Codebase for RS-MET products (Robin Schmidt's Music Engineering Tools)
Other
56 stars 6 forks source link

Can we finally admit I'm not going to be creating my own library? #178

Closed elanhickler closed 6 years ago

elanhickler commented 6 years ago

Robin, let's work together on making your GUI / plugin creation library easier to use.

I'm getting fed up with trying to create easier to use classes and functions that are frankenstein versions of your classes.

Tired of switching from my personal branch to your branch and all the fuss. I can't get the phasescope to work with my frankenstein classes, no idea where it's going wrong and I really don't want to debug it. I'd rather start over with my frankenstein classes (and make it more compatible with yours, or better yet, suggest improvements so I don't have to make these big ugly classes). Mostly, I hate all the repetitive coding and needless independency between parameters and widgets. They can stay independent, but there should be facilities to make them dependent, because that's the case 90% of the time. I'm talking about

this:

image

plus this:

image

It's so much repetitive code and difficult to maintain (brightness is typed 4 times for example (not counting description)). With my frankenstein classes (i.e. myparams) I type "Brightness" once and it takes care of the rest.

Oh here's one idea I just got.

/* 
- convenience function to create widgets from parameter 
- also assigns parameter
- sets description field
- note: in this case I have saved my parameter objects as members in the scopeModule class so I
don't have to access them by string, just a personal preference that allows you to use intellisence
to get the desired parameter.
*/
addWidget(s = sliderBrightness = RSlider(scope->parBrightness));

Actually, I can create my own convenience functions to do this. I'm going to abandon myparams class in favor of convenience functions that work better with your stuff. But there may be a few cases where convenience functions won't help much.

I can suggest ways of making this easier. So..... let's take this slow, let me suggest improvements one at a time and let me stop using my frankenstein classes. I'll be getting ideas for this as I re-make the PhaseScope (I'm doing this so I can add some controls).

RobinSchmidt commented 6 years ago

Mostly, I hate all the repetitive coding and needless independency between parameters and widgets. They can stay independent, but there should be facilities to make them dependent, because that's the case 90% of the time.

well, code repetition sucks, i fully agree with that but i disagree with the notion that this separation is useless. you may not really need it 90% of the time, but you can't simply ignore the other 10%. take the equalizer, for example: each band gets its own set of frequency/gain/bandwidth parameters, but there is only one slider for frequency etc - which get re-assigned a new parameter, when the user selects another band. just one example. and sometimes, you may want the slider to display a different text than the parameter's name. ...but for cases, where all these texts match up, yes, a convenience function may be helpful (the description is actually not meant to be the same text anyway). ...not to mention that the user may not even open a gui - in which case you probably want to avoid to create all the heavyweight gui objects that just clog the memory

RobinSchmidt commented 6 years ago

you have probably heard of the model-view-controller (MVC) pattern: https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller ...although, i personally tend to favor this one: https://en.wikipedia.org/wiki/Presentation%E2%80%93abstraction%E2%80%93control the "abstraction" is the core dsp code in this case, the "control" is the parameter/audio-module infastructure and the presentation is the gui. the advatage of the latter over the former is that the core business logic (our dsp code) is independent from the gui framework (i don't want rapt/rosic classes to call any juce(dependent) code) - all communication is mediated by the "control" part

elanhickler commented 6 years ago

If I learn how to make modules during runtime (have a + butto for LFOs for example) I'll have a much better idea of how to handle things. Didn't understand the factory method at first glance.

RobinSchmidt commented 6 years ago

i think, it should be easy to learn that. you just create them with "new" and add them to the array of child-modules (via addChildAudioModule). the process is no different than what you would otherwise do in your constructor

elanhickler commented 6 years ago

thats fine, but then you also need a new GUI, right? So you need to somehow create ONE function that does stuff on the GUI side and on the not-GUI side... which seems difficult due to locks and separation of widgets and parameters... or do you create two functions and, to create a module [and editor], you have to call a new module function in two different places?

RobinSchmidt commented 6 years ago

yes, you would have to create a new AudioModuleEditor, too - if the overall gui is open. but you already know how to create and add child-editors in the constructor of your AudioModuleEditor subclass. you would need a function that does that same thing on runtime. in your AudioModule, you could have an "addModule" function, that just adds the module itself, and one in the editor that calls the former and also creates the editor....hmm....but what if the user loads a preset with a different configuration of child-modules? i need to look that up

RobinSchmidt commented 6 years ago

ahh...in toolchain, i have made a callback/notification system:

void ToolChainEditor::audioModuleWasAdded(ToolChain *chain,
  AudioModule *module, int index)
{
  ScopedLock scopedLock(*lock);
  updateEditorArray();
  updateActiveEditor();
  updateSelectorArray();
}

void ToolChainEditor::audioModuleWillBeDeleted(ToolChain *chain,
  AudioModule *module, int index)
{
  ScopedLock scopedLock(*lock);
  deleteEditor(index);
  scheduleSelectorArrayUpdate();
}

void ToolChainEditor::audioModuleWasReplaced(ToolChain *chain,
  AudioModule *oldModule, AudioModule *newModule, int index)
{
  ScopedLock scopedLock(*lock);
  deleteEditor(index);
}

the editor gets notified when a new child-module was added to the edited ToolChainAudioModule - then it can take appropriate action inside the callback (add a child-editor for the added module, for example)

elanhickler commented 6 years ago

do we really need a new here?

When would it be bad to simply declare the widget member as NOT a pointer?

RobinSchmidt commented 6 years ago

...maybe this notification system should be factored out into the AudioModuleEditor baseclass

RobinSchmidt commented 6 years ago

When would it be bad to simply declare the widget member as NOT a pointer?

hmmm....i think, when you need to create widgets at runtime....which probably isn't often. if i remember correctly, the use of pointers here is actually a remnant of the old juce version which required the "Components" to be pointers

elanhickler commented 6 years ago

but...

even that doesn't seem to require pointers.

why can't you simply declare a vector of not-pointer widgets and add not-pointer widgets to that? then loop through your widgets for whatever, drawing, painting, etc.

RobinSchmidt commented 6 years ago

hmm - vector of what class? the widget baseclass? i think, then you can't assign subclass objects ...or can you? i'm not sure. but i think, in any case, an assignment like

Widget myNewWidget;
myWidgetVector.push_back(myNewWidget);

implies that the widget object must be copied...so would probably have to deal with lots of temporary objects...but maybe that can be avoided using move-constructors or something. don't know - but that all seems more complicated to me than just using a plain old pointer

elanhickler commented 6 years ago

what? this seems rudimentary, you're doing it already with vectors of widget pointers. You assign any kind of widget to that array.

elanhickler commented 6 years ago

anyway, i guess I better stick to pointers then.

elanhickler commented 6 years ago

well, I really don't see the need for pointers here, so I removed them:

image

The problem comes when you assume that an object manager class needs to delete it, but it wasn't declared with new. uggghhhhh, I'm wasting time suggesting improvements that will only get rid of the new/delete, not really much point in changing it if your library is already working.

RobinSchmidt commented 6 years ago

The problem comes when you assume that an object manager class needs to delete it, but it wasn't declared with new

yeah...i understand and i actually agree. it would just break sooo much of my existing code, if i now change that in the whole library. maybe i can somehow allow both, pointers and non-pointers. i must think about it.

elanhickler commented 6 years ago

I guess the only logical (not horribly error prone) way forward is to allow for both, that would be the first step, but... there's more important things to worry about.

elanhickler commented 6 years ago

first improvement I am going to suggest, give me an "add string value" for vector string

jura_Parameter.cpp

void Parameter::addStringValue(const vector<String>& valueToAdd)
{
  ScopedPointerLock spl(mutex);
  for (const auto & s : valueToAdd)
    stringValues.addIfNotAlreadyThere(s);
  mapper->setRange(0.0, jmax(double(stringValues.size()-1), DBL_MIN));
}

or I can do pull request for this

elanhickler commented 6 years ago

AudioModuleEditor is a ChangeListener, but AudioModule is not a ChangeBroadcaster, very confusing!

Edit: Wait a minute, I think it is more correct for parameters to be change broadcasters.

Edit: Then you would give an std::function member for parameter for change broadcaster so you could do this at the ChangeLister:

void changeListenerCallback(ChangeBroadcaster* source)
{
    bool needsRepaint;

    source->callChangeListenerCallback(&needsRepaint);      

    if (needsRepaint)
        repaint();
}

something like that, maybe there's a more elegant way to trigger repaint.

elanhickler commented 6 years ago

confusing and annoying: no universal "set text" or "set name"

you have setSliderName, setButtonText, setName... fghdfghdf!!!!! some of them work, like, setName I think works for Sliders, but setName does not setButtonText.

this was another reason for my frankenstein class.

elanhickler commented 6 years ago

another big annoyince: lack of std::function for string conversion

RobinSchmidt commented 6 years ago

Wait a minute, I think it is more correct for parameters to be change broadcasters.

parameters don't need to be change broadcasters - at least not in the sense of being a subclass of juce::ChangeBroadcaster - because they have their own broadcast/listener system (class Parameter itself takes the role of the ChangeBroadcaster and ParameterObserver the role of the ChangeListener)

RobinSchmidt commented 6 years ago

you have setSliderName, setButtonText, setName... fghdfghdf!!!!! some of them work, like, setName I think works for Sliders

yes, some naming conventions could certainly be made more consistent

another big annoyince: lack of std::function for string conversion

hmm..so you would want that to be std::function instead of a function pointer? why? do you think, you will need functors for string conversion - or one-off lambda functions?

RobinSchmidt commented 6 years ago

AudioModuleEditor is a ChangeListener in order to update itself after a user has loaded a new preset:

void AudioModuleEditor::changeListenerCallback(juce::ChangeBroadcaster *objectThatHasChanged)
{
  if( objectThatHasChanged == stateWidgetSet )
    updateWidgetsAccordingToState();
}

...hmmm...maybe it would be more elegant if the AudioModule itself would send out a callback for this.

elanhickler commented 6 years ago

I use a handful of one-offs right now, because I like to go the extra mile with it. For example, if a parameter is at its minimum, i like to give a "minimum text" like "frozen" or "bypassed" or "off" stuff like that.

RobinSchmidt commented 6 years ago

ok, yes - i can see that this can be useful

elanhickler commented 6 years ago

old thread, closing, no longer relevant.