search

Daguerreo / NodeEditor

Qt Node Editor. Dataflow programming framework
BSD 3-Clause "New" or "Revised" License
18 stars 5 forks source link

Dynamic Ports #7

Open Daguerreo opened 3 years ago

Daguerreo commented 3 years ago

There are several try of implementation of dynamic ports. The current status is:

209, #212

251 is an improvement to 202 and 219, yet not completed because do not handle remotion of middle ports.

268 try to make a patch to the dynamic loading of 209.

However the best implementation has not a pull request and it's from @matthew-reid dynamicInterface branch

However the issues with that branch are:

Based on those previous attempt I've made a branch dynamic-ports with these features:

To deal with dynamic loading/saving I added a method hasDynamicPorts(PortType) const to NodeDataModel. If a model is marked as dynamic for port(s) type, when serializing it automatically add an entry dynamic_inputsand/or dynamic_outputsto write the number of ports at save time because they can be different from when they are created.

https://user-images.githubusercontent.com/6747152/102946478-16fd2d80-44c1-11eb-91f6-541e64be5b51.mp4

matthew-reid commented 3 years ago

This looks good, I'm glad you were able to improve on my attempt. I'd like to see this merged if the load issue can be fixed.

Daguerreo commented 3 years ago

This looks good, I'm glad you were able to improve on my attempt. I'd like to see this merged if the load issue can be fixed.

thank you @matthew-reid. If at any moment you could look at the code I'd be glad, 4 eyes are better of two.

Daguerreo commented 3 years ago

The bug about loading has been fixed, now feature should be complete.

matthew-reid commented 3 years ago

The code looks good. The only thing I would suggest is to add static helper functions readNumInputs(obj), readNumOutputs(obj) to NodeDataModel to parse the number of ports from the json object, so that the implementation of restore() doesn't need to know about the "dynamic_inputs" and "dynamic_outputs" string. An example restore override would look like:

void MaxModel::restore(const QJsonObject& obj)
{
   int in = readNumInputs(obj);
   if(in > 0){
     // since when node is saved port's number is size+1 with an empty port
     // to restore the correct size of the array it has to be input-1
      _numberList.resize(in-1);
   }
}
Daguerreo commented 3 years ago

I agree with your idea, yet don't know how to put it down without complicate the library or doing something very situational. Maybe a whole helper class for serialized information could be useful since there are several hardcoded key string. Need to think a bit about that.

Sarrasor commented 3 years ago

Hello, @Daguerreo. Cool feature!

I'm trying to implement dynamic outputs, and have faced several issues.

To add outputs dynamically, I have implemented two slots:

void MyNodeDataModel:outputConnectionCreated(Connection const& c)
{
  int out_port_index = c.getPortIndex(QtNodes::PortType::Out);
  if (out_port_index == static_cast<int>(_children_list.size()))
  {
    _children_list.push_back(c.getNode(QtNodes::PortType::In)->nodeDataModel()->name());
    Q_EMIT portAdded(PortType::Out, _children_list.size());
  }
  else
  {
    _children_list[out_port_index] = c.getNode(QtNodes::PortType::In)->nodeDataModel()->name(); 
  }
}

void MyNodeDataModel::outputConnectionDeleted(Connection const& c)
{
  int out_port_index = c.getPortIndex(QtNodes::PortType::Out);
  int in_port_index = c.getPortIndex(QtNodes::PortType::In);
  if((out_port_index != -1) && (in_port_index != -1))
  {
    if (out_port_index < static_cast<int>(_children_list.size()))
    {
      _children_list.erase(_children_list.begin() + out_port_index);
      Q_EMIT portRemoved(PortType::Out, out_port_index);
    }
  }
}

The issue happens on delete. MyNodeDataModel::portRemoved() triggers Node::onPortRemoved(), which will call Node::connectionRemoved(), and since we have already removed current connection, a segfault will occur. The way to solve the problem is to add current connection check:

for (Connection* connection : connections)
{
  int out_port_index = connection->getPortIndex(QtNodes::PortType::Out);
  int in_port_index = connection->getPortIndex(QtNodes::PortType::In);
  if ((portType == PortType::In) && (in_port_index == index))
  {
    continue;
  }
  if ((portType == PortType::Out) && (out_port_index == index))
  {
    continue;
  }
  Q_EMIT connectionRemoved(*connection);
}

However, there is still a problem with consequent connections. If I remove a child node - everything is file. If I just click on in port of a child node, the app will remove the connection and will try to add it again. At the same time, the app will shift other nodes, which will lead to the following issue:

https://user-images.githubusercontent.com/23615113/115986391-965a5500-a5b8-11eb-9d10-2ad55022bcf6.mp4

Maybe I do not understand something. Is there a better way to handle dynamic outputs?

Daguerreo commented 3 years ago

Hello @Sarrasor. Calculator example do contains an example node with dynamic ports. Have you looked at it?

Sarrasor commented 3 years ago

Hello @Sarrasor. Calculator example do contains an example node with dynamic ports. Have you looked at it?

Yep. The thing is - the example considers dynamic input ports, not dynamic output ports.

For example, consider the MaxModel from the calculator example

The logic of the dynamic port increments and signals is implemented in the MaxModel:: setInData(). In my case, however, I want to have dynamic outputs. Since there is no setOutData method, I have implemented the logic in slots

Is there an example of dynamic output ports, which I missed?

Daguerreo commented 3 years ago

I'm going to take a look into it. I could not do it in a very short time though. At first glance seems you're going to remove the port when the connection does still exists in the graphics object.

Daguerreo commented 3 years ago

@Sarrasor I've noticed NodeDataModel::outputConnectionDeleted() and NodeDataModel::ConnectionDeleted() are called twice when a node is deleted. There are a few of double call in the library, this one could affect the behaviour. I'm going to investigate

Daguerreo commented 3 years ago

Ok, I've should fixed the multiple delete connection call and added some nullptr check. In addition, I added a node using dynamic outputs in calculator example

https://user-images.githubusercontent.com/6747152/116112723-1d422700-a6b8-11eb-8257-d14e697af193.mp4

Sarrasor commented 3 years ago

Great, thank you!

Now my code works too:

https://user-images.githubusercontent.com/23615113/116120399-289d4e80-a6c8-11eb-8f31-ce061fac4699.mp4

The only thing I can suggest is to explicitly cast entry.size() to int. Yes, there is a non-negativity check before, but just to be safe.

https://github.com/Daguerreo/NodeEditor/blob/69149b3e4580c342dc0866e841a76fd19218073c/src/NodeState.cpp#L76

if(portIndex >= 0 && portIndex < static_cast<int>(entry.size()))
RValner commented 11 months ago

EDIT: I just realized that this issue thread is on a fork of nodeeditor. Opened a new issue here

@Daguerreo I'm trying to do something very similar as shown in the video above, i.e., whenever an input connection is created, +1 input port is added. I've seen the dynamic_ports example, but it's built on top of AbstractGraphModel, which to me seemed unnecessarily complex. Thus decided to use NodeDelegateModel instead, keeping the code on node level.

The issue i'm having is that if I have for example 3 connections and then remove the middle one, the connections will not be shifted, as shown in the video below:

https://github.com/Daguerreo/NodeEditor/assets/13798173/3f58c0dc-60f0-43e8-a748-774735fbd205

If I add portsAboutToBeDeleted and portsDeleted calls (as instructed here) to the overridden inputConnectionDeleted method:

void UmrfNodeModel::inputConnectionDeleted(QtNodes::ConnectionId const &ci)
{
    std::cout << ci.inPortIndex << std::endl;
    portsAboutToBeDeleted(PortType::In, ci.inPortIndex, ci.inPortIndex);
    input_connections_.erase(input_connections_.begin() + ci.inPortIndex);
    portsDeleted();
}

then deleting any connection above other connections will result removing all connections below it, as shown in the video:

https://github.com/Daguerreo/NodeEditor/assets/13798173/fabc8d02-4c33-456d-9942-b70432ecd933

The std::cout << ci.inPortIndex << std::endl; also shows that inputConnectionDeleted is indeed called for each deleted connection, hinting a recursive behavior. My gut feeling says i've added portsAboutToBeDeleted and portsDeleted to a wrong place, but atm I'm out of ideas, as there is not much to play with in the NodeDelegateModel API. I did skim throuh one of the older commits that implemented the desired behavior, but the API seems to have changed quite a bit in v3.

Any help is much appreciated.