Merge pull request #24 from nulab/dev-23/self-loops

.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

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,24 @@ 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`)
+
+## 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.

autolayout.go

@@ -8,10 +8,11 @@ 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"
 )
 
-// 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 +33,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)
@@ -46,14 +51,25 @@ 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))
 
+		// pre-processing
+		restoreSelfLoops := preprocessor.IgnoreSelfLoops(g)
+
 		// run subgraph through the pipeline
 		for _, phase := range pipeline {
 			phase.Process(g, layoutOpts.params)
 		}
 
+		// post-processing
+		restoreSelfLoops(g)
+		postprocessor.UnreverseEdges(g)
+
 		// collect nodes
 		for _, n := range g.Nodes {
 			if n.IsVirtual && !layoutOpts.output.keepVirtualNodes {
@@ -90,6 +106,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)
 		}

internal/monitor/keys.go

@@ -0,0 +1,7 @@
+package monitor
+
+// constant keys to help standardize log output
+const (
+	KeySkip     = "skip"
+	KeySelfLoop = "self-loop"
+)

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) {

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]

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

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

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
@@ -25,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()
-		}
-	}
 }

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()
+		}
+	}
+}

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)
+		}
+	}
+}

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)

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"},
+}

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) })
+		})
+	})
 }