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

Raise TypeError on invalid point arguments (fix #521) #558

Closed
wants to merge 3 commits into from
Closed

Raise TypeError on invalid point arguments (fix #521) #558

wants to merge 3 commits into from

Conversation

@llenck
Copy link

@llenck llenck commented Oct 14, 2018

Raises TypeError and not ValueError as said in issue to be consistent with the error that is raised by the python algorithm.

@illume illume mentioned this pull request Oct 16, 2018
4 tasks done
@illume
Copy link
Member

@illume illume commented Oct 16, 2018

Thanks for this :)

One improvement could be to make the repeated code into a function so it's not repeated many times.

I'm a bit worried that these extra checks could cause performance issues because we have to iterate through the lines multiple times. But I guess it's better to be correct.

@illume illume changed the base branch from master to subtypemath Oct 16, 2018
@illume illume changed the base branch from subtypemath to master Oct 16, 2018
@illume
Copy link
Member

@illume illume commented Oct 16, 2018

I just noticed that this fixes line and polygon drawing. However there's only one test added. Would you be able to please add a test for these too?

@llenck
Copy link
Author

@llenck llenck commented Oct 16, 2018

Oh wow, until I checked it just now I thought that variable sized arrays on the stack weren't a thing in C and that malloc/free would be more expensive than this solution. But since I was wrong at least about the first one, I guess I'll change that code up since that seems to be a solution that is probably a lot faster, and I doubt that a few python objects will blow the stack up. Should I make a new PR for this?

Edit: github for some reason didnt show your latest comment.
Yeah, since the code was pretty similar in all 3 cases I though testing one would be enough but looking back that was just being lazy on my part after staying up too long :/

@illume
Copy link
Member

@illume illume commented Oct 16, 2018

Sorry, I'm a bit confused about what you mean about the stack?

I guess the logic would be quite complicated if instead of looping over the points twice, the error checking is done in line? Then you'd have weird half drawn shapes I guess... not good.

@llenck
Copy link
Author

@llenck llenck commented Oct 16, 2018

No, what I mean is having something like an int x_coods[n];, where the values for x are stored during the check and then later accessed from instead of getting them again from the python object. Of course, if someone passes too much points at once to this, this will consume a lot of memory on the stack, but I don't think it would cause a stack overflow, modern systems usually have at least a few MB of stack size by default and each pair of coordinates will usually use 8 bytes of memory.

Edit: I need to sleep more, didn't look enough through the existing code. Sorry, please ignore some of the stuff I wrote before, didn't see that there were already lists for storing x/y coords in the polygon drawing function; I'll commit tomorrow to use them instead of always getting new values from the python object. There's no such list/array for the aalines/lines functions though, I'll see if I can implement something readable and faster tomorrow for them. :/

@illume
Copy link
Member

@illume illume commented Oct 17, 2018

Ah ok. Thanks for the explanation. Sounds like you've found a bug there with huge amounts of points.

So coming up with a list of potential improvements...

To get this merged in right away...

  • add similar unit test for other functions that were modified.

Nice to have (perhaps in another PR)...

  • time functions with 100, and 1000 points to compare the speed.
  • look at speeding things up, iff it is slow.
@illume
Copy link
Member

@illume illume commented Oct 17, 2018

@IchMageBaume @Nunu-Willump can you please check your commits are coming in under the correct account?

@llenck
Copy link
Author

@llenck llenck commented Oct 17, 2018

Huh, seems like git decided to use my old account after I updated it yesterday. I'll look into why that might be and try to fix it, but does it matter?

@illume
Copy link
Member

@illume illume commented Oct 17, 2018

@llenck
Copy link
Author

@llenck llenck commented Oct 17, 2018

Thanks for telling me. Changed my ~/.gitconfig to use the newer one now, but as it doesn't really matter to me I'm OK with it being merged like it is now :p

@e1000
Copy link

@e1000 e1000 commented Oct 18, 2018

Hello,

sorry, I just realized I did a duplicate PR ...

  • I've used everywhere the xlist and ylist variables, so I can avoid this block :
    item = PySequence_GetItem(points, 0);
    result = pg_TwoFloatsFromObj(item, &x, &y);
    Py_DECREF(item);

I'm not sure about the performance of doing the loop two times. I thought, the steps done for each separate lines (and much more drawing the whole polygon) are much longer than repeating the loop.

I thought to a macro for this repeated code, but it's tricky to say the least since aalines uses float coordinates (at least now : see #522) ...

Also, I added a macro for checking the colors.

@e1000 e1000 mentioned this pull request Oct 18, 2018
@llenck
Copy link
Author

@llenck llenck commented Oct 18, 2018

Closing since PR #567 does the job better

@llenck llenck closed this Oct 18, 2018
@illume illume reopened this Oct 19, 2018
@illume
Copy link
Member

@illume illume commented Oct 19, 2018

I think what we'll do is merge this PR first, and then merge the other one on top of it.

@e1000
Copy link

@e1000 e1000 commented Oct 19, 2018

I agree to merge this first. I basically just save data and then do some cleanup after it. Plus, he was there before :-)

@llenck
Copy link
Author

@llenck llenck commented Oct 21, 2018

Maybe I'm missing something obvious here, but why are we waiting with merging these 2 PRs?

@e1000
Copy link

@e1000 e1000 commented Oct 22, 2018

what you might miss is that @illume might just be busy elsewhere :)

It's a big project, and AFAIK the bottleneck of the project is currently migration to SDL2 ...

  • project management & packaging ...
@illume
Copy link
Member

@illume illume commented Oct 23, 2018

This was merged in here: #578

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

Successfully merging this pull request may close these issues.

None yet

4 participants