d3cod3 / Mosaic

Mosaic, an openFrameworks based Visual Patching Creative-Coding Platform
https://mosaic.d3cod3.org
Other
421 stars 23 forks source link

ofxVPParameters #22

Open d3cod3 opened 4 years ago

Daandelange commented 4 years ago

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 commented 4 years ago

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

BaseParameter

Optional ParamFeatures (currently ofxVPObjectParameter, to be renamed?)


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 :

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 ?

d3cod3 commented 4 years ago

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:

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

Tell me if something is not clear about inlets/outlets

Daandelange commented 4 years ago

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 commented 4 years ago

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 :

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

Daandelange commented 4 years ago

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.

d3cod3 commented 4 years ago

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/ !!!

Daandelange commented 4 years ago

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

d3cod3 commented 4 years ago

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?

Daandelange commented 4 years ago

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 ?

d3cod3 commented 4 years ago

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.

Daandelange commented 4 years ago

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

Daandelange commented 4 years ago

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() ) )
d3cod3 commented 4 years ago

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

map<int,shared_ptr> objects;

map<int,shared_ptr> parameters;

map<int,shared_ptr> 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.

Daandelange commented 4 years ago

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 :

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

d3cod3 commented 4 years ago

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)

Daandelange commented 4 years ago

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

Daandelange commented 4 years ago

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

d3cod3 commented 4 years ago

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 commented 4 years ago

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

Daandelange commented 4 years ago

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.

d3cod3 commented 4 years ago

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.

Daandelange commented 4 years ago

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

Daandelange commented 4 years ago

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 commented 3 years ago

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 ?

d3cod3 commented 3 years ago

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

Daandelange commented 3 years ago
  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.

d3cod3 commented 3 years ago

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 )

Daandelange commented 3 years ago

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 commented 3 years ago

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.

d3cod3 commented 3 years ago

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.

Daandelange commented 1 year ago

Hey, longtime ago ! I'm having some time for getting back on this awesome parameters madness.

1. Component Graph Structure We talked about unique IDs and addresses earlier. Corrade's LinkedList together with Kirby's (php) Component class inspired me an idea. Edit: I came trough this ofxApp project by Local Projects that uses a blueprint-scemes-graph-entity-component-system too. HasUUID could become a Component class with a little more information. Instead of having map<...> parameters and map<...> objects in Mosaic, we could implement some kind of "scene graph" / "tree structure" / "parent-child" component system all having a common root object. This way, components have addresses, which could also be useful for implementing a control/scripting API later. LinkedList assigns a next pointer to the next component when it's inserted into the array. The iterator is within the items which are automatically assigned a sequence. If we add a parent to this, we got a very performant tree structure.

  • root / Mosaic
    • list / Modules
    • Timeline
    • Sequencer
    • list / Nodes
    • MathOperation
    • Number1
    • Number2

Then the UUID of Number2 would be: mosaic.nodes.number2. Each instance could then provide custom API methods, for example:

2. Ownership of variables About the 3 maps we talked about before, I got an improvement proposal :

map<int,shared_ptr> objects; map<int,shared_ptr> parameters; map<int,shared_ptr> links;

Mosaic shouldn't keep track of these lists, except the objects map perhaps. We can simply keep track of their lifecycle using the constructor and destructor, registering them in a singleton list for being able to loop them quickly. Mosaic doesn't need ownership of links and parameters. This way, pins own their own connections and objects own their own parameters, removing the need of keeping in sync the maps from Mosaic. We haven't discussed much about ownership of variables, and in fact it could simplify the code a lot.

3. Dynamic parameter count (inlets/outlets) This is the only feature since the beginning of this discussion that I still don't know if it will be possible, but I hope so. I fear it will bring a lot of extra logics to handle restoring connections (from save) to pins that will exist only once the Object is setup, but not before. In the worst case, we can always output values as an array trough 1 (single) pin and use a "route" object to extract values from that array. We'll see.