Upgrade to newer agg version by jdeschenes · Pull Request #288 · enthought/enable

jdeschenes · 2017-08-07T02:23:42Z

This is a pretty big diff. I don't know if you guys will want to pursue this option. Essentially, there are several issues in enable that are caused by issues with the current agg version in enable. This could close:

This pull uses the same version of agg as celiagg fixes those issues. While the diff is rather large, it can be summarized and reviewed quite easily:

The folder agg-24 is identical to the one in celiagg(minus some examples/docs, they can be added)
Namespace agg24 was changed to agg(To make sure that the two agg were identical)
changes to kiva/agg/src/kiva_graphics_context.h transform_image_interpolate to make it compile
Changes signature in kiva/agg/src/kiva_graphics_context_base.h of rendering_buffer_ptr to rendering_buffer
Some unittest skip were removed(To account for the failure)
Some changes to unittest since the image generation is slightly different(It looks like they were improved actually)

I have tested this pretty extensively on my Windows machine and so far, there is only one small issue that I saw, where the image doesn't interpolate correctly. My guess is that it is caused by the change to kiva_graphics_context.h. I am sure you guys could spot quickly the error I made.

As far as I am aware, you guys were probably leaning toward using celiagg instead, but this pull could be a good stepping stone in that direction as they would now be sharing the same library underneath.

Let me know if you are interested in pursuing this.

jwiggins · 2017-08-07T13:38:06Z

I think this is really good idea for the medium term. Moving to celiagg is not as straightforward as it should be.

That said, the history of this branch looks a bit crazy. Can you rebase/redo to get rid of the commits from your python3_compat branch?

Also, the agg24 namespace is important. It's meant to keep enable's agg symbols from colliding with matplotlib's agg symbols when both libraries are used in the same process. Crazy, I know. It's a long term dream (pipedream?) of mine to get both libraries to use celiagg.

Also, it might be better to source this from matplotlib, since that's where I grabbed it from when building celiagg.

jwiggins · 2017-08-07T13:48:31Z

One more thing: You could make this diff a bit smaller by avoiding build scripts within agg (Makefile.* and CMakeLists.txt). They aren't used here anyhow. The relevant directories are:

kiva/agg/agg-24/font_freetype
kiva/agg/agg-24/font_win32_tt
kiva/agg/agg-24/include
kiva/agg/agg-24/src

Everything else should be ignored

* Now using the same version of AGG as matplotlib * Removed unittest skips that were hampered by a bug with the old agg version * Some small changes to the unit test as the rendering changed * Adapted function `void transform_image_interpolate` to make it compile * Changed signature of `agg24::rendering_buffer* graphics_context_base::rendering_buffer_ptr()` to `agg24::rendering_buffer& graphics_context_base::rendering_buffer()`

jdeschenes · 2017-08-08T15:20:39Z

I redid a new branch with the same version of AGG as matplotlib. This is a lot cleaner now.

jdeschenes · 2017-08-08T15:38:15Z

There are some failures in the travis build:

One is related to the PyQt Scrollbar
The others are related to the image interpolation. I think I will need your help on that one(As mentionned previously)

P.S. I will try to setup the appveyor and travis for mac os soon, to make sure it is being tested correctly.

jwiggins · 2017-08-08T18:46:57Z



            agg24::path_storage img_outline = img.boundary_path(img_mtx);
-            agg24::rendering_buffer* src_buf = img.rendering_buffer_ptr();


Right. So everywhere you used agg_pixfmt in this method, it appears you should have used other_format instead. Did that not compile for you?

I've tested this locally (macOS) and everything appears to be working as it always did.

The unit tests pass when I make this change

That's the first thing I tried but it doesn't compile on windows. I get a template error diarrhea. Here is an excerpt.

kiva\agg\src\kiva_graphics_context.h(711): note: while compiling class template member function 'kiva::graphics_context<agg24::pixfmt_abgr32>::graphics_context(unsigned char *,int,int,int,kiva::interpolation_e)' kiva\agg\agg_wrap.cpp(4177): note: see reference to function template instantiation 'kiva::graphics_context<agg24::pixfmt_abgr32>::graphics_context(unsigned char *,int,int,int,kiva::interpolation_e)' being compiled kiva\agg\src\kiva_graphics_context.h(618): error C2664: 'agg24::image_accessor_clip<agg24::pixfmt_rgb24>::image_accessor_clip(agg24::image_accessor_clip<agg24::pixfmt_rgb24> &&)': cannot convert argument 1 from 'agg24::pixfmt_abgr32' to 'agg24::pixfmt_alpha_blend_rgb<agg24::blender_rgb24,agg24::rendering_buffer,3,0> &'

Oups, brain fart. Looks like it works just fine. Thanks!

jwiggins · 2017-08-08T18:49:44Z

    def test_determinant(self):
        orig = array((1.0,2.0,3.0,1.0,4.0,5.0))
-        desired = -0.2
+        desired = -5.0


Why would this change?

In agg_trans_perspective.h they had the method determinant which was actually a determinant reciprocal. They fixed the method in the new version and added a new method for the reciprocal.

Crazy. Looks like the matrix inversion code also changed, which explains why none of the other unit tests broke...

jwiggins · 2017-08-08T19:08:55Z

@mdickinson: Once the kiva/agg/src/kiva_graphics_context.h changes are in place, would you like to try this to see if #283 is still reproduced?

jdeschenes · 2017-08-09T00:18:49Z

The image is working perfectly now!

There is a big issue with the scrolling though. I don't think it is related to my branch.

codecov-io · 2017-08-09T00:29:58Z

Codecov Report

Merging #288 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #288      +/-   ##
=========================================
+ Coverage   33.59%   33.6%   +0.01%     
=========================================
  Files         206     206              
  Lines       18278   18278              
  Branches     2407    2407              
=========================================
+ Hits         6140    6142       +2     
+ Misses      11745   11744       -1     
+ Partials      393     392       -1

Impacted Files	Coverage Δ
kiva/fonttools/font_manager.py	`55.23% <0%> (+0.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cb8dd4...f90758e. Read the comment docs.

jdeschenes · 2017-08-09T00:35:12Z

False alarm. Enable doesn't like it if using the mouse wheel while pyqt4 and pyqt5 are installed on the same environment.

jwiggins · 2017-08-11T09:30:00Z

I'm generally 👍 on this, but it would be nice if some others look into whether or not it actually solves the crashing bugs with image rendering. I hope that it does, because that would be awesome.

I'll be on vacation for the next three weeks, so if any Enthought persons feel reasonably confident about this, then merge away (don't forget to squash).

itziakos · 2017-08-18T11:04:44Z

Just a comment about licensing. As far as I understand, these changes have been adapted from the matplotlib fork of agg 2.4, correct? We should make sure we provide the appropriate attribution.

jvkersch · 2017-08-23T10:36:55Z

@itziakos I'm trying to figure out what the right approach is here in terms of licensing. Judging from the Matplotlib license (https://github.com/matplotlib/matplotlib/blob/master/LICENSE/LICENSE) we should be good to go if we reproduce the Matplotlib license along with the Agg source code (and perhaps add a clarifying note somewhere that the Matplotlib license applies only to Agg sources). Do you agree?

itziakos · 2017-08-23T11:44:33Z

My take on this is that the agg source at the matplotlib repo has a separate license https://github.com/matplotlib/matplotlib/blob/master/extern/agg24-svn/src/copying. I think we should append a copy of that license in our agg source license and mention in a note that changes added in this PR have been ported from the matplotlib fork of agg (link to matplotlib) and are subject to https://github.com/matplotlib/matplotlib/blob/master/extern/agg24-svn/src/copying

Maybe, we should check with @jwiggins and @corranwebster, they might have some more experience on such things.

corranwebster · 2017-08-23T12:34:51Z

I'm a little unclear on exactly what the origins of the code in this branch are, but I think it's something like:

take the matplotlib agg code
merge in some fixes from the old agg version that we shipped in Kiva
additional fixes as needed to get the kiva SWIG wrappers to work

If this is the case, then we need to:

include the file https://github.com/matplotlib/matplotlib/blob/master/extern/agg24-svn/src/copying
if the matplotlib team made any modifications to the agg code (which is currently unclear to me), then we also need to follow the matplotlib license
include a BSD license with appropriate contributors for the original fixes + new changes.

Does this seem reasonable/correct?

jdeschenes · 2017-08-23T13:19:17Z

The approach was:

Take the matplotlib code
Fix the namespacing issue(agg vs agg24)
Fix the wrapper

Also, there were extra modifications in the matplotlib repository.

jvkersch · 2017-08-31T16:15:34Z

This fixes enthought/chaco#339 for me (segfault before, nice image plot after), nice work!

As for licensing, since there are changes made by the matplotlib community, it does look like we need to include the matplotlib license as well (along with a brief description of the changes made in this PR). @corranwebster I'm not sure what your 3rd point would look like (BSD license with appropriate contributors...). Can you follow up on this?

corranwebster · 2017-08-31T16:31:26Z

Yes, I agree, looks like we need the matplotlib license. The 3rd point would just be a copy of the main Enable project LICENSE.txt with appropriate dates (ie. should probably update copyright 2006-17 or something).

I'd also suggest adding a README.txt to enable/kiva/agg with a brief explanation of the provenance of the code. That should hopefully cover the licensing aspects.

We can possibly do this in a separate PR as long as we are good about doing it immediately upon the merge of this code.

jvkersch · 2017-09-14T23:54:06Z

@jdeschenes Would you be able to take care of the remaining license issues?

jwiggins · 2017-09-26T19:05:39Z

For AGG, are we not already covered by https://github.com/enthought/enable/blob/master/kiva/agg/agg-24/copying?

Matplotlib did make changes to the AGG source code which we are getting here. So I think we now need to include https://github.com/matplotlib/matplotlib/blob/master/LICENSE/LICENSE as well. Where shall it go? Is it time to add a LICENSE directory to enable?

jvkersch · 2017-10-01T10:12:03Z

@jdeschenes @corranwebster @jwiggins I added the licensing info in a separate PR jdeschenes#1 against @jdeschenes' branch. Let me know what you think.

Licensing info for Agg-2.4

jdeschenes · 2017-10-02T15:14:12Z

It works for me... I just accepted your modifications.

corranwebster · 2017-10-02T16:02:34Z

Licensing LGTM. Unless anyone has objections, I think that this is good to go.

jwiggins · 2017-10-04T07:55:55Z

Thanks for clearing that up @jvkersch, and thanks to @jdeschenes for the upgrade!

jdeschenes force-pushed the agg_2_4 branch from d8b80dc to 15ae7c1 Compare August 8, 2017 15:19

jwiggins reviewed Aug 8, 2017

View reviewed changes

as per @jwiggins suggestion fixed the image interpolation error.

20984df

jwiggins mentioned this pull request Sep 25, 2017

FIX: Increment micro version number #292

Merged

jvkersch added 2 commits October 1, 2017 10:43

Update license date

6e5c175

MAINT: Add licenses.

c9e8eac

jvkersch mentioned this pull request Oct 1, 2017

Licensing info for Agg-2.4 jdeschenes/enable#1

Merged

Merge pull request #1 from enthought/agg_2_4-licenses

f90758e

Licensing info for Agg-2.4

jwiggins merged commit 606e69a into enthought:master Oct 4, 2017

This was referenced Oct 4, 2017

Segmentation faults when running test suite on OS X 10.12. #283

Closed

Kiva AGG renderer breaks when compiled with gcc 4.7+ #97

Closed

Multiple instances of Undefined behavior: left shift of negative value #221

Closed

jdeschenes deleted the agg_2_4 branch October 4, 2017 13:46

jwiggins mentioned this pull request Mar 17, 2021

Create an independent renderer for draw_marker_at_points #724

Merged

jwiggins mentioned this pull request Mar 24, 2021

Handle std::overflow_error exception from AGG #750

Closed



		agg24::path_storage img_outline = img.boundary_path(img_mtx);
		agg24::rendering_buffer* src_buf = img.rendering_buffer_ptr();

Conversation

jdeschenes commented Aug 7, 2017

Uh oh!

jwiggins commented Aug 7, 2017

Uh oh!

jwiggins commented Aug 7, 2017

Uh oh!

jdeschenes commented Aug 8, 2017

Uh oh!

jdeschenes commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwiggins Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

jwiggins Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

jdeschenes Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

jdeschenes Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

jwiggins Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

jdeschenes Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

jwiggins Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

jwiggins commented Aug 8, 2017

Uh oh!

jdeschenes commented Aug 9, 2017

Uh oh!

codecov-io commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jdeschenes commented Aug 9, 2017

Uh oh!

jwiggins commented Aug 11, 2017

Uh oh!

itziakos commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvkersch commented Aug 23, 2017

Uh oh!

itziakos commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corranwebster commented Aug 23, 2017

Uh oh!

jdeschenes commented Aug 23, 2017

Uh oh!

jvkersch commented Aug 31, 2017

Uh oh!

corranwebster commented Aug 31, 2017

Uh oh!

jvkersch commented Sep 14, 2017

Uh oh!

jwiggins commented Sep 26, 2017

Uh oh!

jvkersch commented Oct 1, 2017

Uh oh!

jdeschenes commented Oct 2, 2017

Uh oh!

corranwebster commented Oct 2, 2017

Uh oh!

jwiggins commented Oct 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jdeschenes commented Aug 8, 2017 •

edited

Loading

codecov-io commented Aug 9, 2017 •

edited

Loading

itziakos commented Aug 18, 2017 •

edited

Loading

itziakos commented Aug 23, 2017 •

edited

Loading