clientIO / joint

A proven SVG-based JavaScript diagramming library powering exceptional UIs
https://jointjs.com
Mozilla Public License 2.0
4.48k stars 839 forks source link

Manhattan routing / DirectedGraph autolayout producing overlapped elements on programmatically created graph #678

Closed elmart closed 2 years ago

elmart commented 6 years ago

I'm creating a graph programmatically, and then applying DirectedGraph autolayout. I am using 'manhattan' router for my links without specifying any excludedTypes. Therefore, I would expect no elements to be overlapped if possible. But I'm getting this after applying autolayout:

autolayout

While I would expect this:

expected

The strange thing is that if i delete the wrong link (the one overlapping an element), and create it manually, I get the expected result.

I suppose the difference could be that when autolayout is first run, all node positions are [0, 0], and that somehow makes so that the link is set wrong (maybe because the overlapped node is positioned after the overlapping link).

Any ideas to avoid this?

elmart commented 6 years ago

It seems like a view refresh issue. If I do graph.fromJSON(graph.toJSON()) after relayout, everything gets as expected.

kumilingus commented 6 years ago

Hi, how do you load cells into the graph when the error occurs? graph.fromJSON() ?

elmart commented 6 years ago

Sorry I didn't answer before. For some reason, I didn't receive a notification on your answer, and I haven't manually looked until now.

No, I'm not using graph.fromJSON() to load cells initially. I don't have the json to load from. I'm automatically generating a graph from a more abstract structure which I call serverModel by using a builder which first clears the graph then adds units, then adds links. This is:

loadContext(serverModel) {
          new ServerModelGraphBuilder().buildGraph(this.graph, serverModel);
          this.layoutDirectedGraph();
}

class ServerModelGraphBuilder {
  buildGraph(graph, serverModel) {
    graph.clear();

    graph.set({
      id: serverModel.id,
      name: serverModel.name,
      type: 'app.Context',
      templateFormat: serverModel.templateFormat
    });

    this._buildUnits(graph, serverModel);
    this._buildLinks(graph, serverModel);
  }

  _buildUnits(graph, serverModel) {
    for (const unit of serverModel.units) {
      graph.addCell(_.assign({}, shapes[unit.type], {
        name: unit.name,
        unitConf: _.merge(
          getUnitTypeDefaultConf(unit.type),
          this._transformServerConfToClientConf(unit.conf)
        )
      }));
    }
  }

  _transformServerConfToClientConf(conf) {
    return _.cloneDeep(conf, (value, key) => {
      if (_.isString(key) && (key.match(/^(inPorts|outPorts|bounds)$/))) {
        const result = {};
        result[key] = _.cloneDeep(value);
        return result;
      }
    });
  }

  _buildLinks(graph, serverModel) {
    const elements = graph.getElements();

    for (const link of serverModel.links) {
      graph.addCell({
        type: 'app.Link',
        source: {
          id: elements.find(
            element => element.get('name') == link.source.name
          ).get('id'),
          port: `out-${link.source.port}`
        },
        target: {
          id: elements.find(
            element => element.get('name') == link.target.name
          ).get('id'),
          port: `in-${link.target.port}`
        }
      });
    }
  }
}

As I said before, the strange thing dissappear if after loading this way, I do graph.fromJSON(graph.toJSON()).

kumilingus commented 6 years ago

It seems, the problem will be in the DirectedGraph layout positioning elements one by one. At the time the link between Evaluator and TextFormatter is being updated (calculates manhattan) the SingleKeyMemory element has the default position (0,0) i.e. it's not an obstacle for the link yet.

Here's what I suggest.

var tmpGraph = new joint.dia.Graph;
tmpGraph.addCells([el1, el2, ..., elN], { dry: true });
tmpGraph.addCells([l1, l2, ..., lM], { dry: true })
joint.ui.DirectedGraph.layout(tmpGraph);
graph.resetCells(tmpGraph.getCells());
elmart commented 6 years ago

I understand why a first layout would fail (when positioning the wrong link, there are some units that will become obstacles that are not still there). Why I don't understand is why running layout a second time (once all nodes are already positiones) doesn't fix the wrong link. What is it that gets done differently between running layout a second time, and resetting cells?

kumilingus commented 6 years ago

As JointJS is built on Backbone, setting the attribute value that is already stored on the model triggers no events.

var model = new Model({ x: 5 });
model.set('x', 6); // triggers `change:x` and `change`
model.set('x', 6); // triggers no events

In your case the second layout sets the same elements' position as the first layout. Therefore no events are triggered and LinkViews do not update.

This should also do the trick (manually update the LinkViews). graph.getLinks().map(paper.findViewByModel, paper).forEach(view => view.update());

elmart commented 6 years ago

Exactly. That's why I said this seemed to be a view refresh issue. To me, the ideal solution would be for link elements to change some attribute (so that their view refreshes) when the physical routing path would change, even if source and destination remain the same. Do you think that's feasible?

kumilingus commented 6 years ago

Maybe something like below would work (not tested).

  1. A naive solution

    joint.layout.DirectedGraph.layout(graph, {
    setPosition: function(element, glNode) {
      element.set({
        position: {
          x: glNode.x - glNode.width / 2,
          y: glNode.y - glNode.height / 2
        },
        refresher:  (element.get('refresher') || 0) + 1
     });
    }
    });
  2. Hacky, but performant solution.

    joint.layout.DirectedGraph.layout(graph, {
    setPosition: function(element, glNode) {
      element.set('position', {
         x: glNode.x - glNode.width / 2,
         y: glNode.y - glNode.height / 2
      }, { cacheOnly: true /* will not update links yet */ });
    },
    setVertices: function(link, points) {
      var vertices = points.slice(1, points.length - 1);
      // trigger view update manually 
      link.unset('vertices', { silent: true });
      link.set('vertices', vertices);
    }
    })

    This scenario is definitely something to have in mind when we work on refactoring the links (after v1.2 JointJS release). To define better control of how/when links are updated.

elmart commented 6 years ago

None of the above approaches worked. First one didn't produce any noticeable change. Second produced the following:

screen shot 2017-10-23 at 14 30 53

In any case, it's ok for me by now, as far as this is taken into account for 1.2 release, as you indicated. Do you want me to add some kind of tag so that it doesn't get forgotten?

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 60 days with no activity. Please remove stale label or comment or this will be closed in 14 days.