Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ofxVPParameters #22

Open
d3cod3 opened this issue Apr 4, 2020 · 32 comments
Open

ofxVPParameters #22

d3cod3 opened this issue Apr 4, 2020 · 32 comments
Labels
enhancement New feature or request

Comments

@d3cod3
Copy link
Owner

d3cod3 commented Apr 4, 2020

No description provided.

@d3cod3 d3cod3 added the enhancement New feature or request label Apr 4, 2020
@Daandelange
Copy link
Collaborator

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
Copy link
Collaborator

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 ?

@d3cod3
Copy link
Owner Author

d3cod3 commented Apr 7, 2020

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

@Daandelange
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

d3cod3 commented Apr 16, 2020

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
Copy link
Collaborator

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
Copy link
Owner Author

d3cod3 commented Apr 27, 2020

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
Copy link
Collaborator

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
Copy link
Owner Author

d3cod3 commented Apr 28, 2020

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
Copy link
Collaborator

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

@Daandelange
Copy link
Collaborator

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
Copy link
Owner Author

d3cod3 commented May 3, 2020

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
Copy link
Collaborator

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

@d3cod3
Copy link
Owner Author

d3cod3 commented May 4, 2020

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
Copy link
Collaborator

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

@Daandelange
Copy link
Collaborator

Latest update, in Mosaic-Engine-Tester repo, I got the first working link connections 🚀 .
See d3cod3/Mosaic-Engine-Tester@c913972 .
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
Copy link
Owner Author

d3cod3 commented Jul 14, 2020

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
Copy link
Owner Author

d3cod3 commented Jul 14, 2020

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

@Daandelange
Copy link
Collaborator

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
Copy link
Owner Author

d3cod3 commented Jul 15, 2020

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.

This was referenced Jul 16, 2020
@Daandelange
Copy link
Collaborator

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

@Daandelange
Copy link
Collaborator

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
Copy link
Collaborator

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 Timed vs Transformer objects #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
Copy link
Owner Author

d3cod3 commented Sep 14, 2020

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
Copy link
Collaborator

  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
Copy link
Owner Author

d3cod3 commented Sep 17, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

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 Plugin System #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
Copy link
Owner Author

d3cod3 commented Dec 11, 2020

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
Copy link
Collaborator

Daandelange commented Feb 9, 2023

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:

  • Mosaic.apiCall("mosaic.nodes.number2.setValue(4)")
  • Parameter.apiCall("parameter.parent.addNode('float').parameter('value').connectWith(parameter)")

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants