[webkit-reviews] review granted: [Bug 240259] Implement contain flag for ray() in offset path : [Attachment 459267] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 240259: Implement contain flag for ray() in offset path
https://bugs.webkit.org/show_bug.cgi?id=240259

Attachment 459267: Patch

https://bugs.webkit.org/attachment.cgi?id=459267&action=review




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 459267
  --> https://bugs.webkit.org/attachment.cgi?id=459267
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459267&action=review

> Source/WebCore/ChangeLog:23
> +	   Implement contain flag for ray(). Contains purpose is to have the
entire box
> +	   being animated be contained within the path. "Contained within the
path" is defined
> +	   as having the box remain within a circle with a radius of the path
length and positioned at the offset
> +	   anchor. The way this problem is tackled is based on the approach
here: 
> +	  
https://github.com/ewilligers/petrogale-purpureicollis/blob/20b7403d85d664eb943
232e2a34bc95b6f4a8b62/ray.py.
> +	   The way this solution works is that you construct a coordinate
system with the origin being the offset anchor.
> +	   You then calculate the position of each vertex of the box in this
coordinate system. Next, rotate the vertices based
> +	   on the angle's difference from the x axis. We rotate based on the
x-axis as we will translate the box along the 
> +	   x axis (rather than in the direction of the angle of the ray) as
this simplifies the calculation. Next, using the
> +	   circle equation (x^2 + y^2 = r^2), we want to find an offset such
that (x + offset)^2 + y^2 <= r^2, as this will
> +	   result in all points being contained in the circle. Solving for this
equation, we get the upper and lower bounds 
> +	   of such an offset for each vertex: -x - sqrt(r^2 - y^2) <= offset <=
-x + sqrt(r^2 - y^2). Finally, we find the 
> +	   largest lower bound and smallest upper bound, and choose the larger
of the two values to get the clamped offset.
> +	   This currently doesn't take into account if it is not possible to
fit the box within the path, as this will 
> +	   be completed in a separate patch. Currently test 4 is failing due to
rounding error (might want to look into fixing/
> +	   deleting this test), and test 5 is failing due to the part not
implemented yet.

This needs breaking into paragraphs.

> Source/WebCore/platform/graphics/GeometryUtilities.h:96
> +Vector<FloatPoint> verticesForBox(const FloatRect&, const FloatPoint);

The return value should be a FloatQuad (per maybe a std::array<FloatPoint, 4>.
Vector<> implies variable size, and it never is.

> Source/WebCore/rendering/PathOperation.cpp:87
> +    Vector<std::pair<double, double>> bounds;

Vector<..., 4> for inline capacity. Or use a std::array<> and track the early
exit some other way.

> Source/WebCore/rendering/PathOperation.cpp:125
> +    double length = lengthForPath();

lengthForPath returns a float