Properly minimize the rasterized layers

[mdboom]

Fixes #5814.

[mdboom]

@tacaswell: Your discretion whether to put this on 1.5.1rc or 1.5.2...

[WeatherGod]

weird, I can't find when these lines were introduced. Git blame said they were last touched during the CXX refactor, but when I go to that commit, I can't find these lines anywhere...

[mdboom]

It used to be called tostring_rgba_minimized. And actually it looks like it used to have this fix, which got transferred incorrectly in the C++ refactor:

       // Expand the bounds by 1 pixel on all sides
        xmin = std::max(0, xmin - 1);
        ymin = std::max(0, ymin - 1);
        xmax = std::min(xmax, (int)width);
        ymax = std::min(ymax, (int)height);

[WeatherGod]

What's with the plus 1's and minus 1's?

[mdboom]

The +1 makes sense to me. The -1 I'm not sure, but it's been there forever.

[WeatherGod]

Nah, neither of them make sense to me. Instead, I think the -1 shouldn't be there for x1/y1, and instead of +1 on x2/y2, it should be -1 on width/height. Note, I am not entirely certain if the subsequent code assumes 0 or 1-based indexing. I presume it is 0-based.

[mdboom]

It's zero-based. The range is like a Python slice, where the lower value is inclusive and the upper value is exclusive. Hence the need for +1 on the upper values.

[WeatherGod]

ok, so the -1's doesn't make sense at all.

[WeatherGod]

well, except for the original comment saying that it is trying to expand the bounds on all sides, but doesn't say why

[mdboom]

Yeah -- we could take the -1's out and see what happens to the test suite. Now that we're not doing fuzzy testing, we shouldn't know pretty quickly if it has negative consequences.

[WeatherGod]

hmm, the failure on 2.7 looks spurious, but it isn't one of the usual suspects (one of the stix tests). Restarted anyway.

[WeatherGod]

And Travis failed again, but on a completely different test this time. Restarted.

[WeatherGod]

Hmm, no failures in the Travis tests. Weird, but ok... waiting for appveyor.

[WeatherGod]

I think this should go into 1.5.1 since it fixes a pretty bad error, but I'll let @tacaswell decide if it is worth putting out a rc2 for.

[tacaswell]

👍 to putting this in 1.5.1 👎 on needing an rc2.

[efiring]

On 2016/01/08 9:45 AM, Thomas A Caswell wrote:

👍 to putting this in 1.5.1 👎 on needing an rc2.

Agreed!

[WeatherGod]

appveyor failed with weird errors. I don't think I have the ability to restart them.

[tacaswell]

backported via PR + 2 commits as ac8cfb3 in #5817

[tomspur]

With the current master, I get 4 failures and git bisect blames the latest commit in this pull request.
One example is matplotlib.testing.decorators.test_surface3d.test and the difference of the expected and generated picture looks like:

It seems the right end side of the image is not cropped correctly.

The other failing tests are:

• matplotlib.tests.test_streamplot.test_colormap
• matplotlib.tests.test_image.test_rasterize_dpi
• matplotlib.tests.test_bbox_tight.test_bbox_inches_tight_raster

The following "patch" would fix the first two failing tests, but the other two tests are not 100% identical and still fail:

diff --git a/src/_backend_agg.cpp b/src/_backend_agg.cpp
index e472156..3b9742a 100644
--- a/src/_backend_agg.cpp
+++ b/src/_backend_agg.cpp
@@ -210,10 +210,10 @@ agg::rect_i RendererAgg::get_content_extents()
         }
     }

-    r.x1 = std::max(0, r.x1);
-    r.y1 = std::max(0, r.y1);
-    r.x2 = std::min(r.x2 + 1, (int)width);
-    r.y2 = std::min(r.y2 + 1, (int)height);
+    r.x1 = std::max(0, r.x1 - 1);
+    r.y1 = std::max(0, r.y1 - 1);
+    r.x2 = std::min(r.x2 + 2, (int)width);
+    r.y2 = std::min(r.y2 + 2, (int)height);

     return r;
 }

Do you know, why/how this test could fail here (on Fedora), but not on travis? What else could I do to help debugging this?

[tacaswell]

Is this before or after #5834 went in?

Which formats are failing? If it is all svg, part of the issue may be the rasterization via inkscape.

[tomspur]

The latest commit from #5834 (and the whole v1.5.x branch) seems to work just fine.
Yet, the problem is in the master branch and all failing tests are indeed svg.

[tacaswell]

Oh, we need to merge 1.5.x back into master.

On Tue, Jan 12, 2016 at 5:42 PM Thomas Spura notifications@github.com
wrote:

The latest commit from #5834
#5834 (and the whole
v1.5.x branch) seems to work just fine.
Yet, the problem is in the master branch and all failing tests are indeed
svg.

—
Reply to this email directly or view it on GitHub
#5815 (comment)
.