Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Make hatch linewidth an rcParam#6198
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did we have 3 (!) different default values for this before?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed. I've documented the old values in the style changes what's new.
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should this behatch.linewidth?
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.
Indeed. Will fix. Puzzled by why that didn't raise an exception.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 nowa 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 rcParamThis also changes the default hatch width on some backends.
backported to v2.x asf367f7b |
Fix#235