Mosaic icon indicating copy to clipboard operation
Mosaic copied to clipboard

ofxVPParameters

Open d3cod3 opened this issue 4 years ago • 32 comments

d3cod3 avatar Apr 04 '20 17:04 d3cod3

The purpose of ofxVPParameters is to speed up and simplify the creation of nodes by wrapping common variable/parameter behaviour in a templated class.
It will prevent the creation of redondant code and improve readability.

ofxVPParameters will provide common actions on parameters such as : saving+loading data, providing metadata (name, flagChanged,...), serialisation (to string), custom behaviours (sanitisation+validation, timed-updating, manual updating), displaying data, accessing data.
The exact list has to be defined.

Related to #21 : Loading and saving --> ofxVPParameter->saveTo(XMLHandle); Related to #19 : Pin connections --> ofxVPParameter->getPinData(); Related to #18 : Port objects to ImGui --> Object->parameters->draw();

Daandelange avatar Apr 05 '20 09:04 Daandelange

So what do parameters need to do for this first basic implementation ? For now, I have :

BaseParameter

  • A name (with unique name UUID). uniqueNum (ID) could be added to match current IDs.
  • A typed data storage, TypeName value.
  • A GUI method for displaying or editing, drawImGui()
  • Future Proposals : handle data sampling, changed flags, list all existing parameters, visualise parameters over time, etc.

Optional ParamFeatures (currently ofxVPObjectParameter, to be renamed?)

  • XML saving and loading methods for restoring, toXML(), fromXML()

I'm thinking about adding a feature to define inlet/outlet pins which will map to object pins. Maybe something like ofxVPBaseParameter<type, bHasInlet, bHasOutlet> ? Examples :

  • ofxVPBaseParameter<float, true, true> --> A parameter which can be set with an inlet, modified in the gui, and outputs its value to an outlet too.
  • ofxVPBaseParameter<float, false, false> --> A parameter which can only be modified via GUI an inlet, probably used internally in the object.
  • ofxVPObjectParameter<float, true, true> --> A parameter which can be controlled via an inlet, modified in the gui, used internally, and be saved/restored.

AdditionObject would do :

ofxVPBaseParameter<float, true, false> inValue;
ofxVPObjectParameter<float, true, false> numToAdd;
ofxVPBaseParameter<float, false, true> result;
update(){
    result = inValue + numToAdd;
}

What do you think about this inlet/outlet proposal ? How would this match/interfere-with existing nodes' behaviour/implementation ?

Do you have any other suggestions / needs ?

Daandelange avatar Apr 07 '20 10:04 Daandelange

OK, let me tell you what i have right now with params and objects:

1 - simple inlets, no GUI related, just inlets for internal use, NO saving/restore to/from xml

2 - simple outlets, no GUI related, just outputs some value from the object (internal operation or simple bypass from some inlet), NO saving/restore to/from xml

3 - inlets with GUI (mainly int/floats and sliders/textboxes), if the inlet is connected, the GUI control is deactivated and the internal value came from the wire; if is disconnected, the internal value is directly controlled form user input in the GUI. The value controlled from the GUI is always saved in the xml for restore when opening a patch (with the setCustomVar(value,tag) method).

4 - dynamic inlets/outlets, some objects have the capability of create new inlets/outlets from the GUI (osc receiver or osc sender, or timeline), and in a special case creating inlets directly from a script (shader object, loads a glsl shader, and i have hardcoded a way of name textures, float and int vars to have automatically appearing inlets associated plus their GUI). This cases are like type 3 above, plus the dynamic part.

Basically, all the inlets with related GUI, are saved/restored to/from the xml patch file, and the special dynamic ones, also save/restore the specific object inlets/outlets configuration (there can be a shader object with 4 inlets and 1 outlet, and another shader object (same node) with 2 inlets and 1 outlet).

So, from your explanation about ofxVPParameters, i think we are on the right path, maybe i will add, for the BaseParameter:

  • Connected flag (now i'm using inletsConnected[i] and getIsOutletConnected(i))
  • New connection flag (some objects need some reinitialization when a cable disconnect and another reconnect, for example ofTexture or ofPixels cables, when connected the object receiving it call a one time (1 cycle) block of code to allocate properly internal vars); when the cable disconnect, a bool var change his state to re-launch the one time block of code on reconnecting a new cable.

Apart from that, i think your first implementation covers the issue fine.

Tell me if something is not clear about inlets/outlets

d3cod3 avatar Apr 07 '20 15:04 d3cod3

Thanks for your explanations and details, that clarifies a lot. Sounds good, I'll continue implementing this until I have more questions. I'll probably end up adding a minimal Pin class to simplify pin logic too, hoping it won't involve too much changes. What do you think ?

Don't hesitate suggesting more parameter types I can start implementing too. You can indicate more specific objects with common/specific data types.

Daandelange avatar Apr 08 '20 10:04 Daandelange

This is getting complicated. I'm working on some draft code as a proof of concept for Pins + ofxVPBaseParameters. I'm running in a lot of trouble, but learning a lot.
Reporting some findings :

  • We need abstract untemplated (static memory footprint) base classes so we can store and access them trough (fast) iterable containers (std::vector ?)
  • We need templates so we can access specific data types with common methods.
  • Order of inheritance and multiple inheritance causes some issues too.
  • Typed data must be available to specialized template methods.
  • All this must wrap up in a set of solid containers keeping simplicity in mind.
  • We need constant data references, they don't seem to be problematic yet !

I'll post code when I have bare minimum functionality, or report back with some more details.

Daandelange avatar Apr 10 '20 21:04 Daandelange

Ok, I got major progress ! I'll soon have a basic version to propose. With some concrete code, further discussions will be easier.

Btw, found a really useful utility, pre-compiling C++ code and showing how the compiler parses/unfolds the actual code. Good to see what happens under the hood in complex scenarios, seeing what auto is converted into, implicit functions, etc.

Daandelange avatar Apr 15 '20 13:04 Daandelange

I'm trying to port some objects to test specific stuff, and i just find out the Order of inheritance and multiple inheritance problem with the ofxVPParameters code, generating duplicate symbols on compile.

I'll wait for your code ;P

By the way, great tool the https://cppinsights.io/ !!!

d3cod3 avatar Apr 16 '20 13:04 d3cod3

Yet another great tool I found : https://app.diagrams.net/

I've been thinking around, drawing the logic down to get something future-proof. And I'm not completely happy with my current (gist) proposal too, it feels a bit too complicated.

So here's a slightly different proposal. Changes are mainly the pin logic : inlets could be modifiers and are not guaranteed to be kept alive. Whereas outlets are always accessible (if enabled on the parameter). This might be closer to a master/slave model, bringing some kind of variable ownership, simplifying the dataflow logic. The diagram is not finished and it's almost the first time I'm making such diagrams; I hope it's clear enough to get an idea.

Here's an image : Parameters

And the .drawio xml file to open with diagrams.net, if you wish to edit. Parameters.drawio.zip

Daandelange avatar Apr 25 '20 10:04 Daandelange

Yes, i agree, and it's perfectly clear!

About the question about ParamAbstract classes, i think they don't need to be separated, one big class will do the job fine.

Generally speaking, i think the diagram covers the Mosaic pins/params needs beautifully, at least regarding what we talked about till now.

Just a detail about the HasUID class, i think it's a great idea to add an unique identifier to every element, but i have a question, why uniqueName when we have uniqueID? With uniqueID alone we can cover the logic, no?

d3cod3 avatar Apr 27 '20 16:04 d3cod3

Ok, sound good. About HasUID : uniqueID is numeric, probably a faster key type, best to use it for performance. (that's how Mosaic's current UIDs work, right ?) But I like to have "alternative semantic names" too, for human logic. var x = getParam(135); vs var x = getParam('Constant_1');

Object 1 Object 2 Object 3
uniqueName Constant Constant_2 Constant_3
name Constant Constant Constant
uniqueID 0 1 2

What do you think ? Maybe we don't need the (non-unique) name too ?

Daandelange avatar Apr 27 '20 17:04 Daandelange

Yes, it's handy, you're right, it's better with it!

About the non-unique name, i'll leave it too, for GUI labeling issues.

Imagine a patch with 3 "constant" objects, having the same object with the DragFloat GUI with "number", "number_1", "number_2" labels can be confusing in design/interface terms, it would be better to have the same "number" label for all objects, and maybe on rollover showing the unique name?

This goes for the objects name on headers too, maintain the GUI non-unique name and on rollover show the unique name and the unique ID too?

Well, small details anyway, i think it's ok as you design it from the beginning.

d3cod3 avatar Apr 28 '20 07:04 d3cod3

Yeah, small details, the hover idea sounds good. Let's get this implemented !

Daandelange avatar Apr 30 '20 07:04 Daandelange

I've updated the drawing to better reflect our discussions : Parameters_v2

I'm not sure about storing the address of the pin relative to the object (Pin.ID & Pin.objectID) in #28 , so Ive not added them (yet?). Do objects really have to handle pin logic ? Very customised Objects can, but most won't have to, they just instantiate their parameters and use their values. And as parameters inherit HasUID::UniqueID, they have an absolute address, from which you could also control their pins. Objects hold Parameters each of which extends hasPinOutlet, which in turn handles all the pin logic. Eventually we could add API methods to Parameters so that accessing links is easier.

// EXAMPLES
// Check if a pin can connect with int
myObject.parameters[0].links[0].toInlet.acceptsLinkType<int>()
// connect 2 params together
myObject.parameters[0].createNewLinkWith( otherObject.parameters[0].addModifier( new InletPinModifier() ) )

Daandelange avatar May 02 '20 13:05 Daandelange

This is what i'm imagining it, we will have (maybe) three map vars?

map<int,shared_ptr<PatchObject>> objects;

map<int,shared_ptr<Parameter>> parameters;

map<int,shared_ptr<PinLink>> links;

So, when we create a PatchObject, we store a reference to his parameters in the "global" parameters map.

Then, every Parameter (if hasPinOutlet) store a reference to his PinLinks in the links map (we need PinLinks class to inherit HasUID too)

With that, we have the reference to every element on the patch, the objects, the pins/parameters for every object and all the links of every outlet pin, everything accessible from "outside", as of now, where a map is used to manage all the objects of a patch, and it will be a really easy to read code.

In the case of Pin.ID and Pin.objectID, we can sure remove the first one, using HasUID inheritance will give us the access to every single pin on the patch, so it's not needed anymore.

But about objectID, will not be useful for access map elements from outside?

We will have a way of relate links with pins/parameters, because PinLink class get me the reference to the fromOutlet and toInlet and having an objectID var in the HasPin class will give us a reference to the objects, so we could easily cross-search data from every point (a link will give me the pins that will give me the object)

With that it'll be really easy to port all the actual logic to the new design, then, while working on the port, we can always optimize and remove eventually var redundancy.

d3cod3 avatar May 03 '20 17:05 d3cod3

Ok, sounds good for the 3 maps. Indeed, we need global access to Links too. Precision: I'm not drawing sharedptrs to gain space. Most type* will become sharedptrs.

We can have PinLink->toInlet.ObjectID & PinLink->fromOutlet.ObjectID, no big difference and as you say, we'll remaster it later.

So, from another perspective, we have :

  • Objects : Stored & owned in ofxVP (ObjectFactory...), accessible trough API methods..
  • Parameters : Stored & owned in Object.param1, referenced in map<UID,AbstractParam*> Object:: allMyParams; and in static map<UID,AbstractParam*> AbstractParam::AllParams.
  • PinLinks : Stored & owned in HasOutletTyped<T>::links (which also is a Parameter) while they are all being referenced in PinLink::allLinks for global access.

We'll see later if we'll need PinLinkTyped or if we can keep it abstract. Same for the static allParams and allLinks, maybe they'll be better in separate (factory) classes.

Slightly updated schema : Parameters_v3

Daandelange avatar May 04 '20 09:05 Daandelange

Perfect, i think we are really close.

About PinLink, i think we can keep it abstract, letting the Pins/Params to take care of the type, but we'll see what's best later.

And about allParams and allLinks, i agree, it's probably a good idea to have them wrapped in factory (ParamFactory, LinkFactory)

d3cod3 avatar May 04 '20 16:05 d3cod3

Again, an up-to-date version of the diagram : Parameters_v4

Daandelange avatar Jun 06 '20 10:06 Daandelange

Latest update, in Mosaic-Engine-Tester repo, I got the first working link connections 🚀 . See https://github.com/d3cod3/Mosaic-Engine-Tester/commit/c9139727894d10b8c6c3a556d05cfb6e8b6cddf9 . ofxVPParameters

But by using references to link params, editing 1 variable changes the other connected variable, even upstream... which messes with thinking logic (logical flow direction); although it's also interesting to be able to edit the same value from a downstream node parameter. So should we make InletParamModifiers provide a copy of the parent value ? Also, looped connections make it crash, but they can(?) be prevented.

Also, I feel the need of extending the ImGui node api to support something like beginParam(), beginParamInlet, param menu, etc. ? So parameters easily all get a similar look. #17

Daandelange avatar Jul 14 '20 13:07 Daandelange

Amazing! I'm really happy to see it working!

About the first two issues, can we just limit the logical flow direction with the bIsConnected variable for inlets?

if(bIsConnected)
    inletParam.editable = false;
else
    inletParam.editable = true;

So we avoid inverted flow direction and looped connections. Right now is like that, if a node param has inlet and the inlet is disconnected, you can edit it, but when the inlet is connected, the value is overridden from the cable. Obviously this will happen on the params with gui only, all the params without a gui with "editing" capabilities will not have problems.

And about the extending of the ImGui node api, let's do it "surgically", i've changed various stuff, and just yesterday i was testing it on a retina screen, and the design come up completely distorted ( widths, heights and spacings are most of them off ), so we'll need to do all this plus your extensions with precision ( the fixing retina issues is really a pain in the ass!! )

I'm going to finish something today and i'm going to push a commit ( i'll let you know when is done ), so you'll have the last version of imgui_node_canvas

Let's coordinate on the ImGui node extension then, and congrats for the first linking connections!!!

d3cod3 avatar Jul 14 '20 15:07 d3cod3

Ok, just pushed the last commit on refactor-params-and-gui branch

d3cod3 avatar Jul 14 '20 18:07 d3cod3

Ok, I'll checkout the changes.

Is ImGui even retina-compatible ? I've read trough a lot of DPI issues, it can come from the (older) ofxImGui implementation or incomplete native ImGui support, see ocornut imgui/pull/2826.

I'll try to disable editing for connected params. (ImGui internal beta functionality, but it should work). But this won't fix the looping connection crash. The only solution I see (for now) is a "connection path" checker every time new connection is made. Let's keep this for later.

Daandelange avatar Jul 15 '20 07:07 Daandelange

Well, nothing is retina compatible, but basically you just need to detect it ( already working in Mosaic ), and multiply by 2 every design constant ( spacings, borders, etc... ), hacky but it works. A friend is sending me screenshots with the retina issues, so i'll fix them over time.

About the loop, as a suggestion i think that probably we'll need to make a copy of the parent value ( a pointer copy ) for all the params with GUI/inlet, while not needed for the internal params without GUI/inlet.

d3cod3 avatar Jul 15 '20 17:07 d3cod3

I'll digg the ptr copy method and report back here.

Daandelange avatar Jul 19 '20 13:07 Daandelange

It appeared to be an infinite recursive function call, overwhelming the call stack. Fixed by checking the returned outletValue before connecting against its own value. If they are the same --> refuse to connect. (It's a partial solution as this might not work for later : inlets might be disabled or bypassed, making the value check fail, and the crash occur as soon as it's re-enabled.)

Daandelange avatar Jul 22 '20 08:07 Daandelange

I continued to think and experiment with the next ofxVPParameters steps this summer.

  1. There's some obvious code cleaning to do, but that's less of a matter.
  2. I'm rather concerned about how the new linktypes are implemented. Right now, an int can't connect with a float while they both are VP_LINKTYPE_NUMERIC. One solution could be to force all numeric links to use float, and have objects internally convert it to int/whatever, like Mosaic does it now. But what about having the parameter class handle these linktype conversions ? I think that would simplify patching & writing code so much, while being (most)performant.
  3. Synching/Sampling data #23 : would be nice to discuss possible implementations / needs.
  4. Looking into integrating Tracy to make parameter development easier. (optional debug build flag of course)

What are your thoughts on these points ?

Daandelange avatar Sep 14 '20 10:09 Daandelange

Ok, point 2, i think you're right, basic link types conversions ( as int with float or double ) will be better located inside the parameter class.

Tracy for debug looks amazing, i discover it some time ago looking at imgui projects, i think is another good idea ( if not too complicated to integrate )

about point 3, i'll answer on #23

d3cod3 avatar Sep 14 '20 12:09 d3cod3

  1. Ok, investigating. In first instance, I'm thinking about designating a native/primary data-type on outlets. When listeners (inlets) request a non-native/secondary format (when connecting), allocate that data-format (once) and keep the alternative value up-to-date.

  2. Update on Tracy : --> The client is very plug and play. It installs as a submodule and requires C++11 client-side (Mosaic). Include 1 .cpp file and enable a compiler flag. That's it. My network monitor is showing local traffic when Mosaic is running with Tracy. --> The Profiler (to view data) seems to need c++17, which I don't have, will try on my Linux soon.

Daandelange avatar Sep 17 '20 15:09 Daandelange

point 2, nice idea!

about tracy, the same, i compile Mosaic with c++14 on osx, so i'll need to tests the Profiler on linux too.

And the network local traffic? is about Tracy? Mosaic did check something on startup, download the release file from github to check if new releases are available and inform the user ( in the logger )

d3cod3 avatar Sep 17 '20 21:09 d3cod3

Ok, nice to know, I'm using C++11 for Mosaic on osx. I read some report that OF won't compile in C++17 yet. Yes, 100% sure, I use to block that GitHub call and I had to allow some sockets on launch (using an outgoing firewall). Continue Tracy in #33 .

Daandelange avatar Sep 18 '20 10:09 Daandelange

Hey, no much advancement in terms of code, but I'm still thinking about this from time to time, and planning to resume somewhen this winter. Meanwhile, I've been looking around again for eventual existing solutions to make this easier, and I'm having 3 eventual candidates.

  1. DSPatch : Simple Data Flow library maintained by an Canonical engineer, well updated, but weirdly I cannot find any project it's used in...
  2. Magnum.graphics : Looks like a modern oF, with better graphics API support (oF Vulkan dev has been unactive for a while, maybe thinking about something else then the ongoing MoltenVK implementation). Also it seems to have an embedded solution for Plugins (ping #16 ) . It's build on Corrade, which includes [a signal processing system].(https://doc.magnum.graphics/corrade/interconnect.html) that I could hook into ofxVPParams, to simplify its development and use a shared codebase. Would be worth looking into.
  3. libTwo In between both but less active.

I feel like exploring Corrade::Interconnect for powering ofxVpParameters. What do you think of this ?

Looking at Magnum globally, it could save us a lot of development time on hardcore topics. It looks like something similar to OF but it's far from having all its graphic features and extensions (=connectivity), maybe it's worth looking into, how it would interface with OpenFrameworks. I really like everything they say about Magnum.

Daandelange avatar Dec 10 '20 18:12 Daandelange

Magnum+Corrade looks amazing, didn't know it!

Let's take a dive on it and do some testing, i'm open to use it, coupling it with OF or even substitute OF with it, we'll just need to check if and what we'll lose doing so.

d3cod3 avatar Dec 11 '20 12:12 d3cod3