diff --git a/ci/release/changelogs/next.md b/ci/release/changelogs/next.md index ffe01dbc1e..2dc5c549b7 100644 --- a/ci/release/changelogs/next.md +++ b/ci/release/changelogs/next.md @@ -1,5 +1,7 @@ #### Features 🚀 +- elk: configure the spacing between edges routed between the same pair of layers with `--elk-edgeEdgeBetweenLayers`. It was previously fixed at 50, which forced large gaps between layers on graphs with many links between adjacent layers. + #### Improvements 🧹 - d2svg: diff --git a/d2cli/layout_elk.go b/d2cli/layout_elk.go index 9d77a64976..9787ce6380 100644 --- a/d2cli/layout_elk.go +++ b/d2cli/layout_elk.go @@ -5,9 +5,11 @@ package d2cli import ( "context" + "github.com/d2lang/util-go/go2" + "github.com/d2lang/util-go/xmain" + "github.com/d2lang/d2/d2graph" "github.com/d2lang/d2/d2layouts/d2elklayout" - "github.com/d2lang/util-go/xmain" ) const elkEnabled = true @@ -24,6 +26,7 @@ func registerELKFlags(opts *xmain.Opts) { opts.Int64("", "elk-nodeNodeBetweenLayers", "", int64(d2elklayout.DefaultOpts.NodeSpacing), "the spacing to be preserved between any pair of nodes of two adjacent layers") opts.String("", "elk-padding", "", d2elklayout.DefaultOpts.Padding, "the padding to be left to a parent element’s border when placing child elements") opts.Int64("", "elk-edgeNodeBetweenLayers", "", int64(d2elklayout.DefaultOpts.EdgeNodeSpacing), "the spacing to be preserved between nodes and edges that are routed next to the node’s layer") + opts.Int64("", "elk-edgeEdgeBetweenLayers", "", int64(*d2elklayout.DefaultOpts.EdgeEdgeSpacing), "the spacing to be preserved between pairs of edges routed between the same pair of layers") opts.Int64("", "elk-nodeSelfLoop", "", int64(d2elklayout.DefaultOpts.SelfLoopSpacing), "spacing to be preserved between a node and its self loops") } @@ -46,6 +49,11 @@ func elkLayout(ms *xmain.State) (d2graph.LayoutGraph, error) { if err != nil { return nil, err } + edgeEdgeSpacing, err := layoutIntFlag(ms, "elk-edgeEdgeBetweenLayers", *opts.EdgeEdgeSpacing) + if err != nil { + return nil, err + } + opts.EdgeEdgeSpacing = go2.Pointer(edgeEdgeSpacing) opts.SelfLoopSpacing, err = layoutIntFlag(ms, "elk-nodeSelfLoop", opts.SelfLoopSpacing) if err != nil { return nil, err diff --git a/d2cli/layout_elk_test.go b/d2cli/layout_elk_test.go index eb0bf91b30..01a035dfd6 100644 --- a/d2cli/layout_elk_test.go +++ b/d2cli/layout_elk_test.go @@ -6,6 +6,8 @@ import ( "context" "testing" + "github.com/d2lang/util-go/go2" + "github.com/d2lang/d2/d2graph" "github.com/d2lang/d2/d2layouts/d2elklayout" ) @@ -16,6 +18,7 @@ func TestELKLayoutOptions(t *testing.T) { NodeSpacing: 123, Padding: "[top=11,left=22,bottom=33,right=44]", EdgeNodeSpacing: 67, + EdgeEdgeSpacing: go2.Pointer(78), SelfLoopSpacing: 89, } want := layoutGraphJSON(t, func(ctx context.Context, g *d2graph.Graph) error { @@ -28,7 +31,8 @@ func TestELKLayoutOptions(t *testing.T) { state := layoutOptionsState(t, "--elk-algorithm=layered", "--elk-nodeNodeBetweenLayers=123", "--elk-padding=[top=11,left=22,bottom=33,right=44]", - "--elk-edgeNodeBetweenLayers=67", "--elk-nodeSelfLoop=89") + "--elk-edgeNodeBetweenLayers=67", "--elk-edgeEdgeBetweenLayers=78", + "--elk-nodeSelfLoop=89") resolve := LayoutResolver(t.Context(), state) if err := state.Opts.Flags.Set("elk-nodeNodeBetweenLayers", "234"); err != nil { t.Fatal(err) diff --git a/d2layouts/d2elklayout/layout.go b/d2layouts/d2elklayout/layout.go index e0dd619fa2..f58477914b 100644 --- a/d2layouts/d2elklayout/layout.go +++ b/d2layouts/d2elklayout/layout.go @@ -106,14 +106,18 @@ type ConfigurableOpts struct { NodeSpacing int `json:"spacing.nodeNodeBetweenLayers,omitempty"` Padding string `json:"elk.padding,omitempty"` EdgeNodeSpacing int `json:"spacing.edgeNodeBetweenLayers,omitempty"` + EdgeEdgeSpacing *int `json:"spacing.edgeEdgeBetweenLayers,omitempty"` SelfLoopSpacing int `json:"elk.spacing.nodeSelfLoop"` } +const default_edge_edge_spacing = 50 + var DefaultOpts = ConfigurableOpts{ Algorithm: "layered", NodeSpacing: 70.0, Padding: "[top=50,left=50,bottom=50,right=50]", EdgeNodeSpacing: 40.0, + EdgeEdgeSpacing: go2.Pointer(default_edge_edge_spacing), SelfLoopSpacing: 50.0, } @@ -173,16 +177,15 @@ func (s elkGraphStats) maxSelfLoopLabel(parent *d2graph.Object, isWidth bool) in } type elkOpts struct { - EdgeNode int `json:"elk.spacing.edgeNode,omitempty"` - FixedAlignment string `json:"elk.layered.nodePlacement.bk.fixedAlignment,omitempty"` - Thoroughness int `json:"elk.layered.thoroughness,omitempty"` - EdgeEdgeBetweenLayersSpacing int `json:"elk.layered.spacing.edgeEdgeBetweenLayers,omitempty"` - Direction Direction `json:"elk.direction"` - HierarchyHandling string `json:"elk.hierarchyHandling,omitempty"` - InlineEdgeLabels bool `json:"elk.edgeLabels.inline,omitempty"` - ForceNodeModelOrder bool `json:"elk.layered.crossingMinimization.forceNodeModelOrder,omitempty"` - ConsiderModelOrder string `json:"elk.layered.considerModelOrder.strategy,omitempty"` - CycleBreakingStrategy string `json:"elk.layered.cycleBreaking.strategy,omitempty"` + EdgeNode int `json:"elk.spacing.edgeNode,omitempty"` + FixedAlignment string `json:"elk.layered.nodePlacement.bk.fixedAlignment,omitempty"` + Thoroughness int `json:"elk.layered.thoroughness,omitempty"` + Direction Direction `json:"elk.direction"` + HierarchyHandling string `json:"elk.hierarchyHandling,omitempty"` + InlineEdgeLabels bool `json:"elk.edgeLabels.inline,omitempty"` + ForceNodeModelOrder bool `json:"elk.layered.crossingMinimization.forceNodeModelOrder,omitempty"` + ConsiderModelOrder string `json:"elk.layered.considerModelOrder.strategy,omitempty"` + CycleBreakingStrategy string `json:"elk.layered.cycleBreaking.strategy,omitempty"` SelfLoopDistribution string `json:"elk.layered.edgeRouting.selfLoopDistribution,omitempty"` @@ -196,21 +199,30 @@ type elkOpts struct { ConfigurableOpts } +func edgeEdgeSpacingOrDefault(opts *ConfigurableOpts) *int { + // Callers predating this option leave the field nil, and D2 sent 50 before + // it existed. Nil keeps that rather than dropping to ELK's default. + if opts.EdgeEdgeSpacing == nil { + return go2.Pointer(default_edge_edge_spacing) + } + return go2.Pointer(*opts.EdgeEdgeSpacing) +} + func newRootLayoutOptions(opts *ConfigurableOpts) *elkOpts { return &elkOpts{ - Thoroughness: 8, - EdgeEdgeBetweenLayersSpacing: 50, - EdgeNode: edge_node_spacing, - HierarchyHandling: "INCLUDE_CHILDREN", - FixedAlignment: "BALANCED", - ConsiderModelOrder: "NODES_AND_EDGES", - CycleBreakingStrategy: "GREEDY_MODEL_ORDER", - NodeSizeConstraints: "MINIMUM_SIZE", - ContentAlignment: "H_CENTER V_CENTER", + Thoroughness: 8, + EdgeNode: edge_node_spacing, + HierarchyHandling: "INCLUDE_CHILDREN", + FixedAlignment: "BALANCED", + ConsiderModelOrder: "NODES_AND_EDGES", + CycleBreakingStrategy: "GREEDY_MODEL_ORDER", + NodeSizeConstraints: "MINIMUM_SIZE", + ContentAlignment: "H_CENTER V_CENTER", ConfigurableOpts: ConfigurableOpts{ Algorithm: opts.Algorithm, NodeSpacing: opts.NodeSpacing, EdgeNodeSpacing: opts.EdgeNodeSpacing, + EdgeEdgeSpacing: edgeEdgeSpacingOrDefault(opts), SelfLoopSpacing: opts.SelfLoopSpacing, }, } @@ -218,11 +230,10 @@ func newRootLayoutOptions(opts *ConfigurableOpts) *elkOpts { func newContainerLayoutOptions(opts *ConfigurableOpts) *elkOpts { return &elkOpts{ - Thoroughness: 8, - EdgeEdgeBetweenLayersSpacing: 50, - HierarchyHandling: "INCLUDE_CHILDREN", - FixedAlignment: "BALANCED", - EdgeNode: edge_node_spacing, + Thoroughness: 8, + HierarchyHandling: "INCLUDE_CHILDREN", + FixedAlignment: "BALANCED", + EdgeNode: edge_node_spacing, // ELK 0.12 crashes on compound subgraphs when model-order // processing is enabled below the root. Keep it disabled there. ConsiderModelOrder: "NONE", @@ -232,6 +243,7 @@ func newContainerLayoutOptions(opts *ConfigurableOpts) *elkOpts { ConfigurableOpts: ConfigurableOpts{ NodeSpacing: opts.NodeSpacing, EdgeNodeSpacing: opts.EdgeNodeSpacing, + EdgeEdgeSpacing: edgeEdgeSpacingOrDefault(opts), SelfLoopSpacing: opts.SelfLoopSpacing, Padding: opts.Padding, }, diff --git a/d2layouts/d2elklayout/options_test.go b/d2layouts/d2elklayout/options_test.go index a73c50cc31..f7ad2d76b5 100644 --- a/d2layouts/d2elklayout/options_test.go +++ b/d2layouts/d2elklayout/options_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/d2lang/util-go/go2" + "github.com/d2lang/d2/d2compiler" "github.com/d2lang/d2/d2graph" "github.com/d2lang/d2/lib/geo" @@ -19,6 +21,7 @@ func TestConfigurableOptsSerializeELK012Keys(t *testing.T) { NodeSpacing: 123, Padding: "[top=11,left=22,bottom=33,right=44]", EdgeNodeSpacing: 67, + EdgeEdgeSpacing: go2.Pointer(78), SelfLoopSpacing: 89, } raw, err := json.Marshal(opts) @@ -34,6 +37,7 @@ func TestConfigurableOptsSerializeELK012Keys(t *testing.T) { "spacing.nodeNodeBetweenLayers": float64(123), "elk.padding": "[top=11,left=22,bottom=33,right=44]", "spacing.edgeNodeBetweenLayers": float64(67), + "spacing.edgeEdgeBetweenLayers": float64(78), "elk.spacing.nodeSelfLoop": float64(89), } if len(got) != len(want) { @@ -46,6 +50,75 @@ func TestConfigurableOptsSerializeELK012Keys(t *testing.T) { } } +func TestUnsetEdgeEdgeSpacingKeepsPreviousDefault(t *testing.T) { + // A Go API caller predating the EdgeEdgeSpacing option leaves the field nil, which + // must keep the spacing D2 sent before the option existed rather than ELK's own. + opts := ConfigurableOpts{ + Algorithm: "layered", + NodeSpacing: 70, + Padding: "[top=50,left=50,bottom=50,right=50]", + EdgeNodeSpacing: 40, + SelfLoopSpacing: 50, + } + for name, got := range map[string]*elkOpts{ + "root": newRootLayoutOptions(&opts), + "container": newContainerLayoutOptions(&opts), + } { + if got.EdgeEdgeSpacing == nil || *got.EdgeEdgeSpacing != default_edge_edge_spacing { + t.Errorf("%s edge-edge spacing = %v, want %d", name, got.EdgeEdgeSpacing, default_edge_edge_spacing) + } + } +} + +func TestExplicitZeroEdgeEdgeSpacingIsHonored(t *testing.T) { + // Unset is nil, so an explicit zero is a real request for no extra spacing + // and must reach ELK rather than being read back as the default. + opts := DefaultOpts + opts.EdgeEdgeSpacing = go2.Pointer(0) + for name, got := range map[string]*elkOpts{ + "root": newRootLayoutOptions(&opts), + "container": newContainerLayoutOptions(&opts), + } { + if got.EdgeEdgeSpacing == nil || *got.EdgeEdgeSpacing != 0 { + t.Fatalf("%s edge-edge spacing = %v, want 0", name, got.EdgeEdgeSpacing) + } + raw, err := json.Marshal(got.ConfigurableOpts) + if err != nil { + t.Fatal(err) + } + var fields map[string]any + if err := json.Unmarshal(raw, &fields); err != nil { + t.Fatal(err) + } + if fields["spacing.edgeEdgeBetweenLayers"] != float64(0) { + t.Errorf("%s serialized %s, want an explicit zero for spacing.edgeEdgeBetweenLayers", name, raw) + } + } +} + +func TestUnsetEdgeEdgeSpacingIsOmitted(t *testing.T) { + // ConfigurableOpts is embedded in elkOpts, which is also built bare for + // leaf nodes, ports and edge labels. ELK only applies edgeEdgeBetweenLayers + // to parents, so those elements must not assert a spacing of their own. + for name, opts := range map[string]*elkOpts{ + "leaf node": {SelfLoopDistribution: "EQUALLY"}, + "port": {PortSide: South}, + "edge label": {InlineEdgeLabels: true}, + } { + raw, err := json.Marshal(opts) + if err != nil { + t.Fatal(err) + } + var got map[string]any + if err := json.Unmarshal(raw, &got); err != nil { + t.Fatal(err) + } + if _, ok := got["spacing.edgeEdgeBetweenLayers"]; ok { + t.Errorf("%s options carry spacing.edgeEdgeBetweenLayers, want it omitted (serialized %s)", name, raw) + } + } +} + func TestNativeELKMigrationProfile(t *testing.T) { root := newRootLayoutOptions(&DefaultOpts) if root.ConsiderModelOrder != "NODES_AND_EDGES" || root.CycleBreakingStrategy != "GREEDY_MODEL_ORDER" || root.ForceNodeModelOrder { @@ -104,6 +177,21 @@ func TestConfigurableOptsAffectLayout(t *testing.T) { } }) + t.Run("edge-edge spacing between layers", func(t *testing.T) { + const src = "a -> x\na -> y\nb -> x\nb -> y\nc -> x\nc -> y" + compact := layoutOptionFixture(t, src, func(opts *ConfigurableOpts) { + opts.EdgeEdgeSpacing = go2.Pointer(5) + }) + spacious := layoutOptionFixture(t, src, func(opts *ConfigurableOpts) { + opts.EdgeEdgeSpacing = go2.Pointer(200) + }) + compactGap := verticalGap(objectByID(t, compact, "a"), objectByID(t, compact, "x")) + spaciousGap := verticalGap(objectByID(t, spacious, "a"), objectByID(t, spacious, "x")) + if spaciousGap-compactGap < 300 { + t.Fatalf("edge-edge layer gap changed from %g to %g, want a material increase", compactGap, spaciousGap) + } + }) + t.Run("self-loop spacing", func(t *testing.T) { compact := layoutOptionFixture(t, "a -> a", func(opts *ConfigurableOpts) { opts.SelfLoopSpacing = 20 diff --git a/d2layouts/d2elklayout/wasm_test.go b/d2layouts/d2elklayout/wasm_test.go index 076d9fc347..552d64f438 100644 --- a/d2layouts/d2elklayout/wasm_test.go +++ b/d2layouts/d2elklayout/wasm_test.go @@ -4,9 +4,12 @@ package d2elklayout import ( "context" + "reflect" "strings" "testing" + "github.com/d2lang/util-go/go2" + "github.com/d2lang/d2/d2compiler" "github.com/d2lang/d2/lib/geo" ) @@ -24,6 +27,7 @@ func TestConvertGraphPreservesCustomELKProfile(t *testing.T) { NodeSpacing: 123, Padding: "[top=11,left=22,bottom=33,right=44]", EdgeNodeSpacing: 67, + EdgeEdgeSpacing: go2.Pointer(78), SelfLoopSpacing: 89, } @@ -38,7 +42,9 @@ func TestConvertGraphPreservesCustomELKProfile(t *testing.T) { } root := rootOpts.ConfigurableOpts if root.Algorithm != opts.Algorithm || root.NodeSpacing != opts.NodeSpacing || - root.EdgeNodeSpacing != opts.EdgeNodeSpacing || root.SelfLoopSpacing != opts.SelfLoopSpacing { + root.EdgeNodeSpacing != opts.EdgeNodeSpacing || + root.EdgeEdgeSpacing == nil || *root.EdgeEdgeSpacing != *opts.EdgeEdgeSpacing || + root.SelfLoopSpacing != opts.SelfLoopSpacing { t.Fatalf("root profile = %#v, want algorithm/spacing values from %#v", root, opts) } if len(elkGraph.Children) != 1 || elkGraph.Children[0].ID != "container" { @@ -52,10 +58,12 @@ func TestConvertGraphPreservesCustomELKProfile(t *testing.T) { t.Fatalf("container migration profile = consider %q, cycle %q, force model order %t; want NONE/GREEDY/false", containerOpts.ConsiderModelOrder, containerOpts.CycleBreakingStrategy, containerOpts.ForceNodeModelOrder) } - if containerOpts.ConfigurableOpts != (ConfigurableOpts{ + // EdgeEdgeSpacing is a pointer, so compare by value rather than identity. + if !reflect.DeepEqual(containerOpts.ConfigurableOpts, ConfigurableOpts{ NodeSpacing: opts.NodeSpacing, Padding: opts.Padding, EdgeNodeSpacing: opts.EdgeNodeSpacing, + EdgeEdgeSpacing: opts.EdgeEdgeSpacing, SelfLoopSpacing: opts.SelfLoopSpacing, }) { t.Fatalf("container profile = %#v, want custom spacing and padding", containerOpts.ConfigurableOpts)