| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I tried adapting test_patches.py::test_patch_alpha_coloring to use the cairo backend, and the result reveals there are more issues with RendererCairo.draw_path_collection. With my fix for alpha above, this is the result: So the alpha channel is correct, but the line style is wrong. The result looks correct if I remove RendererCairo.draw_path_collection() altogether to fall back on RendererBase.draw_path_collection(). |
Sorry, something went wrong.
|
The proposed fix looks reasonable. |
Sorry, something went wrong.
|
Had a look at the linestyles issue, and it seems to be unrelated to the grouped draw. It's incorrect also if I disable grouped draw, but correct if I remove RendererCairo.draw_path_collection() altogether. |
Sorry, something went wrong.
|
No progress on the linestyle issue, maybe @anntzer can help? |
Sorry, something went wrong.
|
Looks like diff --git i/lib/matplotlib/backends/backend_cairo.py w/lib/matplotlib/backends/backend_cairo.py
index ce68d198f..3afd5be32 100644
--- i/lib/matplotlib/backends/backend_cairo.py
+++ w/lib/matplotlib/backends/backend_cairo.py
@@ -307,10 +307,13 @@ class RendererCairo(RendererBase):
for k, v in gc_vars.items():
if k == "_linestyle": # Deprecated, no effect.
continue
- try:
- getattr(gc, "set" + k)(v)
- except (AttributeError, TypeError) as e:
- pass
+ elif k == "_dashes":
+ gc.set_dashes(*v) # Needs to be unpacked.
+ else:
+ try:
+ getattr(gc, "set" + k)(v)
+ except (AttributeError, TypeError) as e:
+ pass
gc.ctx.new_path()
paths, transforms = zip(*grouped_draw)
grouped_draw.clear()
@@ -326,7 +329,8 @@ class RendererCairo(RendererBase):
transform = (Affine2D(transform.get_matrix())
.translate(xo, yo - self.height).scale(1, -1))
# rgb_fc could be a ndarray, for which equality is elementwise.
- new_key = vars(gc0), tuple(rgb_fc) if rgb_fc is not None else None
+ new_key = (vars(gc0).copy(), tuple(rgb_fc) if rgb_fc is not None
+ else None)
if new_key == reuse_key:
grouped_draw.append((path, transform))
else:
should fix the dashes issue. Can you confirm? |
Sorry, something went wrong.
|
Can you just integrate it in your pr? |
Sorry, something went wrong.
|
Sure, I can do that |
Sorry, something went wrong.
|
Do you want me to add the test as well? Currently there seems to be no tests for the cairo backend... |
Sorry, something went wrong.
|
There's also no machinery for that, but sure, if you can figure it out how to make it work, go for it! |
Sorry, something went wrong.
|
Sure, a drawback of using check_figures_equal is of course that we assume the reference figure is correct. |
Sorry, something went wrong.
|
You could e.g. compare an ellipse collection with two single ellipses. While in theory both may be wrong. On the one hand, it's reasonable to check that the two code paths yield the same result. Also it's quite unlikely that a future code change with simultaneously change both. So the test would actually be quite good. |
Sorry, something went wrong.
|
So I've added the test, which revealed yet another bug with the grouped draw (set_foreground() wasn't called for _rgb). After fixing this, the test passes, but only because it uses alpha blending and disables the grouped draw. Now if I modify the test and sett fill color alpha to 1.0, thus enabling grouped drawing, the draw order is messed up somehow: |
Sorry, something went wrong.
| edgecolor=(0, 0, 1, 0.75)) | ||
| ax.add_collection(col) | ||
|
|
||
| plt.savefig('patch_alpha_coloring_test.png') |
There was a problem hiding this comment.
no need to save the image in the test, you can check them using tools/triage_tests.py.
Sorry, something went wrong.
|
Looking at this again I'm not surprised by the failure: this is because the pathcollection drawer concatenates all paths and fills them in one go, then draw the edges in one go (so the entire blue edge is drawn after the entire red fill). I overlooked the overlapping-patch problem in #8787, sorry about that. |
Sorry, something went wrong.
|
Ah ok, should I change the patch then to remove draw_path_collection? |
Sorry, something went wrong.
|
At least I can't think of anything better... |
Sorry, something went wrong.
|
The style checker is unhappy: https://travis-ci.org/matplotlib/matplotlib/jobs/456934939#L2255. The OSX test failure should be covered by #12835. |
Sorry, something went wrong.
Remove RendererCairo.draw_path_collection as it made some wrong assumptions in the grouped draw optimization. Add test for the cairo backend to check rendering of path collections. Fixes matplotlib#12773
|
Thanks and congratulations to your first contribution to matplotlib! Hope to see you back some time. |
Sorry, something went wrong.
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
$ git checkout v3.0.x $ git pull
$ git cherry-pick -m1 8272e7b7870d3c85824dbbbde499ff385d268790
$ git commit -am 'Backport PR #12774: Cairo backend: Fix alpha render of collections'
git push YOURFORK v3.0.x:auto-backport-of-pr-12774-on-v3.0.x
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
Sorry, something went wrong.
…v3.0.x Backport PR #12774 on branch v3.0.x
| Back | FazBrowse Home | New Git URL |
Fix for #12773
PR Summary
RendererCairo.draw_path_collection() had two issues related to alpha channels.
First, the call to gc.set_alpha in line 310 would also inadvertently set gc._forced_alpha = True. This is fixed by moving the line which updates the gc from gc_vars below the loop.
Second, the code has an optimization where artists with identical properties would be grouped and then rendered in one go. This strategy doesn't work if the artists overlap and have alpha value < 1.0 in which case alpha blending is necessary. So the second fix disables the grouping optimization if any of the artists have alpha < 1.0 or the whole collection has alpha < 1.0.
PR Checklist
Not yet, but I'll try to write one if necessary.