From dcf93c9b92c3fe8b1405d1938166612a3bdc43e1 Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 10:31:25 +0200 Subject: [PATCH 1/9] Skip empty layers when computing sub-graph shift after processing --- autolayout.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/autolayout.go b/autolayout.go index 52c98ea..667a54c 100644 --- a/autolayout.go +++ b/autolayout.go @@ -90,6 +90,10 @@ func Layout(source graph.Source, opts ...Option) graph.Layout { // compute shift for subsequent subgraphs rightmostX := 0.0 for _, l := range g.Layers { + if len(l.Nodes) == 0 { + // empty layers don't affect the shift + continue + } n := l.Nodes[len(l.Nodes)-1] rightmostX = max(rightmostX, n.X+n.W) } From 715cff1f596aa85b1f7ff936435283278154cfb8 Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 10:40:10 +0200 Subject: [PATCH 2/9] [monitor] Add log keys as const to standardize log output --- internal/monitor/keys.go | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 internal/monitor/keys.go diff --git a/internal/monitor/keys.go b/internal/monitor/keys.go new file mode 100644 index 0000000..8405aa2 --- /dev/null +++ b/internal/monitor/keys.go @@ -0,0 +1,7 @@ +package monitor + +// constant keys to help standardize log output +const ( + KeySkip = "skip" + KeySelfLoop = "self-loop" +) From 1a63dbb4fdcee542b808b545c0f0db2cfb7fd18d Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:01:20 +0200 Subject: [PATCH 3/9] Panic if the input graph has no nodes --- autolayout.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/autolayout.go b/autolayout.go index 667a54c..52393db 100644 --- a/autolayout.go +++ b/autolayout.go @@ -10,8 +10,7 @@ import ( "github.com/nulab/autog/internal/processor" ) -// todo: add interactive layout - +// Layout executes the layout algorithm on the graph G obtained from source. It panics if G contains no nodes. func Layout(source graph.Source, opts ...Option) graph.Layout { layoutOpts := defaultOptions for _, opt := range opts { @@ -32,6 +31,10 @@ func Layout(source graph.Source, opts ...Option) graph.Layout { // populate the graph struct from the graph source G := from(source) + if len(G.Nodes) == 0 { + panic("autog: node set is empty") + } + if layoutOpts.params.NodeFixedSizeFunc != nil { for _, n := range G.Nodes { layoutOpts.params.NodeFixedSizeFunc(n) From ccd6690d1526e09f3eda6bbf5fa15b9211b6be0a Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:24:04 +0200 Subject: [PATCH 4/9] Add short circuiting to each phase for graphs with one node; panic with zero nodes, as that could be a bug in the connected graphs code --- autolayout.go | 4 ++++ internal/phase1/alg_process.go | 8 ++++++++ internal/phase2/alg_process.go | 7 +++++++ internal/phase3/alg_process.go | 7 +++++++ internal/phase4/alg_process.go | 8 ++++++++ internal/phase5/alg_process.go | 8 ++++++++ 6 files changed, 42 insertions(+) diff --git a/autolayout.go b/autolayout.go index 52393db..0d5eb8e 100644 --- a/autolayout.go +++ b/autolayout.go @@ -49,6 +49,10 @@ func Layout(source graph.Source, opts ...Option) graph.Layout { // process each connected components and collect results into the same layout output for _, g := range connected.Components(G) { + if len(g.Nodes) == 0 { + panic("autog: connected sub-graph node set is empty: this might be a bug") + } + out.Nodes = slices.Grow(out.Nodes, len(g.Nodes)) out.Edges = slices.Grow(out.Edges, len(g.Edges)) diff --git a/internal/phase1/alg_process.go b/internal/phase1/alg_process.go index 515f9af..31884ad 100644 --- a/internal/phase1/alg_process.go +++ b/internal/phase1/alg_process.go @@ -8,6 +8,14 @@ import ( // Process runs this cycle breaking algorithm on the input graph. The graph must be connected. func (alg Alg) Process(g *graph.DGraph, _ graph.Params) { imonitor.PrefixFor(alg) + + // self-loop edges are removed from the edge list in a preprocessing step + // if that changes, reevaluate whether short-circuiting still makes sense here + if len(g.Nodes) == 1 { + imonitor.Log(imonitor.KeySkip, "not enough nodes") + return + } + // preprocessing removeTwoNodeCycles(g) if !hasCycles(g) { diff --git a/internal/phase2/alg_process.go b/internal/phase2/alg_process.go index 3de2dff..ecb2203 100644 --- a/internal/phase2/alg_process.go +++ b/internal/phase2/alg_process.go @@ -8,6 +8,12 @@ import ( // Process runs this layering algorithm on the input graph. The graph must be acyclic. func (alg Alg) Process(g *graph.DGraph, params graph.Params) { imonitor.PrefixFor(alg) + + if len(g.Nodes) == 1 { + // a single node defaults to layer zero + goto initLayers + } + switch alg { case LongestPath: execLongestPath(g) @@ -17,6 +23,7 @@ func (alg Alg) Process(g *graph.DGraph, params graph.Params) { panic("layering: unknown alg value") } +initLayers: m := map[int]*graph.Layer{} for _, n := range g.Nodes { layer := m[n.Layer] diff --git a/internal/phase3/alg_process.go b/internal/phase3/alg_process.go index 66fbe48..a8974e6 100644 --- a/internal/phase3/alg_process.go +++ b/internal/phase3/alg_process.go @@ -8,6 +8,13 @@ import ( // Process runs this ordering algorithm on the input graph. The graph nodes must be layered. func (alg Alg) Process(g *graph.DGraph, params graph.Params) { imonitor.PrefixFor(alg) + + if len(g.Nodes) == 1 { + // a single node defaults to position zero in layer zero + imonitor.Log(imonitor.KeySkip, "not enough nodes") + return + } + switch alg { case NoOrdering: return diff --git a/internal/phase4/alg_process.go b/internal/phase4/alg_process.go index 6173602..9592252 100644 --- a/internal/phase4/alg_process.go +++ b/internal/phase4/alg_process.go @@ -8,6 +8,14 @@ import ( // Process runs this positioning algorithm on the input graph. The graph nodes must be layered and ordered. func (alg Alg) Process(g *graph.DGraph, params graph.Params) { imonitor.PrefixFor(alg) + + if len(g.Nodes) == 1 { + // the node defaults to position (0,0) and the layer is as large as the node itself + g.Layers[0].W = g.Nodes[0].W + g.Layers[0].H = g.Nodes[0].H + return + } + switch alg { case NoPositioning: return diff --git a/internal/phase5/alg_process.go b/internal/phase5/alg_process.go index 3b9c26d..481e701 100644 --- a/internal/phase5/alg_process.go +++ b/internal/phase5/alg_process.go @@ -8,6 +8,14 @@ import ( func (alg Alg) Process(g *graph.DGraph, params graph.Params) { imonitor.PrefixFor(alg) + // currently self-loops are ignored, but it might make sense to route them here instead + // in that case, this code would have to account for graphs with a single node and a self-loop, e.g. N1 -> N1 + // instead of short-circuit + if len(g.Nodes) == 1 { + imonitor.Log(imonitor.KeySkip, "not enough nodes") + return + } + // side effects: this call merges long edges, basically undoes phase 3's breakLongEdges. // virtual nodes which the edges go through are collected into route structs // after this call, graph traversals that follow directed edges won't see virtual nodes anymore From 28990fbd03cc185cf2b8f257a527b38ce08c5a2f Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:56:16 +0200 Subject: [PATCH 5/9] [preproc] Ignore self-loops during main pipeline (fixes #23) This is done by simply removing self-loops from G before processing and readding them afterwards. Self-loops are not routed. --- autolayout.go | 7 ++++ .../preprocessor/ignore_self_loops.go | 32 +++++++++++++++++++ internal/processor/processor.go | 4 +++ internal/testfiles/adjacency_lists.go | 11 +++++++ internal/testfiles/bugfix_test.go | 11 +++++++ 5 files changed, 65 insertions(+) create mode 100644 internal/processor/preprocessor/ignore_self_loops.go diff --git a/autolayout.go b/autolayout.go index 0d5eb8e..806f787 100644 --- a/autolayout.go +++ b/autolayout.go @@ -8,6 +8,7 @@ import ( "github.com/nulab/autog/internal/graph/connected" imonitor "github.com/nulab/autog/internal/monitor" "github.com/nulab/autog/internal/processor" + "github.com/nulab/autog/internal/processor/preprocessor" ) // Layout executes the layout algorithm on the graph G obtained from source. It panics if G contains no nodes. @@ -56,11 +57,17 @@ func Layout(source graph.Source, opts ...Option) graph.Layout { out.Nodes = slices.Grow(out.Nodes, len(g.Nodes)) out.Edges = slices.Grow(out.Edges, len(g.Edges)) + // pre-processing + restoreSelfLoops := preprocessor.IgnoreSelfLoops(g) + // run subgraph through the pipeline for _, phase := range pipeline { phase.Process(g, layoutOpts.params) } + // post-processing + restoreSelfLoops(g) + // collect nodes for _, n := range g.Nodes { if n.IsVirtual && !layoutOpts.output.keepVirtualNodes { diff --git a/internal/processor/preprocessor/ignore_self_loops.go b/internal/processor/preprocessor/ignore_self_loops.go new file mode 100644 index 0000000..e792c59 --- /dev/null +++ b/internal/processor/preprocessor/ignore_self_loops.go @@ -0,0 +1,32 @@ +package preprocessor + +import ( + "github.com/nulab/autog/internal/graph" + imonitor "github.com/nulab/autog/internal/monitor" + "github.com/nulab/autog/internal/processor" +) + +func IgnoreSelfLoops(g *graph.DGraph) processor.F { + del := graph.EdgeSet{} + for _, e := range g.Edges { + if e.From == e.To { + imonitor.Log(imonitor.KeySelfLoop, "removed: "+e.From.ID) + del[e] = true + } + } + for e := range del { + // using the appropriate From-Out and To-In fields for clarity, + // but it's always the same node with the same incoming and outgoing edge + e.From.Out.Remove(e) + e.To.In.Remove(e) + g.Edges.Remove(e) + } + return func(g *graph.DGraph) { + for e := range del { + imonitor.Log(imonitor.KeySelfLoop, "added: "+e.From.ID) + e.From.Out.Add(e) + e.To.In.Add(e) + g.Edges.Add(e) + } + } +} diff --git a/internal/processor/processor.go b/internal/processor/processor.go index 9fb9ad8..6ad2def 100644 --- a/internal/processor/processor.go +++ b/internal/processor/processor.go @@ -4,8 +4,12 @@ import ( "github.com/nulab/autog/internal/graph" ) +// P represents a pipeline processor type P interface { Phase() int String() string Process(*graph.DGraph, graph.Params) } + +// F represents a standalone function that can execute a processing task. +type F func(*graph.DGraph) diff --git a/internal/testfiles/adjacency_lists.go b/internal/testfiles/adjacency_lists.go index 5da7ae1..2b2c78d 100644 --- a/internal/testfiles/adjacency_lists.go +++ b/internal/testfiles/adjacency_lists.go @@ -199,3 +199,14 @@ var pn_brockackerman_BrockAckerman = [][]string{ {"N15", "N16"}, {"N16", "N13"}, } + +var singleSelfLoop = [][]string{ + {"a", "a"}, +} + +var withSelfLoop = [][]string{ + {"a", "b"}, + {"b", "c"}, + {"b", "b"}, + {"a", "d"}, +} diff --git a/internal/testfiles/bugfix_test.go b/internal/testfiles/bugfix_test.go index 8f3034a..7ffd8e0 100644 --- a/internal/testfiles/bugfix_test.go +++ b/internal/testfiles/bugfix_test.go @@ -81,4 +81,15 @@ func TestCrashers(t *testing.T) { assert.Len(t, layout.Nodes, 3) assert.Len(t, layout.Edges, 3) }) + + t.Run("self-loop", func(t *testing.T) { + t.Run("program halts", func(t *testing.T) { + src := graph.EdgeSlice(withSelfLoop) + assert.NotPanics(t, func() { _ = autog.Layout(src) }) + }) + t.Run("successful with single node", func(t *testing.T) { + src := graph.EdgeSlice(singleSelfLoop) + assert.NotPanics(t, func() { _ = autog.Layout(src) }) + }) + }) } From ec4d8fe2f9e4de9bd683523dcc7ebdae3ea5b23e Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:57:59 +0200 Subject: [PATCH 6/9] [docs] Mention in README that self-loops aren't routed --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2cf1cb9..68b6471 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ Unlike many similar projects that only provide a frontend in their language of c ## Usage -autog is a Go library that can be added as a dependency to your project using standard Go module commands. It requires Go 1.21: +autog is a Go library that can be added as a dependency to your project using standard Go module commands. It requires Go 1.22: $ go get -u github.com/nulab/autog@latest @@ -167,6 +167,8 @@ The result is indeed a set of cubic Bezier control points. This project is actively under development, but it is currently in version 0. Please be aware that the public API and exported methods may undergo changes. +- Self-loops don't break the program any more ([issue #23](https://github.com/nulab/autog/issues/23)) but are not supported. The final layout includes self-loop edges but those edges are not routed (`e.Points` is `nil`) + ## Bug reporting If you encounter a bug, please open a new issue and include at least the input graph that triggered the bug to help us reproduce and address it. From f98bc085a20ebb6d0e7835352cb3f8b97e91ab2e Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 12:00:27 +0200 Subject: [PATCH 7/9] [p5,postproc] Move code that restores reversed edges to a standalone post-processor --- autolayout.go | 2 ++ internal/phase5/alg_process.go | 8 -------- internal/processor/postprocessor/unreverse_edges.go | 13 +++++++++++++ 3 files changed, 15 insertions(+), 8 deletions(-) create mode 100644 internal/processor/postprocessor/unreverse_edges.go diff --git a/autolayout.go b/autolayout.go index 806f787..6f7382b 100644 --- a/autolayout.go +++ b/autolayout.go @@ -8,6 +8,7 @@ import ( "github.com/nulab/autog/internal/graph/connected" imonitor "github.com/nulab/autog/internal/monitor" "github.com/nulab/autog/internal/processor" + "github.com/nulab/autog/internal/processor/postprocessor" "github.com/nulab/autog/internal/processor/preprocessor" ) @@ -67,6 +68,7 @@ func Layout(source graph.Source, opts ...Option) graph.Layout { // post-processing restoreSelfLoops(g) + postprocessor.UnreverseEdges(g) // collect nodes for _, n := range g.Nodes { diff --git a/internal/phase5/alg_process.go b/internal/phase5/alg_process.go index 481e701..a88af3c 100644 --- a/internal/phase5/alg_process.go +++ b/internal/phase5/alg_process.go @@ -33,12 +33,4 @@ func (alg Alg) Process(g *graph.DGraph, params graph.Params) { default: panic("routing: unknown alg value") } - - // post-processing, restore all reversed edges - for _, e := range g.Edges { - if e.IsReversed { - // reverse back - e.Reverse() - } - } } diff --git a/internal/processor/postprocessor/unreverse_edges.go b/internal/processor/postprocessor/unreverse_edges.go new file mode 100644 index 0000000..0f6cc7c --- /dev/null +++ b/internal/processor/postprocessor/unreverse_edges.go @@ -0,0 +1,13 @@ +package postprocessor + +import "github.com/nulab/autog/internal/graph" + +// UnreverseEdges restores edge direction that was reversed during the cycle-breaking phase +func UnreverseEdges(g *graph.DGraph) { + for _, e := range g.Edges { + if e.IsReversed { + // reverse back + e.Reverse() + } + } +} From 0ab857af80e65771b21d7df297abc23924911c19 Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 12:22:20 +0200 Subject: [PATCH 8/9] [docs] Mention commit guidelines in README --- README.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/README.md b/README.md index 68b6471..f257ebc 100644 --- a/README.md +++ b/README.md @@ -169,6 +169,22 @@ Please be aware that the public API and exported methods may undergo changes. - Self-loops don't break the program any more ([issue #23](https://github.com/nulab/autog/issues/23)) but are not supported. The final layout includes self-loop edges but those edges are not routed (`e.Points` is `nil`) +## Commit guidelines + +Commits should be prefixed with a short name in square brackets (a tag) that summarizes +which main area of the code was changed. The prefixes may change as the repository structure changes. + +The prefixes are: + +- `[pN-name]`: phase `N`, where `N` is a number from 1 to 5 and an optional `name` mnemonic indicating a phase N's algorithm +- `[preproc]`: preprocessing code that runs before phase 1 +- `[postproc]`: postprocessing code that runs after phase 5 +- `[graph]`: the `graph` package(s), changes to the structures and types used throughout the program +- `[monitor]`: the `monitor` package +- `[geom]`: the `geom` package +- `[docs]`: documentation, within the code (e.g. comments for godocs) or README +- + ## Bug reporting If you encounter a bug, please open a new issue and include at least the input graph that triggered the bug to help us reproduce and address it. From df79890f3750a3b586cd801df7fbb19a0d1115e5 Mon Sep 17 00:00:00 2001 From: Gabriele <10689839+vibridi@users.noreply.github.com> Date: Fri, 20 Sep 2024 12:42:00 +0200 Subject: [PATCH 9/9] Add github workflow to run unit tests --- .github/workflows/go.yml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 .github/workflows/go.yml diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml new file mode 100644 index 0000000..c9d6072 --- /dev/null +++ b/.github/workflows/go.yml @@ -0,0 +1,19 @@ +name: Run Unit Tests + +on: + pull_request: + types: [opened, synchronize] + +jobs: + unit-test: + runs-on: ubuntu-latest + steps: + - name: Checkout repo + uses: actions/checkout@v4 + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: 'go.mod' + cache: false + - name: Run Tests + run: make test