Skip to content

Loading…

Rastized background color #2479

Merged
merged 3 commits into from

4 participants

@tacaswell
Matplotlib Developers member

Addresses issue #2473

@tacaswell tacaswell patch to fix issue #2473.
The change to the agg propagate to how `imsave` works so one test had
to be changed to account for the fact that pixels with alpha=0 are now
set to (1, 1, 1, 0) instead of (0, 0, 0, 0).
b3fc5ff
@mdboom
Matplotlib Developers member

This seems like a reasonable solution, given that we can't do alpha blending in postscript anyway. I wonder if there's a way to pass down the actual axes fill color and use that, though. We should also update the clear method itself in the same way.

@tacaswell tacaswell added class variable `_fill_color` to RendererAgg to hold the
color to use in `clear` calls to the render_base.

Changed all uses of `clear((...))` -> `clear(_fil_color)`.
3020d13
@tacaswell
Matplotlib Developers member

Added a class variable so that the value isn't as hard coded. I didn't add a setter function because that seems like an API change to me which should go onto master once this is merged into 1.3.x and 1.3.x is merged into master.

@tacaswell
Matplotlib Developers member

I am confused by what travis is doing here. It complies fine on my box, but travis complains that the new class member does not exist in the scope.

@mdboom
Matplotlib Developers member

That's definitely better. But what I was getting at is that the axes background can be changed by the user, and maybe we should use that value. I think the easiest way is to probably add an optional argument to "clear" to pass in a color, and have the code that draws the quadmesh call clear explicitly first.

@tacaswell
Matplotlib Developers member

Do you want that added to this PR or a separate PR against master?

@mdboom
Matplotlib Developers member

Ideally added to this PR -- I think it all fits under the category of bugfix. But if it looks too convoluted to solve that way, I think this PR is an obvious improvement over the current state of affairs.

@tacaswell
Matplotlib Developers member

Sorry for going quiet.

I am entering the home stretch of my PhD (defense is scheduled for December). I needed this fixed for some of my figures, but I don't want to commit to adding much else.

@pelson pelson commented on the diff
src/_backend_agg.cpp
@@ -421,7 +421,8 @@
rendererAA(),
rendererBin(),
theRasterizer(),
- debug(debug)
+ debug(debug),
+ _fill_color(agg::rgba(1, 1, 1, 0))
@pelson Matplotlib Developers member
pelson added a note

Why was this value changed from (0, 0, 0, 0) to (1, 1, 1, 0)? This was a change I introduced in d8fce7b which I'm surprised is not triggering some of the tests I added to fail. Would you mind providing some more detail in the description of the PR please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on the diff
lib/matplotlib/tests/test_image.py
@@ -147,7 +147,8 @@ def test_imsave_color_alpha():
# Wherever alpha values were rounded down to 0, the rgb values all get set
# to 0 during imsave (this is reasonable behaviour).
# Recreate that here:
- data[data[:, :, 3] == 0] = 0
+ for j in range(3):
+ data[data[:, :, 3] == 0, j] = 1
@pelson Matplotlib Developers member
pelson added a note

The change from 0 to 1 worries me. Could you provide some more information about what this PR achieves in the description please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Matplotlib Developers member

I'd like to put the brakes on this PR as I have a few concerns, mainly with regards to the modification of expected behaviour (the test which was changed). I think we need some more detail as to the motivation for the change (please update the description of the PR) - only then is it feasible to agree on wether this is a bug fix, or a feature enhancement, and indeed whether this change is desirable. Right now I'm undecided and very much open minded on this PR.

PRs of interest to this: #1899, #1954, #1868. Pinging @Westacular as someone who got their hands really dirty on the Agg colour handling - your comments would be very welcome here.

Finally, just because I'm not 100% sold on this PR @tacaswell, it doesn't mean I don't appreciate how much effort has been put into this PR, so thank you!

Cheers,

@tacaswell
Matplotlib Developers member

@pelson this addresses issue #2473 which has code to reproduce the problem I am trying to address.

It looks like the agg backend renders the background as (0, 0, 0, 0) (transparent black). This work fine for anything which can do alpha-blending, however eps can not, so if you rasterize selected artistist which do not fully fill their bounding box you get a black background filling the rest of the bounding box. By changing the color to (1, 1, 1, 0) (transparent white) I don't think think that backends which can do alpha blending will care (as transparent is transparent), but degrades more gracefully in cases when the backend can't deal with alpha.

The change to the test is needed because it is testing the array returned. I don't think that there is any visible change in the pngs.

That said, I have only on a few occasions dug down into the Agg rendering so am doing a bit of cargo-cult programming here. I needed issue #2473 fixed for some figures in my thesis and took what I thought was the most direct route, but make no claim it is the best route.

@pelson
Matplotlib Developers member

Thanks for the update @tacaswell. I see your motivation for this change now and @mdboom's comments make a lot more sense to me :smile:
Obviously this change fixes the situation for a large number of cases, but on the other hand, it is not the correct solution and pretending that eps' major limitation doesn't exist can also cause problems. For instance, the eps and png output from your modified code are significantly different:

from pylab import *

figure(facecolor='red')
ax = axes()
ax.patch.set_facecolor('none')

plt.plot(0.8, 2.5, 'ob', zorder=-10)
X, Y = meshgrid(linspace(0, 1, 2048), linspace(0, 1, 1024))
Y *= 2 + sin(linspace(0, 2*np.pi, 2048))
C = np.sin(X + Y)
pcolormesh(X, Y, C, rasterized=True)

savefig('test.eps', facecolor='red')
show()

Whilst I don't have a problem with this change, the correct fix is much harder (and in general, may not be possible without adding transparency to eps files). My current thoughts are two approaches:

  • render everything below the rasterized object to Agg as well as the eps backend (not a go-er IMHO)
  • use clip paths on the rasterized image to clip out transparent pixels (I think this is doable, but probably hard)
  • I've not tried it, but apparently there is a plugin for ghostscript which does add alpha to eps files (couldn't for the life of me find the link) - perhaps that is one way to go also
  • Don't use eps - I'm guessing its the publisher insisting on eps files, right? Would PDF do?

:+1: for merging this, but acknowledging that it isn't the "correct" solution.

Thanks @tacaswell

@mdboom
Matplotlib Developers member

@pelson: I agree with your summary, i.e., this is not a complete solution, but I think it's "less bad" than the status quo.

FWIW, Cairo (itself, not our Cairo backend) deals with alpha transparency in PS exactly as you describe in your first bullet: it rasterizes everything up to and including transparent objects. The cairo-based tool "pdftocairo" will do this as well. I'd suggest that that is not a terrible solution to point users to -- either using our Cairo backend or outputting PDF and converting with pdftocairo. This, of course, assumes a Unixy environment where it's easy to install cairo.

Your second suggestion is interesting -- in the general case, you could end up with really large clip paths, I suppose.

@tacaswell
Matplotlib Developers member

@pelson That is true, but without these changes, the white region would just be replaced with a black region and it is still just as broken. There are cases (such as what drove me to do this) where a white region is less broken than a black region.

As a side note, is there an easy way for me to change an issue into a PR? That would have lead to less confusion in this case.

@pelson
Matplotlib Developers member

There are cases where a white region is less broken than a black region.

That is precisely what I meant with the statements: Whilst I don't have a problem with this change, the correct fix is much harder ...
:+1: for merging this, but acknowledging that it isn't the "correct" solution.

@tacaswell
Matplotlib Developers member

Sorry, just making sure every one was on the same page.

@pelson pelson merged commit 3ed08be into matplotlib:v1.3.x

1 check passed

Details default The Travis CI build passed
@tacaswell tacaswell deleted the tacaswell:rastized_background branch
@tacaswell
Matplotlib Developers member

@mdboom Can this be cherry picked to master? (should I just do it?)

@pelson
Matplotlib Developers member

This will get merged back to master by hand at some point - @mdboom and myself do this fairly regularly. Would it be helpful to you for me to do that now?

@tacaswell
Matplotlib Developers member

Yes. I need this along with a few other branches on master to generate the figures for my thesis. It would make my life easier if this were in master

@pelson
Matplotlib Developers member

Ok @tacaswell - hopefully that will make life easier for you. Commit to master: 06d0144

@tacaswell
Matplotlib Developers member

thanks

@neothemachine

Sorry for reviving, but I just stumbled upon the same issue and would need a black clearing color. It's really a shame that it cannot be set manually from Python world. I tried calling fig._cachedRenderer._renderer.clear(..) as a quick hack but this needs a agg::rgba which I have no idea how to produce from Python.

@tacaswell
Matplotlib Developers member

@neothemachine Sorry about breaking things for you! iirc there was discussion of being able to pass this value through, could you make a new issue to make sure it does not get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 30, 2013
  1. @tacaswell

    patch to fix issue #2473.

    tacaswell committed
    The change to the agg propagate to how `imsave` works so one test had
    to be changed to account for the fact that pixels with alpha=0 are now
    set to (1, 1, 1, 0) instead of (0, 0, 0, 0).
  2. @tacaswell

    added class variable `_fill_color` to RendererAgg to hold the

    tacaswell committed
    color to use in `clear` calls to the render_base.
    
    Changed all uses of `clear((...))` -> `clear(_fil_color)`.
  3. @tacaswell
This page is out of date. Refresh to see the latest.
Showing with 10 additions and 5 deletions.
  1. +2 −1 lib/matplotlib/tests/test_image.py
  2. +5 −4 src/_backend_agg.cpp
  3. +3 −0 src/_backend_agg.h
View
3 lib/matplotlib/tests/test_image.py
@@ -147,7 +147,8 @@ def test_imsave_color_alpha():
# Wherever alpha values were rounded down to 0, the rgb values all get set
# to 0 during imsave (this is reasonable behaviour).
# Recreate that here:
- data[data[:, :, 3] == 0] = 0
+ for j in range(3):
+ data[data[:, :, 3] == 0, j] = 1
@pelson Matplotlib Developers member
pelson added a note

The change from 0 to 1 worries me. Could you provide some more information about what this PR achieves in the description please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
assert_array_equal(data, arr_buf)
View
9 src/_backend_agg.cpp
@@ -421,7 +421,8 @@ RendererAgg::RendererAgg(unsigned int width, unsigned int height, double dpi,
rendererAA(),
rendererBin(),
theRasterizer(),
- debug(debug)
+ debug(debug),
+ _fill_color(agg::rgba(1, 1, 1, 0))
@pelson Matplotlib Developers member
pelson added a note

Why was this value changed from (0, 0, 0, 0) to (1, 1, 1, 0)? This was a change I introduced in d8fce7b which I'm surprised is not triggering some of the tests I added to fail. Would you mind providing some more detail in the description of the PR please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
_VERBOSE("RendererAgg::RendererAgg");
unsigned stride(width*4);
@@ -430,7 +431,7 @@ RendererAgg::RendererAgg(unsigned int width, unsigned int height, double dpi,
renderingBuffer.attach(pixBuffer, width, height, stride);
pixFmt.attach(renderingBuffer);
rendererBase.attach(pixFmt);
- rendererBase.clear(agg::rgba(0, 0, 0, 0));
+ rendererBase.clear(_fill_color);
rendererAA.attach(rendererBase);
rendererBin.attach(rendererBase);
hatchRenderingBuffer.attach(hatchBuffer, HATCH_SIZE, HATCH_SIZE,
@@ -1287,7 +1288,7 @@ void RendererAgg::_draw_path(path_t& path, bool has_clippath,
pixfmt hatch_img_pixf(hatchRenderingBuffer);
renderer_base rb(hatch_img_pixf);
renderer_aa rs(rb);
- rb.clear(agg::rgba(0.0, 0.0, 0.0, 0.0));
+ rb.clear(_fill_color);
rs.color(gc.color);
try {
@@ -2425,7 +2426,7 @@ RendererAgg::clear(const Py::Tuple& args)
_VERBOSE("RendererAgg::clear");
args.verify_length(0);
- rendererBase.clear(agg::rgba(0, 0, 0, 0));
+ rendererBase.clear(_fill_color);
return Py::Object();
}
View
3 src/_backend_agg.h
@@ -241,6 +241,9 @@ class RendererAgg: public Py::PythonExtension<RendererAgg>
const int debug;
+ agg::rgba _fill_color;
+
+
protected:
double points_to_pixels(const Py::Object& points);
agg::rgba rgb_to_color(const Py::SeqBase<Py::Object>& rgb, double alpha);
Something went wrong with that request. Please try again.