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

Port of aggdraw to agg 2.4 #50

Merged
merged 5 commits into from
Jul 28, 2019
Merged

Port of aggdraw to agg 2.4 #50

merged 5 commits into from
Jul 28, 2019

Conversation

dov
Copy link
Contributor

@dov dov commented Jan 21, 2019

  • Move agg2 source directory to agg
  • Update agg to 2.4
  • Update aggdraw source to match and work with agg 2.4 interfaces
  • Update tests to work with new code

- Move agg2 source directory to agg
- Update agg to 2.4
- Update aggdraw source to match and work with agg 2.4 interfaces
- Update tests to work with new code
@dov dov requested a review from mraspaud as a code owner January 21, 2019 19:59
@karimbahgat
Copy link

Is this the same as #48, which passed, but redone from scratch to make sure which agg version was the starting point (as discussed in #48)?
Any clue why appveyor failed but travis not (what's the difference)?

@djhoese
Copy link
Member

djhoese commented Feb 5, 2019

@karimbahgat Yes this is a copy of #48, if you read the discussion there you'll see that @dov needed/wanted to start over to clean things up. As for appveyor failing and travis not, appveyor is for Windows testing. If you look at the logs it seems appveyor is using a bad compiler (or something) and doesn't recognize the C++ syntax for ::. I'm not a C++ expert, but I think this syntax has been around for a while so not sure why the Windows compiler isn't recognizing it.

@dov ideas?

@dov
Copy link
Contributor Author

dov commented Feb 5, 2019

Ok. I seem to have fixed it. The problem is that windows.h by default defines macro for min() and max() that messes up the use of these functions in STL. You can prevent windows.h from defining these macros by a define. And this indeed seems to have solved the Windows compilation problem.

@djhoese
Copy link
Member

djhoese commented Feb 6, 2019

@dov Any idea why agg main would not have run in to this issue?

@dov
Copy link
Contributor Author

dov commented Feb 6, 2019

@djhoese What exactly do you mean by agg main? If you mean the agg C++ package, then I guess that they defined this variable in their visual studio settings, or cmake, or whatever they use for compiling under Windows. This is part of the windows compilation heuristics you learn to live with. Just do a google search for "NOMINMAX windows" and you'll see what I mean. In short, don't worry too much about it. :-)

@dov
Copy link
Contributor Author

dov commented Feb 6, 2019

Thinking about it again, you can also define it in setup.py if you know how to create a CPPDEFINE for windows compilations. Personally I just found it easier to do it in the C++ source code.

@djhoese
Copy link
Member

djhoese commented Feb 6, 2019

Yeah that's what I meant. I was initially alarmed by you modifying the headers files. I'm not that concerned about it so no need to change it. However, I do have some macro definitions in the setup.py already for the version number.

Hhhmmm maybe I'll try to do that on your branch unless you have time to tackle it.

@dov
Copy link
Contributor Author

dov commented Feb 7, 2019

Ok. I moved the define to setup.py . Let's see if it passes the windows checks. (I admit that I don't have a working windows compilation environment for python modules on...)

@dov
Copy link
Contributor Author

dov commented Feb 7, 2019

I have to admit that I don't understand these regression test failures. Import of numpy failed? What does that have to do with us?

@djhoese
Copy link
Member

djhoese commented Feb 7, 2019

Looks like the conda-forge package is broken. There are multiple issues related to it like conda-forge/numpy-feedstock#127

@djhoese
Copy link
Member

djhoese commented Feb 7, 2019

I've restarted the failing tests just to see if they pass now. Unlikely, but easiest way of "doing something" without doing anything.

@djhoese
Copy link
Member

djhoese commented Feb 7, 2019

@djhoese
Copy link
Member

djhoese commented Feb 7, 2019

Fixed!

@djhoese djhoese mentioned this pull request Feb 7, 2019
# Conflicts:
#	CHANGELOG.md
@a-hurst
Copy link
Contributor

a-hurst commented Mar 15, 2019

This is great, fantastic work!

One small concern with the pull request in its current state: right now it behaves similarly to the old dov agg2.4 version in that it changes the default line-join and line-cap styles to “rounded”, which means that merging this request would cause unexpected visual changes in projects written with the existing aggdraw version (e.g. I know aggdraw was/is popular among some experimental psychologists for generating visual stimuli in Python experiments, and the rounding behaviour change would mean plenty of experiments suddenly looking different). Could I request that the defaults be changed back to the original line-join/line-cap behaviour for the sake of backwards compatibility?

@djhoese
Copy link
Member

djhoese commented Jul 28, 2019

Ok, it's been long enough. I'm merging this to master. We'll wait a week to let anyone who would like to test off of the master branch do that, then I'll make a release and we'll see how many people have problems. Theoretically the agg C++ library should only fix functionality in newer versions, right? ...right?!?! That's how all software development goes. 😨

@djhoese djhoese merged commit 14960d7 into pytroll:master Jul 28, 2019
@a-hurst
Copy link
Contributor

a-hurst commented Jul 29, 2019

Main issue I'm foreseeing here is that the default line join and line cap type changes from square to rounded with the new 2.4 changes, unexpectedly changing previous default behaviour. It's also not clear from the docstrings or code what the different possible values/arguments for the 'linejoin' and 'linecap' args actually are, making the change extra disruptive since it's not obvious how to revert to the old behaviour.

Sometime within the next week, hopefully before the next PyPi wheels are uploaded, I'll open an issue with some screenshots demonstrating the difference, and do some more extensive testing with my aggdraw-dependent package to see if there are any other issues (everything else seemed to work fine last time I tested the 2.4 branch with it, though).

@djhoese
Copy link
Member

djhoese commented Jul 29, 2019

@a-hurst I'm so sorry. I didn't see your comment from March. It would be great if you could make an issue mapping out the differences. If it is easy enough to set the defaults to the old behavior then I'm fine doing that (@mraspaud thoughts?).

@a-hurst What are your thoughts on a deprecation cycle? We could either have a function for "preserve legacy defaults" or in the other direction "use future defaults". Or, less likely, remove the default cap style and make users have to choose one...no that's a bad idea.

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

Successfully merging this pull request may close these issues.

None yet

4 participants