Bug 37276 – LayoutTests/fast/canvas/pointInPath.html passed, actually it failed

• Status: CLOSED
• Resolution: FIXED
• Component: New Bugs

[qi]

pointInPath passed becuase of the pointInPath-expected.txt (LayoutTests/platform/qt/fast/canvas/):

FAIL ctx.isPointInPath(10, 0) should be true. Was false.
FAIL ctx.isPointInPath(29, 0) should be true. Was false.

[qi]

HTML5 spec: Points on the path itself are considered to be inside the path.

Qt's API (QPainterPah::contains) doesn't count the point at the path.

[qi]

Created attachment 53169 [details]
pointInPath

QPainterPath::Contains doesn't count the point on the bound, but isPointInPath should count the point on the bound (based on the HTML5 spec)

[Chang Shu]

(In reply to comment #2)
> Created an attachment (id=53169) [details]
> pointInPath
> 
> QPainterPath::Contains doesn't count the point on the bound, but isPointInPath
> should count the point on the bound (based on the HTML5 spec)

I would suggest to put the code in a separate function like isPointOnPathBorder. But you can make the change after any reviewers commented on the patch.

[Laszlo Gombos]

Comment on attachment 53169 [details]
pointInPath

Overall the approach and the algorithm looks good to me.

I think this code could use some comments and/or refactoring; specific suggestions below..

> +    if (!contains) {
> +        // check whether the point is on the bound

As Chang suggested, I think it make sense to break this out into its own function.

> +    
> +        QPolygonF polygon = m_path.toFillPolygon();
> +        QPointF p1 = polygon.at(0);
> +        QPointF p2;
> +        QPointF p = point;

Variable p seems to be used only as an alias to point; this might save some typing, but it makes a bit harder to read for me. If the function is factored out p would become the argument, in which case the code is OK.

> +
> +        for (int i = 1; i < polygon.size(); ++i) {
> +            p2 = polygon.at(i);

Comment explaining why special casing is needed if the x coordinates are the same would be helpful (e.g. slope can not be calculated if the x coordinates are the same).

> +            if (p.x() == p1.x() || p.x() == p2.x()) {
> +                if (p.x() == p1.x() && p.x() == p2.x()
> +                    && p.y() <= qMax(p1.y(), p2.y()) && p.y() >= qMin(p1.y(), p2.y())) {
> +                    contains = true;
> +                    break;
> +                

A simple comment like "If the slopes between p and p1 is equal to that between p and p2 then the three points lie on the same line" would help.

> +            } else if ((p.y()-p1.y()) / (p.x()-p1.x()) == (p2.y()-p.y()) / (p2.x()-p.x())) {
> +                if (p.x() <= qMax(p1.x(), p2.x()) && p.x() >= qMin(p1.x(), p2.x())
> +                    && p.y() <= qMax(p1.y(), p2.y()) && p.y() >= qMin(p1.y(), p2.y())) {
> +                    contains= true;
> +                    break;
> +                }
> +            }
> +            p1 = p2;
> +        }
> +    }
>  
>      const_cast<QPainterPath*>(&m_path)->setFillRule(savedRule);
>      return contains;
> Index: LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt
> ===================================================================
> --- LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt	(revision 57339)
> +++ LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt	(working copy)
> @@ -37,8 +37,8 @@ Translate context (10,20)
>  Transform context (1, 0, 0, -1, 0, 0)
>  Rectangle at (0,0) 20x20
>  PASS ctx.isPointInPath(5, 5) is false
> -FAIL ctx.isPointInPath(10, 0) should be true. Was false.
> -FAIL ctx.isPointInPath(29, 0) should be true. Was false.
> +PASS ctx.isPointInPath(10, 0) is true
> +PASS ctx.isPointInPath(29, 0) is true
>  PASS ctx.isPointInPath(10, 19) is true
>  PASS ctx.isPointInPath(21, 10) is true
>  PASS ctx.isPointInPath(29, 19) is true

With this change LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt becomes the same as LayoutTests/fast/canvas/pointInPath-expected.txt (as it should be). Can we just delete the qt specific expected results and go with the cross-platform one ? 

This does not quite deserve the r-, but I'd like to see if we can address some of this feedback, so r- for now.

[Chang Shu]

Actually, I am not sure if the algorithm is right. It looks to me when p == p1 (but != p2), the function returns false.

[Chang Shu]

a couple of more issues after I looked into the details:
1. the algorithm didn't seem to check the last edge, i.e., from point n-1 to 0.
2. i think the algorithm can be simplified to ((x0<=x<=x1)||(x0>=x>=x1))&&((y1-y0)*(x-x0)==(y-y0)(x1-x0)).
3. i am also concerned about precisions introduced by floating number. i.e., the above equation might need to be adjusted a bit.

[qi]

Created attachment 54187 [details]
patch2

Based on Laszlo and Chang's comments, I did some change:
1> Take the code for checking point on the border to a function
2> Used a simplified formula, but I did a little bit change. Because we do need to check if the point.y is between p1.y and p2.y. For example: if p1.x==p2.x==p.x, but p1.y!=p2.y, any p.y will still make (y2-y1)*(x-x1)==(y-y1)*(x2-x1) true.
3> We don't need to care point n-1 to 0, because path will force n-1 is same with 0 to close the path.
4> Remove pointInPath-expected.txt from qt as Laszlo said.
5> Put some comments about algorithm.

[qi]

Created attachment 54191 [details]
patch3

1. Add some new comments
2. Remove png and checknum files

[Laszlo Gombos]

Comment on attachment 54191 [details]
patch3

All review comments addresses, it looks good to me. r+. Thanks.

[WebKit Commit Bot]

Comment on attachment 54191 [details]
patch3

Clearing flags on attachment: 54191

Committed r58207: <http://trac.webkit.org/changeset/58207>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r58207 cherry-picked into qtwebkit-2.0 with commit 0d9e7dc77517ffdc19506525a0eea9e01d16d7fa