Upgrade agg to SVN version #3777
Conversation
|
The failing image comparisons are very subtle. Haven't got to the bottom of why yet, but the difference in colors is quite minor. |
Conflicts: src/_tkagg.cpp
Conflicts: setupext.py
|
On close reading of the code, it seems that the new results are actually more correct. The image interpolation calculation is being done in 32-bit rather than 16-bit integers in the new Agg version resulting in slightly smaller rounding errors. I've updated the base images here. |
|
I am seeing a bunch of extraneous whitespaces. |
|
Are they coming from the SVN version of agg? |
|
Yes. The Agg files are included verbatim. |
| @@ -302,10 +302,10 @@ namespace agg | |||
| alpha_mask_u8(const self_type&); | |||
| const self_type& operator = (const self_type&); | |||
|
|
|||
| agg::rendering_buffer* m_rbuf; | |||
| rendering_buffer* m_rbuf; | |||
There was a problem hiding this comment.
dropping a namespace? perhaps we should push up some patches to them?
There was a problem hiding this comment.
In this case, it's just the dropping of a redundant namespace specifier, since this code is in a namespace agg block. It's not actually changing the namespace in which rendering_buffer lives. Generally, agg doesn't using the namespace prefix everywhere in its internal code, which is typical C++ practice.
|
Should I trim the whitespace on Agg code now? It makes things easier now, but will make tracking Agg SVN harder in the long run, unless I can convince Agg to change their trailing whitespace policy, which seems unlikely. |
| double ct2_y = 0.0; | ||
| double end_x = 0.0; | ||
| double end_y = 0.0; | ||
| double ct2_x; |
There was a problem hiding this comment.
these are getting passed into functions uninitialized. That doesn't seem right to me.
There was a problem hiding this comment.
Actually, their addresses are passed to vertex, where they should be set, but they aren't currently, due to the fault of matplotlib-side code. See #3781.
| @@ -704,18 +698,6 @@ namespace agg | |||
| } | |||
| } | |||
|
|
|||
| i = m_num_cells & cell_block_mask; | |||
There was a problem hiding this comment.
same here. I am wondering if the changes to the above while-loop rendered this block unneeded?
There was a problem hiding this comment.
Not sure. This bit of code is pretty opaque. Were it a bug, however, I'm sure our regression tests would hit it.
There was a problem hiding this comment.
If I am reading it right, I think the original code was taking care of the last chunk that was smaller than the regular chunk size, and the updated code takes care of that inside the while loop. This is really opaque.
|
I am happy enough with this and I suspect getting any happier would involve spending a week doing nothing but reading the Agg code (and get worse before it got better) so 👍 from me. Maybe we want to do 1.5 a few months so that we don't have these back-end changes + new features all mixed up in one release? |
|
Agreed. There isn't anything here that fundamentally scares me (although I haven't looked at the image diffs yet). The only thing that I want checked prior to merging is the impact (if any) to Cairo. |
|
so, it looks like all of the agg changes only impacted two images? Would it make sense to rebaseline some of the alpha-using images? |
|
OK with me. @tacaswell your suggestion for a release soon is good--but that's easy for me to say, because you are the one doing the work. |
MNT : Upgrade agg to SVN version
|
Yes -- the agg changes only impacted 2 images that test saving images to vector formats. I'd rather not rebaseline any of the other alpha-using tests, as they are correct to begin with, and this shouldn't change that. I don't think this should impact Cairo in any way -- that's an entirely different backend and codepath. |
As a first step toward doing image interpolation in floating-point, and having (optional) floating-point rendering buffers for more accurate alpha blending, this upgrades our included copy of Agg to the current SVN version here: http://sourceforge.net/p/agg/svn/HEAD/tree/
This version of Agg is a fork of Agg 2.4, so it is still under a BSD license.
A few caveats: There is a regression in the alpha blending on RGBA 32-bit (8-bit-per-plane) buffers caused by doing the alpha blending calculation in 8-bits rather than 32-bits as it was before. I've communicated on the Agg mailing list about a solution, but I can't find the "right way" to do it. So, instead, I've included the one-off solution that uses the old calculation from Agg-2.4 verbatim.
The core Agg developers seem to see no need for releasing tagged versions of Agg. (Again, discussed on the mailing list). This may be a problem for some packagers, since matplotlib can no longer be built against the "system" Agg. Currently, Fedora uses a system Agg for its matplotlib package, but Debian, Arch, MacPorts, and Homebrew do not.