Skip to content

Fix single pixel markers #695

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 29, 2012
Merged

Fix single pixel markers #695

merged 6 commits into from
Feb 29, 2012

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Feb 6, 2012

The pixel shape is now optimized so it does the right thing in the Agg backend. For raster backends, linejoin and cap styles is now correctly passed to and used in the backend.

@plutino
Copy link

plutino commented Feb 6, 2012

Thanks Mike. This is really fast! I'll give it a try once I figure out how to check out from github and how not to mess up with my current mpl installation.

@@ -263,9 +274,11 @@ def _set_circle(self, reduction = 1.0):

def _set_pixel(self):
self._path = Path.unit_rectangle()
self._transform = Affine2D().translate(-0.5, 0.5)
self._transform = Affine2D().translate(-0.5, -0.5) \
.scale(0.5, 0.5).translate(0.5, 0.5)
self._snap_threshold = False
Copy link
Member

Choose a reason for hiding this comment

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

Left this in by accident?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're quick. I just saw this myself and amended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you reverting this change? I think this is a key to get a single pixel marker.

Copy link
Member Author

Choose a reason for hiding this comment

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

No -- there was previously a "print" statement here -- that has been removed, but this change will not be.

@plutino
Copy link

plutino commented Feb 9, 2012

As I tested just now, the Agg backend works perfectly. Any fix for postscript output? It's still 2x2 squares.

Also, in this branch the subplot configuration tool has some problem. When making adjustment and the screen being redrawn, the indicating bar changes from a blue rectangle to a blue triangle. I wish I can attach a screenshot here.

@plutino
Copy link

plutino commented Feb 9, 2012

My bad. pixels in postscript are indeed smaller than before, just not single pixel as the ticker line.

@mdboom
Copy link
Member Author

mdboom commented Feb 9, 2012

For the postscript (or any vector backend) there's no way to enforce strictly one pixel. All we can do is have it be one "dot" in whatever the "dots per inch" is set to. By that measurement, I believe it's correct, but let me know if you see otherwise.

@leejjoon
Copy link
Contributor

leejjoon commented Feb 9, 2012

My only concern here is that the center position of the original marker and the new one is shifted by (0.5, 0.5) pixel.
With the snapping thing happen, I'm not sure why we need that shift or if that shift has any consequences.

@leejjoon
Copy link
Contributor

leejjoon commented Feb 9, 2012

I did some test.

plot([0.012], [0.4], "k,")
xlim(0,1); ylim(0,1)

As one of y-ticks is at 0.4, I expected the maker will be aligned with the tickline, but it does not.

AGG backend

PS backend

If I adjust the transformation to

Affine2D().translate(-0.5, -0.5).scale(0.5, 0.5)

which I tend to think is a right transformation.
I get a expected result for the ps backend, but the point in AGG backend is blurred over 2x2 pixels and the center is slightly off. Maybe this has something to do with the snapping?

Also, note that the pixel size in PS backend is a bit larger than the tick line width. I guess this is expected as we are stroking a square of 0.5x0.5 pixel with 1 pixel thick line, i.e., resulting in a 1.5x1.5 pixel square. Maybe we can reduce the square size a bit more?

@mdboom
Copy link
Member Author

mdboom commented Feb 9, 2012

Snapping is turned off for single pixel markers. By definition for Agg, the center of a pixel is (0.5, 0.5), so the only way to draw a single pixel is by defining that as the center. We should probably fix the ticks to match this rather than the other way around, though we'll have to see what consequences that has.

@WeatherGod
Copy link
Member

Where are we on this? Does the snapping fix have anything to do with this?

@mdboom
Copy link
Member Author

mdboom commented Feb 28, 2012

I've been working with this, but it's kind of a rat's nest down there. I'll see what @leejjoon's snapping fixes do to this.

@leejjoon
Copy link
Contributor

@mdboom, should the translation to a pixel center of (0.5, 0.5) be done in the backckend level? I'm not sure why we need that shift for ps or pdf backend.

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

I think I have a solution here now. Can you test?

@WeatherGod
Copy link
Member

It looks perfect in PS. In the png version, there is no gap between the pixel marker and the tick. Also, it doesn't show up as a single pixel. Then again, I guess that could be due to compression?

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

Hmm... it should show up as a single pixel -- PNG uses lossless compression -- that's puzzling. Can you put a picture up somewhere (or e-mail to me?)

@WeatherGod
Copy link
Member

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

Hmm... I'm stumped. Anything set in your matplotlibrc?

@WeatherGod
Copy link
Member

Same result after getting rid of my modified matplotlibrc file. I should also note that in GTKAgg, the pixel point looks to be located exactly one pixel north of the tick mark (when doing show()). It didn't look like that for TKAgg.

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

Dumb question: Are you sure you're running the latest version? You seem to be describing the behavior before today's latest round of changes.

@WeatherGod
Copy link
Member

I am running your branch off based off of v1.1.x, which is what this PR is set for. I will note that I had to do some round-about way of getting it because (even though I have your repo for a remote) I couldn't just fetch your branch. I had to create my own branch (off of v1.1.x) and pull your branch. But the log looks right to me.

@@ -263,9 +274,9 @@ def _set_circle(self, reduction = 1.0):

def _set_pixel(self):
self._path = Path.unit_rectangle()
self._transform = Affine2D().translate(-0.5, 0.5)
self._transform = Affine2D().translate(-0.49999, -0.50001)
Copy link
Member

Choose a reason for hiding this comment

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

If anything deserves a comment, this does. This just doesn't give me any sort of "warm fuzzies"....

Copy link
Member Author

Choose a reason for hiding this comment

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

Just added a comment in a new commit.

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

Ok -- one of my changes didn't make it in. Please try again now -- sorry for the wild goose chase.

@WeatherGod
Copy link
Member

Woot! Success! It looks perfect in show() (GTKAgg and TkAgg), png, ps, pdf and svg.

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

Great! Merging...

mdboom added a commit that referenced this pull request Feb 29, 2012
@mdboom mdboom merged commit e270359 into matplotlib:v1.1.x Feb 29, 2012
@WeatherGod
Copy link
Member

Looks like merging this into master needs some work. This is holding up the merging of the doc fixes as well.

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

I'll take a look

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

Seems to be resolved now.

@WeatherGod
Copy link
Member

did you push your changes up? I don't see any change

@mdboom
Copy link
Member Author

mdboom commented Feb 29, 2012

I see it now...

@WeatherGod
Copy link
Member

So do I, and I now have that all taken care of. Thanks!

@mdboom
Copy link
Member Author

mdboom commented Mar 1, 2012

As @leejjoon pointed out in #726, this introduces an offset in the markers. It also introduces an offset in the clipbox. Working on this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants