FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

improve snapping algorithm by leejjoon · Pull Request #726 · matplotlib/matplotlib · GitHub

Repository navigation

improve snapping algorithm - #726

Closed
leejjoon wants to merge 2 commits into
matplotlib:masterfrom
leejjoon:improve-agg-snapping
Closed

leejjoon wants to merge 2 commits into
matplotlib:masterfrom
leejjoon:improve-agg-snapping

Conversation

Copy link
Copy Markdown
Contributor

I think the current snapping gives somewhat wrong result.

            *x = mpl_round(*x) + m_snap_value;      
            *y = mpl_round(*y) + m_snap_value;

For example, consider m_snap_value=0.5 and (x, y) = (0.45, 0.55). This is supposed to be snapped to (0.5, 0.5) but the above code gives (0.5, 1.5). And I think the right thing to do is subtract m_snap_value before rounding.

             *x = mpl_round(*x-m_snap_value) + m_snap_value;
             *y = mpl_round(*y-m_snap_value) + m_snap_value;

Copy link
Copy Markdown
Contributor Author

@mdboom: Can you check this?

mdboom commented Feb 28, 2012

Copy link
Copy Markdown
Member

Looks right. Interestingly, it doesn't seem to cause any additional test failures -- but as we drift further and further from the baseline images, we'll probably want to update them at some point.

Copy link
Copy Markdown
Member

Do we want this for v1.1.1?

mdboom commented Feb 28, 2012

Copy link
Copy Markdown
Member

Given that 1.1.1 is imminent, I would think no. This feels like a pretty pervasive change to me -- I would hate to later discover things like images aligning correctly to the edges of the axes breaks in some corner cases because of this change.

Copy link
Copy Markdown
Contributor Author

The commit f4f2a1a introduces one pixel offset between lines and markers for integer coordinates, e.g., (0, 0). I believe that this is related to how maker paths are snapped.

    snap_t             marker_path_snapped(marker_path_transformed,
                                           gc.snap_mode,
                                           marker_path.total_vertices(),
                                           gc.linewidth);

If we consider a marker path (0, 0.5) - (0, 1.5) and linewidth of 1, I believe that the path actually snapped and becomes (-0.5, 0.5) - (0.5, 0.5) as round(-0.5) returns -1, while it is supposed to (or used to) become (0.5, 0.5) - (0.5, 1.5).

I think one option is just to translate the marker path by (0.5, 0.5) before the snapping (cbc5fed). But @mdboom may have different (and better) idea.

mdboom commented Feb 29, 2012

Copy link
Copy Markdown
Member

Closing -- this was merged as part of #695.

mdboom closed this Feb 29, 2012

leejjoon commented Mar 1, 2012

Copy link
Copy Markdown
Contributor Author

I don't see these changes in #695. Did you mean that #695 also solves the current issue? Just in case, with the changes in this PR applied, it seems that #695 still shows 1 pixel offset.

mdboom commented Mar 1, 2012

Copy link
Copy Markdown
Member

Ok -- I see. This PR was incorporated in #695, but I missed the fact that it had problems. I'm looking into this now and am reopening #695 to do the work over there.

EDIT: @leejjoon: you can see a solution to the problems presented here on top of #695 in #734. Can you test with all of your use cases? It seems to a) have pixel markers align with the ticks correctly, b) have the ticks align with the axes rectangle correctly, c) have the clipbox align with the axes rectangle, d) not distort the shape of the markers and e) makes evenly spaced data look reasonably even.

mdboom mentioned this pull request Mar 1, 2012
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

3 participants


Back | FazBrowse Home | New Git URL