plotter: implement GlyphBoxer for plotter.Line #762
Conversation
Fixes gonum#761. Signed-off-by: Sebastien Binet <binet@cern.ch>
|
one existential question: should we create one gigantic glyphbox (or rather 4, one around each of the |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #762 +/- ##
==========================================
+ Coverage 73.34% 74.85% +1.50%
==========================================
Files 56 58 +2
Lines 5159 7368 +2209
==========================================
+ Hits 3784 5515 +1731
- Misses 1191 1657 +466
- Partials 184 196 +12
... and 54 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It's worth taking a look at how this affects this file when zoomed in.
There was a problem hiding this comment.
it seems like, for the green "pre-" step, the outer limit of the steps is now the outer shell of the black line.
There was a problem hiding this comment.
I think I've convinced myself it's coming from an hysteresis effect stemming from the original width of the (vertical) line.
it only shows for vertical lines because, in the example, the p.Y.{Min,Max} are fixed and the glyph boxes won't change the data view range in y (as ymax=20 is quite larger than the biggest y value).
the effect is then coming from clipping the x/y-ranges (via padX in plot.go) + (probably) some floating point rounding errors.
|
I think what you have is probably the best approach. I am concerned about the impact on the step plot though. It does weird things to the alignment in ways that I would not expect. |
Fixes #761.
Please take a look.