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

Add ConnectionPolicy to NodeDataModel #44

Closed
wants to merge 6 commits into from

Conversation

russelltg
Copy link
Contributor

Currently, it's hardcoded that input connections can have 1 connection and input can have as many as they want.
This changes this, (keeping that as the default) but adds a nodeConnectionPolicy virtual function to NodeDataModel
so NodeDataModel instances can choose that on a per-dock basis.

I might have missed a few places that need to use this refactor.

It was allowed before, but canConnect disallowed it.

Maybe there should be a setting in the graph for connection policy
because I know you don't want multiple input connections (doesn't really
make sense for what you're trying to do...)
@paceholder
Copy link
Owner

What is the purpose of merging two inputs?

@russelltg
Copy link
Contributor Author

In chigraph, there are two types of connections: "exec" connections, that denote flow control and data connections, which are like your connections. Exec connections have to be able to have multiple inputs to allow for looping etc.

@paceholder
Copy link
Owner

I'll look through the code and merge this weekend

@paceholder
Copy link
Owner

The commit solves a particular problem but breaks the current concept of a "computational graph", which does not consider loops and "two-to-one" connections.

I think I need to elaborate the structure of the framework and construct several layers:

  1. Pure graph tools. Arbitrary number of inputs and outputs etc.
  2. Computational graph on top of it with propagation of the data

@russelltg
Copy link
Contributor Author

Mmm that is true. Maybe the computational graph should be implemented by the client via signals etc. That would definitely be the most flexible approach, and would make the "headless mode" more feasable.

@russelltg
Copy link
Contributor Author

The layer idea is quite good as well, maybe like a model-view like thing with the ability to define custom models, with the default being a computational graph.

@russelltg
Copy link
Contributor Author

russelltg commented Jan 21, 2017

All the hardcoded biases towards one input have been removed, but we sill have to think about the architecture

@facontidavide
Copy link
Contributor

I am very interested to get this PR merged... any progress?

@russelltg
Copy link
Contributor Author

If you have an idea on how to implement this in a clean way, absolutely. What do you think?

@facontidavide
Copy link
Contributor

My two cents...

In terms of dataflow, it make no sense to allow a PortType::In to have multiple connections.
If we agree, we can change this to make the NodeConnectionPolicy a property of PortType::Out only.

This is exactly the use case I need, that is not currently covered by the main branch; I can prepare a PR with that.

On the other hand...

I would love NodeEditor to be usefull also for other applications like state machines, but actually the source code is heavily focused on dtaflow and would require a major change that probably we don't want to do right now.

My pragmatic suggestion is to focus only on the NodeConnectionPolicy for PortType::Out. What do you think?

@russelltg
Copy link
Contributor Author

russelltg commented Mar 24, 2017

I'm willing to write that (should only take a few minutes), @paceholder what do you think? Is this something you're interested in?

@paceholder
Copy link
Owner

Do I understand it right, you propose to have two policies -- one for just single connection for the "OUT" ports and another one for multiple possible connections?

@facontidavide
Copy link
Contributor

Correct. OUT can be either single or multi connection, while IN can only be single connection

@paceholder
Copy link
Owner

Yes, than it makes sense and still preserves the current strategy of having "computational graph".
We can think later how to generalize the framework for just "graph editing"

@russelltg
Copy link
Contributor Author

Alright I'll file a new PR for this (I'll keep this branch because im using it on my project)

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

Successfully merging this pull request may close these issues.

None yet

3 participants