Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Rastized background color#2479

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

Merged
pelson merged 3 commits intomatplotlib:v1.3.xfromtacaswell:rastized_background
Oct 18, 2013

Conversation

tacaswell
Copy link
Member

Addresses issue#2473

The change to the agg propagate to how `imsave` works so one test hadto be changed to account for the fact that pixels with alpha=0 are nowset to (1, 1, 1, 0) instead of (0, 0, 0, 0).
@mdboom
Copy link
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 theclear method itself in the same way.

color to use in `clear` calls to the render_base.Changed all uses of `clear((...))` -> `clear(_fil_color)`.
@tacaswell
Copy link
MemberAuthor

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
Copy link
MemberAuthor

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
Copy link
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
Copy link
MemberAuthor

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

@mdboom
Copy link
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
Copy link
MemberAuthor

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.

@@ -421,7 +421,8 @@
rendererAA(),
rendererBin(),
theRasterizer(),
debug(debug)
debug(debug),
_fill_color(agg::rgba(1, 1, 1, 0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why was this value changed from(0, 0, 0, 0) to(1, 1, 1, 0)? This was a change I introduced ind8fce7b 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?

@pelson
Copy link
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 handsreally 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
Copy link
MemberAuthor

@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 anyvisible 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 thebest route.

@pelson
Copy link
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 😄
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?

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

Thanks@tacaswell

@mdboom
Copy link
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 withpdftocairo. 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
Copy link
MemberAuthor

@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 isless 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
Copy link
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
Copy link
MemberAuthor

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

pelson added a commit that referenced this pull requestOct 18, 2013
Set default agg canvas background color to ```(1, 1, 1, 0)```, rather than ```(0, 0, 0, 0)```.
@pelsonpelson merged commit3ed08be intomatplotlib:v1.3.xOct 18, 2013
@tacaswelltacaswell deleted the rastized_background branchOctober 18, 2013 15:04
@tacaswell
Copy link
MemberAuthor

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

@pelson
Copy link
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
Copy link
MemberAuthor

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

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull requestOct 28, 2013
Set default agg canvas background color to ```(1, 1, 1, 0)```, rather than ```(0, 0, 0, 0)```.
@pelson
Copy link
Member

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

@tacaswell
Copy link
MemberAuthor

thanks

@letmaik
Copy link
Contributor

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 callingfig._cachedRenderer._renderer.clear(..) as a quick hack but this needs aagg::rgba which I have no idea how to produce from Python.

@tacaswell
Copy link
MemberAuthor

@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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@tacaswell@mdboom@pelson@letmaik

[8]ページ先頭

©2009-2025 Movatter.jp