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
plot: fix handling of font descents #642
Conversation
00295c0
to
9888640
Compare
Codecov Report
@@ Coverage Diff @@
## master #642 +/- ##
==========================================
- Coverage 71.52% 70.28% -1.25%
==========================================
Files 55 55
Lines 5492 4620 -872
==========================================
- Hits 3928 3247 -681
+ Misses 1390 1203 -187
+ Partials 174 170 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
perhaps, if not too much trouble, separate the commits into 2:
- rewiring of the
-regen
flag infrastructure - fixing the fonts' descent handling
(it's ok to leave them both into the same PR)
y += a.Label.Height(a.Label.Text) | ||
y += a.Label.Padding | ||
} | ||
|
||
marks := a.Tick.Marker.Ticks(a.Min, a.Max) | ||
ticklabelheight := tickLabelHeight(a.Tick.Label, marks) | ||
descent := a.Tick.Label.Font.Extents().Descent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't we move this up at line 263 and drop the line 265?
descent
should be a constant for a given font.Font
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L265 is for the label and L273 is for the tick label.
@@ -372,12 +374,13 @@ func (a verticalAxis) draw(c draw.Canvas) { | |||
} | |||
|
|||
major := false | |||
descent := a.Tick.Label.Font.Extents().Descent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: perhaps move that up at ~line 355 and drop the one at line 366 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason.
plot_test.go
Outdated
got := r.Actions | ||
c := vgimg.PngCanvas{Canvas: vgimg.New(5*vg.Centimeter, 5*vg.Centimeter)} | ||
l.Draw(draw.New(c)) | ||
b := bytes.NewBuffer([]byte{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b := bytes.NewBuffer([]byte{}) | |
b := new(bytes.Buffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
9888640
to
0694c3f
Compare
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This is probably worth a patch release. WDTY? |
Yes, I was about to suggest that as well. |
I'll do that tomorrow. |
interestingly, after updating my reference files on a Linux machine, tests failed on the Darwin machine (from Github Actions) but was ok on Windows: |
Font handling differences? |
could be, though I am using the embedded |
the delta is available here: the diff there: it seems there's only one pixel difference, not involving fonts, actually... |
there's a 1-pixel difference on darwin w/ the linux generated reference plot. see: gonum/plot#642 (comment) drop the image check on darwin for that particular test. Updates go-hep#818.
Please take a look.
Fixes #294