Make hatch linewidth an rcParam #6198
Conversation
| @@ -32,6 +32,8 @@ patch.facecolor : b | |||
| patch.edgecolor : k | |||
| patch.antialiased : True # render patches in antialiased (no jaggies) | |||
|
|
|||
| hatch.linewidth : 1.0 | |||
There was a problem hiding this comment.
Did we have 3 (!) different default values for this before?
There was a problem hiding this comment.
Actually 4, since SVG was "1 point" and Agg was "1 pixel". We could try to emulate that old brokenness if we want (I guess).
There was a problem hiding this comment.
No, I think unify on one is an ok complete break. We should at least document what the old values were. On the off chance we get someone who really cares, they likely only care on one backend (or we would have heard from them sooner!).
There was a problem hiding this comment.
Agreed. I've documented the old values in the style changes what's new.
b352ddc to
7a982fb
Compare
| @@ -1182,7 +1182,7 @@ def writeHatches(self): | |||
| 0, 0, sidelen, sidelen, Op.rectangle, | |||
| Op.fill) | |||
|
|
|||
| self.output(0.1, Op.setlinewidth) | |||
| self.output(rcParams['hatch.strokewidth'], Op.setlinewidth) | |||
There was a problem hiding this comment.
should this be hatch.linewidth?
There was a problem hiding this comment.
Indeed. Will fix. Puzzled by why that didn't raise an exception.
3435e36 to
f95cccd
Compare
|
Passing Travis. AppVeyor seems blocked for last 3 hours. |
|
I think this is up next, but for some reason, AppVeyor only seems to be building one part of the matrix at a time. |
| @@ -1182,7 +1182,7 @@ def writeHatches(self): | |||
| 0, 0, sidelen, sidelen, Op.rectangle, | |||
| Op.fill) | |||
|
|
|||
| self.output(0.1, Op.setlinewidth) | |||
| self.output(rcParams['hatch.linewidth'], Op.setlinewidth) | |||
There was a problem hiding this comment.
How significant of a performance hit is it to be constantly re-querying this dictionary?
Also, is it the right thing to access the rcParams this late in the draw process? We are pretty bad at being consistent, but I don't recall doing this sort of rcParam access this late anywhere else. What if someone uses an rc context to set up the hatches, but the savefig call is outside that context?
There was a problem hiding this comment.
Fair point on being consistent, but there is currently no other API to change this value. The main reason for getting this PR in for 2.0 is to put as rcparam in so we can change it and provide a way to get back to the old version. Adding a proper API for changing this can be done later as a new feature and at that point it should be set at artist creation time, not draw time.
|
I am happy to merging this as-is to move 2.0 along. This improves things, there is now a way to change the hatch width and it is uniform across all of the backends (that we control) and closes a 3 digit bug! Still to do are:
@mdboom This has one overlap with the test images in the tick centering PR, maybe we should stack them into one merge? |
|
I think the axes spacing in #6129 will also touch many test images and maybe should be folded in as well if you're going to do so already. |
|
@tacaswell wrote:
Sure -- not sure how best to do that, though. |
|
rebase one of them on the other? |
|
I think we should just merge this and #6200 separately. It's sort of rebase hell to pull out the test images from this one -- plus it will make bisecting harder later. |
|
fair enough. |
ENH/API: Make hatch linewidth an rcParam This also changes the default hatch width on some backends.
|
backported to v2.x as f367f7b |
Fix #235