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

Change default linejoin and linecap to preserve previous behavior #62

Merged
merged 17 commits into from
Nov 6, 2019

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Aug 19, 2019

Closes #61. This changes the default line join and cap types to miter and butt which preserves the previous Agg settings. @dov any idea/reason why these defaults were changed when switching to agg 2.4? Also, @a-hurst please review.

I got the mapping of the constants here since the main antigrain website seems broken. Using raw enum integers this way is not the most flexible or backwards compatible. However, mapping strings to the constants is not the most straight forward thing to do in C++ so I will wait until we migrate to cython to change this to accept ints (backwards compatible) or strings for the names of the join and cap types. I did realize that #3 was doing this string checking (FYI @ simonkolotov), but since it looks like the API in 2.4 changed quite a bit, I didn't really have time to adjust that PR to work for this. This at least preserves the current behavior.

@djhoese djhoese added the bug label Aug 19, 2019
@djhoese djhoese requested a review from mraspaud as a code owner August 19, 2019 14:28
@djhoese djhoese self-assigned this Aug 19, 2019
@djhoese
Copy link
Member Author

djhoese commented Aug 21, 2019

FYI the failing appveyor is a problem with ci-helpers. This won't be an issue in a couple days when my fixes to ci-helpers are merged.

@a-hurst
Copy link
Contributor

a-hurst commented Aug 24, 2019

It looks like the line_cap line still needs to be changed to revert to the previous behaviour. Otherwise, this looks good! The new docstrings are great.

@djhoese
Copy link
Member Author

djhoese commented Sep 23, 2019

Sorry, I completely forgot about this. Should be complete. Will wait for tests.

@djhoese
Copy link
Member Author

djhoese commented Sep 23, 2019

@a-hurst Any chance you're a powershell expert? The appveyor builds are failing because (I think) the if statements are running commands like conda activate test in a separate scope that isn't affecting the rest of the script activation. This is different than how conda used to be activated which I think was call activate test.

@a-hurst
Copy link
Contributor

a-hurst commented Sep 24, 2019

@djhoese unfortunately not, I'm very much a UNIX person so I really had to fumble my way through setting up appveyor to get aggdraw builds working originally. That said, maybe it's worth a shot putting the "%CMD_IN_ENV%" in front of the "conda activate test" line, or alternatively putting the "call" in front of "conda activate test"? The only way I managed to get it working in the first place was a lot of trial and error.

Edit: Also, just looked at the files changed in the pull request and noticed the new sum value for all the pixels in test_graphics2() in the selftest.py file (51200). Curious, I looked back at the original selftest.py from before the 2.4 pull request was merged and noticed it was 50800. Given that line joins/ends are now the same, it looks like that confirms my finding in #60 that shapes are being drawn slightly larger following the update. @dov, any idea what changes on the back end from agg 2.2 to 2.4 might have caused this?

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a comment on the ci-helpers

ci/travis-install.sh Outdated Show resolved Hide resolved
@djhoese djhoese merged commit 6c1cf87 into pytroll:master Nov 6, 2019
@djhoese djhoese deleted the bugfix-linejoin branch November 6, 2019 20:28
@djhoese
Copy link
Member Author

djhoese commented Nov 6, 2019

@a-hurst Now that this is merged we should see how different our various use cases are. I will try running some of the pytroll package tests (pycoast) with the master branch of aggdraw and see if/how many of our tests fail.

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

Successfully merging this pull request may close these issues.

Default drawing behaviour different after merging agg2.4
3 participants