Skip to content

Draw markers around center of pixels #5603

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 2 commits into from
Dec 3, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Dec 2, 2015

When the snapping of circle markers was disabled in 37927b2, in order to make them look more round, it also made them half a pixel off of center (since the snapping was no longer happening). This shifts non-snapped markers by half a pixel to keep (0, 0) running down the middle of the pixel.

BEFORE THIS PR:

old

AFTER THIS PR:

new

As a procedural point, we should try to get this in before #5307, as it will change many of the baseline images (for the better, we hope).

@mdboom mdboom added this to the Critical bugfix release (1.5.1) milestone Dec 2, 2015
@WeatherGod
Copy link
Member

Would probably be good to have some tests specifically for this. Different markers, different sizes, different locations and at different resolutions. Does shifting it half a pixel only impact the visual result if rounding would have been an issue?

I also wonder if a similar approach might solve the contourf problem?

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

Would probably be good to have some tests specifically for this. Different markers, different sizes, different locations and at different resolutions. Does shifting it half a pixel only impact the visual result if rounding would have been an issue?

Once the tests are lowered to zero tolerance in #5307, the existing tests should do that and detect changes of this magnitude. And there are already tests that test all of the different markers (test_axes.test_marker_styles). I think it best not to add additional tests in this case.

I also wonder if a similar approach might solve the contourf problem?

I don't think so. I've already experimented with snapping to solve that problem some time ago, and it doesn't help.

@tacaswell
Copy link
Member

Does this address #3400 ?

@WeatherGod
Copy link
Member

And when we lower the tolerance to zero, and images are remade en masse, are we going to be able to verify every single marker is drawn correctly? Also, we don't have all that many tests that use different resolutions?

I would love to see test images made at fairly low resolutions that draws lines like you have above and plots markers at their intersections. Having it at a low resolution would make it visually obvious that something is wrong.

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

And when we lower the tolerance to zero, and images are remade en masse, are we going to be able to verify every single marker is drawn correctly?

Yes. That was why I built the comparison tool in #5521, to make that kind of thing much easier.

Also, we don't have all that many tests that use different resolutions?

That's true.

I would love to see test images made at fairly low resolutions that draws lines like you have above and plots markers at their intersections. Having it at a low resolution would make it visually obvious that something is wrong.

That's a good point.

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

Does this address #3400 ?

I think not, unfortunately -- addressing that requires the line and the marker line to know about each other and snap in sync. We don't really have that kind of global knowledge. But #3400 only really becomes a problem if the user customizes the line width or marker edge width to be a mismatch of even/odd sizes.

@QuLogic
Copy link
Member

QuLogic commented Dec 2, 2015

Was test_axes/stackplot_test_baseline.png really supposed to change here?

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

Was test_axes/stackplot_test_baseline.png really supposed to change here?

Yep. For reasons that are rather obscure, stackplots end up going through the draw_marker code path on Agg.

@WeatherGod
Copy link
Member

So, the stackplots are polygons, right? Would it make sense to treat
contourf polygons similarly?

I am being overly hopeful, but the stackplots do look better now, so I just
wanted to make sure that this possibility has been considered.
On Dec 2, 2015 6:27 PM, "Michael Droettboom" notifications@github.com
wrote:

Was test_axes/stackplot_test_baseline.png really supposed to change here?

Yep. For reasons that are rather obscure, stackplots end up going through
the draw_marker code path on Agg.


Reply to this email directly or view it on GitHub
#5603 (comment)
.

tacaswell added a commit that referenced this pull request Dec 3, 2015
FIX: Draw markers around center of pixels
@tacaswell tacaswell merged commit 9a7dfd7 into matplotlib:master Dec 3, 2015
tacaswell added a commit that referenced this pull request Dec 3, 2015
FIX: Draw markers around center of pixels
@tacaswell
Copy link
Member

backported to 1.5.x as 34c7dad

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.

4 participants