Make hatch linewidth an rcParam #6198

mdboom · Mar 21, 2016

Did we have 3 (!) different default values for this before?

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).

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!).

Agreed. I've documented the old values in the style changes what's new.

should this be hatch.linewidth?

Indeed. Will fix. Puzzled by why that didn't raise an exception.

mdboom · Mar 23, 2016

Passing Travis. AppVeyor seems blocked for last 3 hours.

QuLogic · Mar 23, 2016

I think this is up next, but for some reason, AppVeyor only seems to be building one part of the matrix at a time.

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?

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.

so, here in AGG we have the graphics context holding this new parameter, but in the vector backends we don't use the graphics context for that?

Yes -- the C++ side can not access rcParams directly, so we have to stuff it somewhere to pass to _backend_agg.so.

There is no public API to change the hatch yet and it looks like vector backends do not have the proper hooks set up for the relevant functions to have a reference to the gc.

tacaswell · Mar 27, 2016

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:

early binding on all relevant artists (currently all backends will grab the hatch value at draw time instead of artist creation time
provide an API to change the hatch linewidth at both creation time and via OO interface
have all of the backends pull from the gc instead of rcparams directly.

@mdboom This has one overlap with the test images in the tick centering PR, maybe we should stack them into one merge?

QuLogic · Mar 27, 2016

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 · Mar 28, 2016

I think that #6129 can be done with rcparams in a way that will allow classic mode to not change the tests. This one and #6200 require changes to the tests.

mdboom · Apr 11, 2016

@tacaswell wrote:

@mdboom This has one overlap with the test images in the tick centering PR, maybe we should stack them into one merge?

Sure -- not sure how best to do that, though.

tacaswell · Apr 11, 2016

rebase one of them on the other?

mdboom · Apr 11, 2016

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.

tacaswell · Apr 11, 2016

fair enough.

tacaswell · Apr 11, 2016

backported to v2.x as f367f7b

mdboom added this to the 2.0 (style change major release) milestone Mar 21, 2016

mdboom added the needs_review label Mar 21, 2016

QuLogic added default changes Rcparams labels Mar 22, 2016

tacaswell referenced this pull request Mar 27, 2016
Closed
Rasterizing patch changes filling of hatches in pdf backend #6228

tacaswell merged commit 949056c into matplotlib:master Apr 11, 2016
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

tacaswell removed the needs_review label Apr 11, 2016

tacaswell added a commit that referenced this pull request Apr 11, 2016

tacaswell Merge pull request #6198 from mdboom/hatch-edge-width
ENH/API: Make hatch linewidth an rcParam This also changes the default hatch width on some backends.
f367f7b

This was referenced Nov 5, 2016

Closed

control hatch linewidth and fill border linewidth separately #4026

Merged

Make hatch colour an rcParam. #7415

@@ -32,6 +32,8 @@ patch.facecolor        : b
      patch.edgecolor        : k
      patch.antialiased      : True    # render patches in antialiased (no jaggies)
+    +hatch.linewidth        : 1.0

@@ -1182,7 +1182,7 @@ def writeHatches(self):
 , 0, sidelen, sidelen, Op.rectangle,
                                  Op.fill)
-    -            self.output(0.1, Op.setlinewidth)
+    +            self.output(rcParams['hatch.strokewidth'], Op.setlinewidth)

mdboom	`Make hatch linewidth an rcParam`		`3b2e7e9`
mdboom	`Add what's new`		`1516bd0`
mdboom	`Fix PDF hatch linewidth`		`f0d7391`
mdboom	`Fix SVG`	All checks have passed 2 successful checks continuous-integration/appveyor/pr — AppVeyor build succeeded Details continuous-integration/travis-ci/pr — The Travis CI build passed Details	`f95cccd`

@@ -1182,7 +1182,7 @@ def writeHatches(self):
 , 0, sidelen, sidelen, Op.rectangle,
                                  Op.fill)
-    -            self.output(0.1, Op.setlinewidth)
+    +            self.output(rcParams['hatch.linewidth'], Op.setlinewidth)

@@ -363,7 +363,7 @@ RendererAgg::_draw_path(path_t &path, bool has_clippath, const facepair_t &face,
              hatch_path_trans_t hatch_path_trans(hatch_path, hatch_trans);
              hatch_path_curve_t hatch_path_curve(hatch_path_trans);
              hatch_path_stroke_t hatch_path_stroke(hatch_path_curve);
-    -        hatch_path_stroke.width(1.0);
+    +        hatch_path_stroke.width(points_to_pixels(gc.hatch_linewidth));

matplotlib/matplotlib

Make hatch linewidth an rcParam #6198

mdboom commented Mar 21, 2016

mdboom added this to the 2.0 (style change major release) milestone Mar 21, 2016

mdboom added the needs_review label Mar 21, 2016

tacaswell commented on the diff Mar 22, 2016

jenshnielsen and 1 other commented on an outdated diff Mar 22, 2016

QuLogic added default changes Rcparams labels Mar 22, 2016

mdboom added some commits Mar 21, 2016

mdboom commented Mar 23, 2016

QuLogic commented Mar 23, 2016

WeatherGod commented on the diff Mar 24, 2016

WeatherGod commented on the diff Mar 24, 2016

tacaswell commented Mar 27, 2016

QuLogic commented Mar 27, 2016

tacaswell referenced this pull request Mar 27, 2016

Rasterizing patch changes filling of hatches in pdf backend #6228

tacaswell commented Mar 28, 2016

mdboom commented Apr 11, 2016

tacaswell commented Apr 11, 2016

mdboom commented Apr 11, 2016

tacaswell commented Apr 11, 2016

tacaswell merged commit `949056c` into matplotlib:master Apr 11, 2016
2 checks passed

2 checks passed

tacaswell removed the needs_review label Apr 11, 2016

tacaswell added a commit that referenced this pull request Apr 11, 2016

tacaswell commented Apr 11, 2016

This was referenced Nov 5, 2016

control hatch linewidth and fill border linewidth separately #4026

Make hatch colour an rcParam. #7415

matplotlib/matplotlib

Join GitHub today

Make hatch linewidth an rcParam #6198

Conversation

mdboom commented Mar 21, 2016

mdboom added this to the 2.0 (style change major release) milestone Mar 21, 2016

mdboom added the needs_review label Mar 21, 2016

tacaswell commented on the diff Mar 22, 2016

Show 2 comments Hide comments jenshnielsen and 1 other commented on an outdated diff Mar 22, 2016

QuLogic added default changes Rcparams labels Mar 22, 2016

mdboom added some commits Mar 21, 2016

All checks have passed

mdboom commented Mar 23, 2016

QuLogic commented Mar 23, 2016

WeatherGod commented on the diff Mar 24, 2016

WeatherGod commented on the diff Mar 24, 2016

tacaswell commented Mar 27, 2016

QuLogic commented Mar 27, 2016

tacaswell referenced this pull request Mar 27, 2016

tacaswell commented Mar 28, 2016

mdboom commented Apr 11, 2016

tacaswell commented Apr 11, 2016

mdboom commented Apr 11, 2016

tacaswell commented Apr 11, 2016

Hide details View details tacaswell merged commit 949056c into matplotlib:master Apr 11, 2016 2 checks passed

2 checks passed

tacaswell removed the needs_review label Apr 11, 2016

tacaswell added a commit that referenced this pull request Apr 11, 2016

tacaswell commented Apr 11, 2016

This was referenced Nov 5, 2016

jenshnielsen and 1 other commented on an outdated diff Mar 22, 2016

tacaswell merged commit `949056c` into matplotlib:master Apr 11, 2016
2 checks passed