Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plotter: fix handling of x/y-offsets in Labels glyph boxes #711

Merged
merged 3 commits into from
Jul 1, 2021

Conversation

sbinet
Copy link
Member

@sbinet sbinet commented Jun 30, 2021

This CL fixes the handling of the X/Y-offsets of plotter.Labels: they
were previously ignored in the glyph box computation.
This CL also adds a test that exercizes offsetted labels and their glyph box.

Fixes #710.

Please take a look.

@sbinet
Copy link
Member Author

sbinet commented Jun 30, 2021

this is a spinoff of #708 and #710.
(I'll rebase #708 off that one once/when/if it's merged)

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2021

Codecov Report

Merging #711 (3933092) into master (bd0e370) will decrease coverage by 0.07%.
The diff coverage is 56.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage   71.91%   71.84%   -0.08%     
==========================================
  Files          56       58       +2     
  Lines        4957     5171     +214     
==========================================
+ Hits         3565     3715     +150     
- Misses       1206     1261      +55     
- Partials      186      195       +9     
Impacted Files Coverage Δ
plotter/quartile.go 70.09% <0.00%> (ø)
vg/geom.go 62.50% <0.00%> (-14.43%) ⬇️
plotter/boxplot.go 85.86% <100.00%> (ø)
plotter/labels.go 80.00% <100.00%> (-0.65%) ⬇️
vg/vggio/context.go 100.00% <0.00%> (ø)
vg/vggio/vggio.go 67.37% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd0e370...3933092. Read the comment docs.

@sbinet
Copy link
Member Author

sbinet commented Jun 30, 2021

note that we're still not out of the woods: the Aq and Bg labels in red should align correctly on the (resp.) blue and green horizontal lines (they should be the baselines.)

plotter/labels.go Outdated Show resolved Hide resolved
plotter/labels.go Outdated Show resolved Hide resolved
// label with positive x/y-offsets
loffp, err := plotter.NewLabels(plotter.XYLabels{
XYs: []plotter.XY{
{X: +0.0 + left, Y: +0},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the unary +s and decimal points necessary?

plotter/labels_test.go Outdated Show resolved Hide resolved
@sbinet
Copy link
Member Author

sbinet commented Jul 1, 2021

PTAL

Copy link
Member

@kortschak kortschak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One unresolved nit in labels_test.go.

Comment on lines 95 to 105
func (l *Labels) GlyphBoxes(p *plot.Plot) []plot.GlyphBox {
bs := make([]plot.GlyphBox, len(l.Labels))
for i, label := range l.Labels {
bs[i].X = p.X.Norm(l.XYs[i].X)
bs[i].Y = p.Y.Norm(l.XYs[i].Y)
sty := l.TextStyle[i]
bs[i].Rectangle = sty.Rectangle(label)
bs[i] = plot.GlyphBox{
X: p.X.Norm(l.XYs[i].X),
Y: p.Y.Norm(l.XYs[i].Y),
Rectangle: l.TextStyle[i].Rectangle(label).Add(l.Offset),
}
}
return bs
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you made it happen :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Team work; I wouldn't have looked without prompting.

This CL fixes the handling of the X/Y-offsets of plotter.Labels: they
were previously ignored in the glyph box computation.
This CL also adds a test that exercizes offsetted labels and their glyph box.

Fixes gonum#710.
@sbinet
Copy link
Member Author

sbinet commented Jul 1, 2021

sorry, fixed loffp and not the other.

PTAL.
(rebased+merged preemptively)

@sbinet sbinet merged commit a58aa90 into gonum:master Jul 1, 2021
@sbinet sbinet deleted the labels-glyph-box branch July 1, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plotter: Labels x/y-offsets are in vg.Length and it's inconvenient
3 participants