graphp / graph

GraPHP is the mathematical graph/network library written in PHP.
MIT License
693 stars 74 forks source link

How to add ports for edges (GraphViz) #92

Closed clemens-tolboom closed 10 years ago

clemens-tolboom commented 10 years ago

In #36 a commented on the record structure which allows for 'deep' edge snap points.

Using tailport and headport for an edge we can do this. tail-port-label-head-2

digraph {

  node [
    shape=record
  ];

  source [
    label="<f0> left |<middle> middle |<f2> right"
    shape=record
  ]

  target [
    label="<f0> left |<f1> middle |<right> right"
    shape=record
  ]

  source -> target [
    tailport = middle
    headport = right
    taillabel = source
    label = middle
    headlabel = target
  ]

}

Having a vertex label like

node [shape=record];
struct1 [label="<f0> left | <f1> middle | <f2> right"];
another

we can connect an edge to f1 like this

a -> struct1:f1

Using portpos like

a -> struct1:f1:nw;

is even worse.

AFAIK we don't have support for this.

Defining this as a Layout is not an option either?

XREF

clue commented 10 years ago

Thanks for bringing this to my attention! Adding some kind of support for GraphViz' port definitions certainly sounds tempting.

Currently, just merely adding vertex ports requires a deeper understanding of GraphViz record/Mrecord structures or HTML labels, as in your above example. Personally, I think this is not quite ideal, but it's okay considering this is a general purpose graph library and not just a GraphViz dot abstraction. Should we choose to stick to that, I think it makes sense to add some kind of special layout attributes like this:

$a = $graph->getVertex('a');
$struct1 = $graph->getVertex('struct1');
$edge = $a->createEdgeTo($struct1);

$edge->getLayout()->setAttribute('port.target', 'f1');

We would then have to update our GraphViz class to filter out these edge layout attributes and rewrite the dot output accordingly.

I've updated the PR title to reflect the feature request.

clemens-tolboom commented 10 years ago

I agree with we have a graph lib and not a graphviz lib. And settings attributes for graphviz is then the way to do this.

Some remarks:

I'll write a test for this :)

Is there a way to attach this issue to a PR?

clue commented 10 years ago

Is port.target specific enough?

Probably not :> This might seem okay for directed edges, whereas there's no distinguished "target" for an undirected one. Any suggestions?

[…] to distinguish from real graphviz attributes.

Indeed! Prefixing this might be a good idea. Considering this feature is limited to GraphViz, perhaps prefix this with graphviz.?

Is there a way to attach this issue to a PR?

Just open a new PR and include "fixes #xx" in the description.

clemens-tolboom commented 10 years ago
Is port.target specific enough?

Probably not :> This might seem okay for directed edges, whereas there's no distinguished "target" for an undirected one. Any suggestions?

Reading http://www.graphviz.org/doc/info/attrs.html#h:undir_note we can relax on not working as graphviz states

To avoid possible confusion when such attributes are required, the user is encouraged to use a directed graph.

That means we should not care :-/ ... OTOH we prob could help our users with taking care for upgrading a undirected to a directed graph. But that's another issue.

Prefixing this might a good idea. Considering this feature is limited to GraphViz, perhaps prefix this with graphviz.?

Great suggestion graphviz.

Just open a new PR and include "fixes #xx" in the description.

Always nice to learn new stuff :)

clue commented 10 years ago

Is port.target specific enough?

Probably not :> This might seem okay for directed edges, whereas there's no distinguished "target" for an undirected one. Any suggestions?

Reading http://www.graphviz.org/doc/info/attrs.html#h:undir_note we can relax on not working as graphviz states

GraphViz documentation certainly makes a valid point. Now even irrespective of what GraphViz does, what should our API look like? Using the words "target" and "source" will ultimately be just as ambiguous in our context, as there's no such concept for an undirected edge.

Unless we can find a better wording I'm kind of okay with that, provided we add some documentation.

upgrading a undirected to a directed graph

I don't think that's an option here. Adding two directed edges instead of a single undirected one certainly leads to a whole new level of issues. Besides, this doesn't actually fix our naming issue, as we'd still have to apply the port definition on the original undirected edge.

clemens-tolboom commented 10 years ago

When we regard vertex - edge - vertex we can easily express vertex - (target:f1) - edge - (target:f6) - vertex

That is the relation vertex - edge has a layout / attribute. That is probably a no-go. Trouble is in example an ERD too has arrow type on both sides.

We are save if we only proces layout values on DirectedEdge only.

These would workout ok I guess:

graphviz.from.target = f1
graphviz.from.arrow = diamond
graphviz.to.target = f4
graphviz.to.arrow = vee

Let's wait for this to solve after moving Graphviz out of this repo?

clemens-tolboom commented 10 years ago

An example for arrows is https://bytebucket.org/estin/sadisplay/wiki/static/schema2.png found on https://bitbucket.org/estin/sadisplay/wiki/Home

clue commented 10 years ago

arrow type on both sides

Thanks for the links. Arrows use the normal layout attribute settings and can be changed as usual:

$edge->getLayout()->setAttribute('arrowtail', 'diamond')->setAttribute('arrowhead', 'vee');

Also, regarding port definitions: After some further inspection it turns out the following definition

source:sourceport -> target:targetport

is equivalent to

source -> target [tailport=sourceport, headport=targetport]

As such, your initial example can be expressed like this:

// a -> struct1:f1
$edge = $va->createEdgeTo($vstruct1);
$edge->getLayout()->setAttribute('headport', 'f1');

Would you care to confirm this? If so, I'd like to keep this as-is and consider this bug done. Perhaps we should add some examples (separate repo? #97) to show its usage?

clemens-tolboom commented 10 years ago

Nice:)

I've updated the issue summary.

clemens-tolboom commented 10 years ago

When we have new repo let's add code examples (read tests) generating all our puzzles.

My current practice is to generate artifacts in the tests directory. People interested in those output just have to run tests.

Created a new milestone to move issue 'out of sight'

clue commented 10 years ago

Yep, once #97 is done, we should probably add some layout tests.

Other than that, I assume this ticket can be closed?

clemens-tolboom commented 10 years ago

Nope ... we have to build examples and this is one of them.

clue commented 10 years ago

we have to build examples

I agree. Considering we're about to split off graphviz to a separate project (#97), I've linked this ticket to a dedicated documentation ticket there (graphp/graphviz#2).

So closing this in the core graph library.