From 900e4bab83a7a42afdbd6bc4112375ea67f057fe Mon Sep 17 00:00:00 2001 From: Alexander Wang Date: Sun, 12 Feb 2023 09:59:44 -0800 Subject: [PATCH 1/3] fix note placement bug --- d2layouts/d2sequence/constants.go | 23 +++++---- d2layouts/d2sequence/sequence_diagram.go | 65 ++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 15 deletions(-) diff --git a/d2layouts/d2sequence/constants.go b/d2layouts/d2sequence/constants.go index 95de80681..3a92ab1a4 100644 --- a/d2layouts/d2sequence/constants.go +++ b/d2layouts/d2sequence/constants.go @@ -1,21 +1,24 @@ package d2sequence -// leaves at least 25 units of space on the left/right when computing the space required between actors -const HORIZONTAL_PAD = 50. +// units of space on the left/right when computing the space required between actors +const HORIZONTAL_PAD = 40. -// leaves at least 25 units of space on the top/bottom when computing the space required between messages -const VERTICAL_PAD = 50. +// units of space on the top/bottom when computing the space required between messages +// TODO lower +const VERTICAL_PAD = 40. -const MIN_ACTOR_DISTANCE = 250. +const MIN_ACTOR_DISTANCE = 150. -const MIN_ACTOR_WIDTH = 150. +const MIN_ACTOR_WIDTH = 100. -const SELF_MESSAGE_HORIZONTAL_TRAVEL = 100. +const SELF_MESSAGE_HORIZONTAL_TRAVEL = 80. -const GROUP_CONTAINER_PADDING = 24. +const GROUP_CONTAINER_PADDING = 12. + +const EDGE_GROUP_LABEL_PADDING = 20. // min vertical distance between messages -const MIN_MESSAGE_DISTANCE = 80. +const MIN_MESSAGE_DISTANCE = 30. // default size const SPAN_BASE_WIDTH = 12. @@ -24,7 +27,7 @@ const SPAN_BASE_WIDTH = 12. const SPAN_DEPTH_GROWTH_FACTOR = 8. // when a span has a single messages -const MIN_SPAN_HEIGHT = 80. +const MIN_SPAN_HEIGHT = 30. const SPAN_MESSAGE_PAD = 16. diff --git a/d2layouts/d2sequence/sequence_diagram.go b/d2layouts/d2sequence/sequence_diagram.go index c1dd1de2a..53cdc8e86 100644 --- a/d2layouts/d2sequence/sequence_diagram.go +++ b/d2layouts/d2sequence/sequence_diagram.go @@ -156,6 +156,7 @@ func newSequenceDiagram(objects []*d2graph.Object, messages []*d2graph.Edge) *se for _, message := range sd.messages { sd.verticalIndices[message.AbsID()] = getEdgeEarliestLineNum(message) + // TODO this should not be global yStep, only affect the neighbors sd.yStep = math.Max(sd.yStep, float64(message.LabelDimensions.Height)) // ensures that long labels, spanning over multiple actors, don't make for large gaps between actors @@ -176,7 +177,6 @@ func newSequenceDiagram(objects []*d2graph.Object, messages []*d2graph.Edge) *se if _, exists := sd.firstMessage[message.Dst]; !exists { sd.firstMessage[message.Dst] = message } - } sd.yStep += VERTICAL_PAD @@ -209,6 +209,9 @@ func (sd *sequenceDiagram) placeGroups() { group.ZIndex = GROUP_Z_INDEX sd.placeGroup(group) } + for _, group := range sd.groups { + sd.adjustGroupLabel(group) + } } func (sd *sequenceDiagram) placeGroup(group *d2graph.Object) { @@ -273,6 +276,56 @@ func (sd *sequenceDiagram) placeGroup(group *d2graph.Object) { ) } +func (sd *sequenceDiagram) adjustGroupLabel(group *d2graph.Object) { + if group.LabelHeight == nil { + return + } + + heightAdd := (*group.LabelHeight + EDGE_GROUP_LABEL_PADDING) - GROUP_CONTAINER_PADDING + if heightAdd < 0 { + return + } + + group.Height += float64(heightAdd) + + // Extend stuff within this group + for _, g := range sd.groups { + if g.TopLeft.Y < group.TopLeft.Y && g.TopLeft.Y+g.Height > group.TopLeft.Y { + g.Height += float64(heightAdd) + } + } + for _, s := range sd.spans { + if s.TopLeft.Y < group.TopLeft.Y && s.TopLeft.Y+s.Height > group.TopLeft.Y { + s.Height += float64(heightAdd) + } + } + + // Move stuff down + for _, m := range sd.messages { + if go2.Min(m.Route[0].Y, m.Route[len(m.Route)-1].Y) > group.TopLeft.Y { + for _, p := range m.Route { + p.Y += float64(heightAdd) + } + } + } + for _, s := range sd.spans { + if s.TopLeft.Y > group.TopLeft.Y { + s.TopLeft.Y += float64(heightAdd) + } + } + for _, g := range sd.groups { + if g.TopLeft.Y > group.TopLeft.Y { + g.TopLeft.Y += float64(heightAdd) + } + } + for _, n := range sd.notes { + if n.TopLeft.Y > group.TopLeft.Y { + n.TopLeft.Y += float64(heightAdd) + } + } + +} + // placeActors places actors bottom aligned, side by side with centers spaced by sd.actorXStep func (sd *sequenceDiagram) placeActors() { centerX := sd.actors[0].Width / 2. @@ -354,7 +407,7 @@ func (sd *sequenceDiagram) placeNotes() { rankToX[sd.objectRank[actor]] = actor.Center().X } - for i, note := range sd.notes { + for _, note := range sd.notes { verticalIndex := sd.verticalIndices[note.AbsID()] y := sd.maxActorHeight + sd.yStep @@ -363,8 +416,10 @@ func (sd *sequenceDiagram) placeNotes() { y += sd.yStep } } - for _, otherNote := range sd.notes[:i] { - y += otherNote.Height + sd.yStep + for _, otherNote := range sd.notes { + if sd.verticalIndices[otherNote.AbsID()] < verticalIndex { + y += otherNote.Height + sd.yStep + } } x := rankToX[sd.objectRank[note]] - (note.Width / 2.) @@ -499,7 +554,7 @@ func (sd *sequenceDiagram) routeMessages() error { if isSelfMessage || isToDescendant || isFromDescendant || isToSibling { midX := startX + SELF_MESSAGE_HORIZONTAL_TRAVEL - endY := startY + MIN_MESSAGE_DISTANCE + endY := startY + MIN_MESSAGE_DISTANCE*1.5 message.Route = []*geo.Point{ geo.NewPoint(startX, startY), geo.NewPoint(midX, startY), From 281f163acfdebf380c716420e67957fb09b366bf Mon Sep 17 00:00:00 2001 From: Alexander Wang Date: Sun, 12 Feb 2023 10:25:28 -0800 Subject: [PATCH 2/3] tests --- d2layouts/d2sequence/constants.go | 2 +- d2layouts/d2sequence/layout_test.go | 4 +- d2layouts/d2sequence/sequence_diagram.go | 4 + .../diagram_wider_than_tooltip/sketch.exp.svg | 38 +- .../dagre/board.exp.json | 98 +- .../dagre/sketch.exp.svg | 6 +- .../elk/board.exp.json | 90 +- .../elk/sketch.exp.svg | 6 +- .../dagre/board.exp.json | 28 +- .../dagre/sketch.exp.svg | 6 +- .../elk/board.exp.json | 28 +- .../elk/sketch.exp.svg | 6 +- .../dagre/board.exp.json | 360 +++---- .../dagre/sketch.exp.svg | 28 +- .../elk/board.exp.json | 360 +++---- .../elk/sketch.exp.svg | 28 +- .../dagre/board.exp.json | 52 +- .../dagre/sketch.exp.svg | 6 +- .../elk/board.exp.json | 52 +- .../elk/sketch.exp.svg | 6 +- .../dagre/board.exp.json | 72 +- .../dagre/sketch.exp.svg | 12 +- .../elk/board.exp.json | 72 +- .../elk/sketch.exp.svg | 12 +- .../dagre/board.exp.json | 128 +-- .../dagre/sketch.exp.svg | 16 +- .../elk/board.exp.json | 128 +-- .../elk/sketch.exp.svg | 16 +- .../dagre/board.exp.json | 422 ++++---- .../dagre/sketch.exp.svg | 28 +- .../elk/board.exp.json | 422 ++++---- .../elk/sketch.exp.svg | 28 +- .../dagre/board.exp.json | 44 +- .../dagre/sketch.exp.svg | 10 +- .../elk/board.exp.json | 44 +- .../elk/sketch.exp.svg | 10 +- .../dagre/board.exp.json | 202 ++-- .../dagre/sketch.exp.svg | 24 +- .../elk/board.exp.json | 202 ++-- .../elk/sketch.exp.svg | 24 +- .../dagre/board.exp.json | 44 +- .../dagre/sketch.exp.svg | 6 +- .../elk/board.exp.json | 44 +- .../elk/sketch.exp.svg | 6 +- .../dagre/board.exp.json | 206 ++-- .../dagre/sketch.exp.svg | 26 +- .../elk/board.exp.json | 206 ++-- .../elk/sketch.exp.svg | 26 +- .../dagre/board.exp.json | 258 ++--- .../dagre/sketch.exp.svg | 6 +- .../elk/board.exp.json | 258 ++--- .../elk/sketch.exp.svg | 6 +- .../dagre/board.exp.json | 96 +- .../dagre/sketch.exp.svg | 8 +- .../sequence_diagram_note/elk/board.exp.json | 96 +- .../sequence_diagram_note/elk/sketch.exp.svg | 8 +- .../dagre/board.exp.json | 256 ++--- .../dagre/sketch.exp.svg | 34 +- .../sequence_diagram_real/elk/board.exp.json | 256 ++--- .../sequence_diagram_real/elk/sketch.exp.svg | 34 +- .../dagre/board.exp.json | 146 +-- .../dagre/sketch.exp.svg | 18 +- .../elk/board.exp.json | 146 +-- .../elk/sketch.exp.svg | 18 +- .../dagre/board.exp.json | 150 +-- .../dagre/sketch.exp.svg | 22 +- .../elk/board.exp.json | 150 +-- .../elk/sketch.exp.svg | 22 +- .../dagre/board.exp.json | 254 ++--- .../dagre/sketch.exp.svg | 30 +- .../sequence_diagram_span/elk/board.exp.json | 254 ++--- .../sequence_diagram_span/elk/sketch.exp.svg | 30 +- .../sequence_diagrams/dagre/board.exp.json | 918 +++++++++--------- .../sequence_diagrams/dagre/sketch.exp.svg | 54 +- .../sequence_diagrams/elk/board.exp.json | 842 ++++++++-------- .../sequence_diagrams/elk/sketch.exp.svg | 54 +- .../dagre/board.exp.json | 130 +-- .../dagre/sketch.exp.svg | 16 +- .../elk/board.exp.json | 130 +-- .../elk/sketch.exp.svg | 16 +- .../dagre/board.exp.json | 100 +- .../dagre/sketch.exp.svg | 20 +- .../elk/board.exp.json | 100 +- .../elk/sketch.exp.svg | 20 +- 84 files changed, 4324 insertions(+), 4320 deletions(-) diff --git a/d2layouts/d2sequence/constants.go b/d2layouts/d2sequence/constants.go index 3a92ab1a4..8e4b4e5e5 100644 --- a/d2layouts/d2sequence/constants.go +++ b/d2layouts/d2sequence/constants.go @@ -29,7 +29,7 @@ const SPAN_DEPTH_GROWTH_FACTOR = 8. // when a span has a single messages const MIN_SPAN_HEIGHT = 30. -const SPAN_MESSAGE_PAD = 16. +const SPAN_MESSAGE_PAD = 10. const LIFELINE_STROKE_WIDTH int = 2 diff --git a/d2layouts/d2sequence/layout_test.go b/d2layouts/d2sequence/layout_test.go index 20220ce03..8be43712d 100644 --- a/d2layouts/d2sequence/layout_test.go +++ b/d2layouts/d2sequence/layout_test.go @@ -407,8 +407,8 @@ func TestSelfEdges(t *testing.T) { t.Fatalf("route does not end at the same actor, start at %.5f, end at %.5f", route[0].X, route[3].X) } - if route[3].Y-route[0].Y != d2sequence.MIN_MESSAGE_DISTANCE { - t.Fatalf("expected route height to be %.f5, got %.5f", d2sequence.MIN_MESSAGE_DISTANCE, route[3].Y-route[0].Y) + if route[3].Y-route[0].Y != d2sequence.MIN_MESSAGE_DISTANCE*1.5 { + t.Fatalf("expected route height to be %.5f, got %.5f", d2sequence.MIN_MESSAGE_DISTANCE*1.5, route[3].Y-route[0].Y) } } diff --git a/d2layouts/d2sequence/sequence_diagram.go b/d2layouts/d2sequence/sequence_diagram.go index 53cdc8e86..c0b070e9b 100644 --- a/d2layouts/d2sequence/sequence_diagram.go +++ b/d2layouts/d2sequence/sequence_diagram.go @@ -288,6 +288,10 @@ func (sd *sequenceDiagram) adjustGroupLabel(group *d2graph.Object) { group.Height += float64(heightAdd) + // Spans are special + // If a span starts above the group, then extend it + // Otherwise, move it down + // Extend stuff within this group for _, g := range sd.groups { if g.TopLeft.Y < group.TopLeft.Y && g.TopLeft.Y+g.Height > group.TopLeft.Y { diff --git a/d2renderers/d2svg/appendix/testdata/diagram_wider_than_tooltip/sketch.exp.svg b/d2renderers/d2svg/appendix/testdata/diagram_wider_than_tooltip/sketch.exp.svg index f174f6658..d2aee73c9 100644 --- a/d2renderers/d2svg/appendix/testdata/diagram_wider_than_tooltip/sketch.exp.svg +++ b/d2renderers/d2svg/appendix/testdata/diagram_wider_than_tooltip/sketch.exp.svg @@ -3,7 +3,7 @@ id="d2-svg" style="background: white;" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" -width="1932" height="2282" viewBox="-134 -49 1932 2282">1Like starbucks or something -2I'm not sure what this is +}]]>1Like starbucks or something +2I'm not sure what this is