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

Clean aaline #596

Merged
merged 15 commits into from Nov 5, 2018

Conversation

Projects
None yet
2 participants
@e1000
Copy link

e1000 commented Nov 3, 2018

  • much of variable renaming in aaline to get things clearer
  • a aaline algorithm in Python
  • little draw_test.py cleanup
  • make 2 aaline tests pass for Python
@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Nov 3, 2018

I don't know if it's considered a good practice in C to return inside a loop like a did in draw.c around lines 838-842 and 899-902 : did it because it saves one variable definition ...

@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Nov 3, 2018

More remarks:

  • I dropped the half-integers in the Py-algo
  • I have to add + 1 at the end of the line to get anything reasonable ...
  • going through the algorithm, I don't understand the usage of trunc, it seems to me, it should be ceil or floor (cf my Python-version of the algo)
  • I'll have to fix some more tests (cf #522)
@illume

This comment has been minimized.

Copy link
Member

illume commented Nov 4, 2018

  • trunc always rounds towards zero, so for negative numbers it's different to ceil or floor. dlon tested before that perhaps trunc is slower than a simple int() cast.
  • Usually it's good to only return in one place in a function... if possible. The reasoning is that otherwise it's easy to miss the places where the function could return. However in this case, they are very close to each other.
@e1000

This comment has been minimized.

Copy link
Author

e1000 commented Nov 4, 2018

Can somebody help with the failing tests? it's strange messages from apple, and I'm more about piguins...

@illume

This comment has been minimized.

Copy link
Member

illume commented Nov 4, 2018

Looks like the mac build bots were having issues. Restarted the jobs, and they don't fail now.

@illume illume merged commit 2b2dfb3 into master Nov 5, 2018

4 checks passed

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

@illume illume deleted the clean-aaline branch Nov 7, 2018

@dlon dlon referenced this pull request Mar 23, 2019

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.