Fix #5895: Properly clip MOVETO commands #5911

Merged
merged 7 commits into from Feb 22, 2016

Conversation

Projects
None yet
3 participants
Owner

mdboom commented Jan 25, 2016

I haven't run the tests on this yet... we'll see what Travis CI thinks.

@mdboom mdboom Fix #5895: Properly clip MOVETO commands
1d52371

mdboom added the needs_review label Jan 25, 2016

mdboom referenced this pull request Jan 25, 2016

Closed

Polar Projection PDF Issue #5895

Member

WeatherGod commented Jan 25, 2016

Travis failed with lots of small differences.

mdboom added some commits Jan 26, 2016

@mdboom mdboom Handle paths with a single moveto point 4e5e624
@mdboom mdboom Remove unused members
cf78dc1
@mdboom mdboom Fix backward compatibility
4ab4f0e
@mdboom mdboom Fix test for new behavior
37705f3
@mdboom mdboom Update log_scales SVG
0c6376f
@mdboom mdboom Further simplification
32a8ea1
Owner

mdboom commented Jan 27, 2016

This is passing Travis now and ready for final review.

@tacaswell tacaswell commented on the diff Feb 2, 2016

src/path_converters.h
}
while ((code = m_source->vertex(x, y)) != agg::path_cmd_stop) {
- if (code == (agg::path_cmd_end_poly | agg::path_flags_close)) {
+ switch (code) {
+ case (agg::path_cmd_end_poly | agg::path_flags_close):
@tacaswell

tacaswell Feb 2, 2016

Owner

what does this line do? My attempts to understand this by writing code have not gone well

// switch_statement1.cpp
#include <iostream>

enum test_e
  {
    A, B, C
  };



void helper(test_e inp)
{
  switch (inp)
  {
  case (A | B):
    std::cout<< "One"<< std::endl;
    break;
  case C:
    std::cout<< "Three"<< std::endl;
    break;

  default:
    std::cout<< "NONE?!"<< std::endl;
  }
}

int main() {
  helper(A);
  helper(B);
  helper(C);

}

gives

22:07 $ ./a.out
NONE?!
One
Three
@tacaswell

tacaswell Feb 2, 2016

Owner

with some digging I now see that this filtering with two things that just look like they are members of the same enum set.

@mdboom

mdboom Feb 2, 2016

Owner

The value agg::path_cmd_end_poly | agg::path_flags_close is how Agg indicates the end of a path that is also closed. Why that's not a single enum value, I don't know, but that's how it's done in Agg so we must follow. If it makes the code clearer I could create a new alias:

end_and_close_poly = agg::path_cmd_end_poly | agg::path_flags_close
@tacaswell

tacaswell Feb 2, 2016

Owner

Nah, I am just on a wading-into-c-code-I-do-not-understand kick the last few days.

@tacaswell tacaswell commented on the diff Feb 2, 2016

src/path_converters.h
m_moveto = true;
m_source->rewind(path_id);
}
+ int draw_clipped_line(double x0, double y0, double x1, double y1)
+ {
+ unsigned moved = agg::clip_line_segment(&x0, &y0, &x1, &y1, m_cliprect);
+ // moved >= 4 - Fully clipped
+ // moved & 1 != 0 - First point has been moved
+ // moved & 2 != 0 - Second point has been moved
+ if (moved < 4) {
+ if (moved & 1 || m_moveto) {
+ queue_push(agg::path_cmd_move_to, x0, y0);
@tacaswell

tacaswell Feb 2, 2016

Owner

We need some way to mark that this move is 'synthetic' so that the logic an line 366 knows not to re-set the m_initX / m_initY values. I think that would allow us to clip polygons and fix #5948 more gracefully.

@tacaswell tacaswell added a commit that referenced this pull request Feb 22, 2016

@tacaswell tacaswell Merge pull request #5911 from mdboom/clipping-fix
Fix #5895: Properly clip MOVETO commands
6922209

@tacaswell tacaswell merged commit 6922209 into matplotlib:master Feb 22, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tacaswell removed the needs_review label Feb 22, 2016

@tacaswell tacaswell added a commit that referenced this pull request Feb 22, 2016

@tacaswell tacaswell Merge pull request #5911 from mdboom/clipping-fix
Fix #5895: Properly clip MOVETO commands
 Conflicts:
	lib/matplotlib/tests/baseline_images/test_axes/log_scales.svg

	kept master version of file
512c8d0
Owner

tacaswell commented Feb 22, 2016

back ported as 512c8d0

@tacaswell tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 22, 2016

@tacaswell tacaswell Merge pull request #5911 from mdboom/clipping-fix
Fix #5895: Properly clip MOVETO commands
 Conflicts:
	lib/matplotlib/tests/baseline_images/test_axes/log_scales.svg

	kept master version of file
6569980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment