Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Convert Agg extension to pybind11#27011
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I rebased this on top of#27087 so that I can re-use the type casters that it has. However, there is still an issue with the |
@QuLogic When you have time and motivation to come back to this, ping me and I will review it. |
| auto should_simplify = src.attr("should_simplify").cast<bool>(); | ||
| auto simplify_threshold = src.attr("simplify_threshold").cast<double>(); | ||
| if (!value.set(vertices.ptr(), codes.ptr(), | ||
| if (!value.set(vertices.inc_ref().ptr(), codes.inc_ref().ptr(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know why this change is needed now, but otherwise things randomly crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We think there are decrements in the iterator destructor there is a decr which we think we need to keep as there are still other usages of the iterators from c++ that we need to remove first.
| should_simplify, simplify_threshold)) { | ||
| return false; | ||
| throw py::error_already_set(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was technically wrong before, but otherwise,pybind11 would substitute its own generic "cannot convert type PyType to C++Type" error message, so not fatal.
| /* value.clippath = src.attr("get_clip_path")().cast<ClipPath>(); */ | ||
| convert_clippath(src.attr("get_clip_path")().ptr(), &value.clippath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I leave the casting version in, then all sorts of memory corruption occurs, and I've yet to figure out why. Instead of wasting even more time on that now, I'd rather get this going, and once we drop NumPy headers, come back to clean this up properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
11ac056 to83a882bCompareI rebased and added the usual |
This is a _very_ straightforward port, and several parts can be cleanedup in future commits.
This only does the simple ones that are not shared with the pathextension. Those more complex ones will be done separately.
d4ea011 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
PR summary
This is somewhat partially implemented, in that the NumPy array parts are not changed yet. I think I'll want to do path first, to make this simpler. I'll leave this as draft until then.
PR checklist