Fix PHP bug #64641: imagefilledpolygon doesn't draw horizontal line

[cmb69]

As reported in https://bugs.php.net/64641 1-dimensional horizontal
filled polygons are not drawn at all.

This commit does not yet fix the bug, but rather adds a failing test case
to show the behavior. This test should be marked as XFAIL, but I don't
know how to do that yet.

The patch suggested in the PHP bug report was used to create
php_bug_64641.png which is the expected image, but this patch would break
tests/imagefilledpolygon/gdimagefilledpolygon1. From a quick glance at the
resulting image diff, it seems the patch is plain wrong.

[vapier]

for autotools, i think you want to:

• define XFAIL_TESTS in tests/Makefile.am at the top like TESTS
• add the test both to TESTS and XFAIL_TESTS in the Makemodule.am file

[cmb69]

Thanks, Mike. I've committed the respective changes, but please don't merge this PR yet – I'm hoping to actually fix the bug ASAP.

[pierrejoye]

A quick note :)

Polygon drawing uses scanot lines algorithm. We could loop over the points and draw a single line if the points are aligned (vertically or horizontallly).

Changing the main algorithm for this special case is not necessary nor desired (add complexity).

[pierrejoye]

See https://www.cs.rit.edu/~icss571/filling/index.html

For a detailed explanation

[cmb69]

Thanks, Pierre. I'll still have to digest the algorithm, but it seems I've found the culprit, namely the special case handling of horizontal edges. Obviously, that can't work for a 1-dimensional polygon (i.e. consisting of horizontal edges only), because the edge table (im->polyInts[]) would be empty.

So, yes, we need some special handling for 1-dimensional polygons as you've suggested. I'll work on a patch.

[pierrejoye]

Btw document it could do it as well :)

[cmb69]

I have committed the fix now, and removed the XFAIL provisions.

Btw document it could do it as well :)

I've thought about that as well, but IMHO it seems to be an arbitrary restriction (Why can I draw a vertical filled polygon, but not a horizontal one? Why can I draw an unfilled horizontal polygon, but not a filled one?)

Furthermore, during testing the patch against PHP I've noticed several other test failures. It turned out the gdImageFilledPolygon() is used in gdImageFilledArc(), so this is also affected by the fix. An example (expected, actual, diff):

As one can see, the actual arc is more symmetric than the expected arc (we have a single pixel at the top and bottom, as well as at the left and right hand side). IMO a good reason to apply the fix. We would have to assess the effect of this BC break, though.

Just seen that all Travis checks are failing; I'll have a look at this right away.

[pierrejoye]

Hm. I thought I rewrote this function using the same algorithm I use for ellipse, except for antialised operations. Have to check that.

[cmb69]

I thought I rewrote this function using the same algorithm I use for
ellipse, except for antialised operations.

gdImageArc() just forwards to gdImageFilledArc(), and this is using gdImageFilledPolygon().

[cmb69]

Just seen that all Travis checks are failing; I'll have a look at this right away.

I don't understand, why the checks are failing. Apparently, that is caused by failing to fopen() the expected image, but I don't see why this can happen. Locally, the tests are running fine (after git clean). Maybe a Travis hickup?

[pierrejoye]

Hm doesn't it have to use the absolute path to the expected image?

[pierrejoye]

See https://github.com/libgd/libgd/blob/master/tests/gdimagerotate/bug00067.c for an example

[cmb69]

Hm doesn't it have to use the absolute path to the expected image?
See https://github.com/libgd/libgd/blob/master/tests/gdimagerotate/bug00067.c for an example

According to @vapier sprintf() shouldn't be used anymore for constructing gdAssertImageEqualsToFile() paths. And according to the code relative paths are looked up in tests/.

[vapier]

yes, you shouldn't attempt to construct your own paths anymore. the gdtest helpers should cover all your needs. this deals with all the edge cases with build/test envs.

that said, the issue here is that you need to add all new images to EXTRA_DIST in the Makemodule.am file. notice that the failing section is make distcheck which verifies a release passes.

[cmb69]

that said, the issue here is that you need to add all new images to EXTRA_DIST in the Makemodule.am file.

Thanks! I could have noticed that myself, though…

I've pushed the fix. If the Travis checks succeed, I'll merge later (and also commit the fix to PHP 5.6+).