Skip to content
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

Fix draw polygon ; tests & code cleanup #515

Merged
merged 22 commits into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@e1000
Copy link

e1000 commented Aug 30, 2018

[edit] Progress on Fix #234 and #313

I did not succeed in solving the bug(s), but I hope I worked out the problem a bit through the tests
which then adds two more failing tests:

[edit] The solution is to just draw horizontal lines all the time;
I think, it should not be too expensive on performance on average...

  • add draw_test.DrawPolygonTest class
  • this includes tests for the issues #234 & #313 (which both fail as expected)
  • some polygon C-code simplifications
  • remove duplicated equality testing in drawhorzlineclip / drawhorzline C-code

@e1000 e1000 changed the title Draw polygon : tests & code cleanup Fix draw polygon ; tests & code cleanup Aug 30, 2018

e1000 added some commits Aug 31, 2018

e1000
draw polygon : handle horizontal lines outside main loop
* this should be better for performance

# __doc__ (as of 2008-08-02) for pygame.draw.polygon:
RED = (255, 0, 0)

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

pygame.Color('red') is a good alternative to these color constants.

This comment has been minimized.

Copy link
@e1000

e1000 Aug 31, 2018

Author

done

self.assertEqual(self.surface.get_at((5, y)), RED)

def test_draw_symetric_cross(self):
# issue #234 : the result is/was different wether we fill or not

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

This comment could be a docstring.

self.assertEqual(self.surface.get_at((x, y)), RED)

def test_illumine_shape(self):
# Extracted from bug #313

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

A docstring here of what is being tested would be useful.

# 1. one-pixel-high, filled
pygame.draw.rect(self.surface, RED, (0, 0, 10, 10), 0)
pygame.draw.polygon(self.surface, GREEN, [(x, 2) for x, y in CROSS], 0)
for x in range(7):

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

7 could be put into a variable name.


def test_draw_square(self):
pygame.draw.polygon(self.surface, RED, SQUARE, 0)
# note : there is a discussion (#234) if draw.polygon should include or

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

This comment could go in the doc string too.

This comment has been minimized.

Copy link
@e1000

e1000 Aug 31, 2018

Author

done

return;
}

/* special case : horizontal border lines with miny < y < maxy */

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

I don't understand this part. What is this special case about exactly?

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

Very nice set of tests and bug fixes :)

One possible improvement to this function, would be improving the naming of the variables.

int *vx, int *vy
polyints[ints++] = (y - y1) * (x2 - x1) / (y2 - y1) + x1;
}
else if ((y == maxy) && (y > y1) && (y <= y2)) {
if ( ((y >= y1) && (y < y2)) ||

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

What is being checked here?

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

I think once the variables get longer names this expression will be more clear.

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

It looks like the C code needs to be run through clang-format.

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Aug 31, 2018

I also consider that the whole draw.c need some documentation and better variable naming.
I'll start doing my best with this algorithm ...

e1000
@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Aug 31, 2018

better now or too much comments ? I guess it's a standard algorithm, buy anyway

}
else if ((y == maxy) && (y > y1) && (y <= y2)) {
polyints[ints++] = (y - y1) * (x2 - x1) / (y2 - y1) + x1;
if ( ((y >= y1) && (y < y2)) ||

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

One way to make expressions a bit smaller is to give names to their components.

bool y_crosses_edge = ((y >= y1) && (y < y2))
bool on_lowest_line = ((y == maxy) && (y2 == maxy))
if (y_crosses_edge || on_lowest_line) {

However, I wonder if C compilers are smart enough to opitimze this well... ?

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

Thanks for that. It's waaaaaay more readable now.

Yeah definitely, the whole draw.c could use some improvements. Since you had that function in your head, I thought you would be able to make it easier to understand for the next person.

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

I asked in the Discord chat room if someone is available to read the cleaned up function.

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Aug 31, 2018

Yes indeed, it took me a while to understand it, so I could document the algorithm indeed!

I had a quick search on the net, and it seems the algorithm we use is intended for float point corners...
Also , for debugging & understanding, I started translating the algorithm into Python. Are we interested to save that kind of stuff somewhere?

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

Oh, a python implementation is nice :)

Not sure where to put it though... How about you put it in a gist, and then link to the gist near the C function?

int minx, maxx;

/* Determine X bounds */
minx = point_x[0];

This comment has been minimized.

Copy link
@illume

illume Aug 31, 2018

Member

I think these types can be declared on the same line as they are assigned.

int minx = point_x[0];
int maxx = point_x[0];
@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

Looks like it broke in appyveyor Visual C at some point a while ago.

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Aug 31, 2018

Sorry, I do not understand the broken appyveyor Visual C ; what is "a while ago", before this branch?

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Aug 31, 2018

python-code : Not sure what a "gist" is.
We have ./test/test_utils/png.py , (python implementation of png , and it's only used in tests...) maybe that's the right place for that kind of stuff?

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

A gist is a paste bin type place on github. You can put code there that is also versioned.
https://gist.github.com/

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

If you look at the commits tab of this PR, you can see each time appveyor compiled and ran the tests. So, somewhere between the last time it passed, and the first time it failed is where the error was introduced.

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

It looks like the old visual c is having problems with the type declaration here:

src_c/draw.c(1610) : error C2143: syntax error : missing ';' before 'type'
src_c/draw.c(1612) : error C2065: 'n_intersections' : undeclared identifier
src_c/draw.c(1634) : error C2065: 'n_intersections' : undeclared identifier
src_c/draw.c(1638) : error C2065: 'n_intersections' : undeclared identifier
src_c/draw.c(1640) : error C2065: 'n_intersections' : undeclared identifier

int n_intersections;

https://stackoverflow.com/questions/18912912/syntax-error-missing-before-type-in-c
So, that variable should be declared at the top of the scope.

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

CPython has a policy now of having python versions of all new C modules. It's a very useful thing to have for testing, and debugging... as you've shown. So perhaps it should go in src_py/draw_py.py

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Aug 31, 2018

alright, I like also the idea to have it in src_py ; no risk of confusion with other python modules? maybe a subfolder?

I've put int n_intersections = 0; inside the loop; I hope it makes appveyor happy...

@illume

This comment has been minimized.

Copy link
Member

illume commented Aug 31, 2018

If it goes in src_py/ then it can't be called draw.py. Because then there could be a confusion on which module to load... the draw.so/dll or the draw.py. There is a convention that the 'c' module is called something like _draw.so, with the leading underscore. But in this case we can not have the draw.py be there because it does not do imports from the _draw.so. So calling it something like draw_py.py could be a good temporary name. In the future perhaps the draw.py would import from _draw.c the functions it needs.

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Sep 3, 2018

alright, python draw code will go into another branch...

@illume

This comment has been minimized.

Copy link
Member

illume commented Sep 3, 2018

Cool, thanks. Let's merge this one in then!

@illume illume merged commit 5590709 into pygame:master Sep 3, 2018

2 checks passed

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

@illume illume referenced this pull request Oct 16, 2018

Closed

1.9.5 release notes. #561

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.