Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path.convertConicToQuads() incorrectly implemented #820

Open
romainguy opened this issue Nov 1, 2023 · 0 comments
Open

Path.convertConicToQuads() incorrectly implemented #820

romainguy opened this issue Nov 1, 2023 · 0 comments

Comments

@romainguy
Copy link

Path.convertConicToQuads() invokes SkPath::ConvertConicToQuads via _nConvertConicToQuads() but misuses the return value. Here is the current implementation:

        fun convertConicToQuads(p0: Point, p1: Point, p2: Point, w: Float, pow2: Int): Array<Point> {
            Stats.onNativeCall()
            val maxResultPointCount = (1 + 2 * (1 shl pow2)) // See Skia docs
            var pointCount = 0
            val coords = withResult(FloatArray(maxResultPointCount * 2)) {
                pointCount = _nConvertConicToQuads(p0.x, p0.y, p1.x, p1.y, p2.x, p2.y, w, pow2, it)
            }
            return Array(pointCount) { Point(coords[2 * it], coords[2 * it + 1]) }
        }

The value returned by _nConvertConicToQuads is used as a number of points when it's actually a number of quadratic curves. Since each curve is made of 3 points (although in the array the curves all overlap by 1 point), the returned array is missing most of the data, and provides invalid results (returning for instance 2 points when at least 3 are required to build a single quadratic curve).

copybara-service bot pushed a commit to androidx/androidx that referenced this issue Nov 14, 2023
This change is a series of improvement to path APIs, in large part as
foundational work for upcoming changes to Cubic/PathEasing:

- Add the ability to set the winding direction when adding shapes to
  a Path (addOval(), addRect() and addRoundRect()). The winding
  direction is necessary to create complex shapes with holes. For
  instance a donut is made of two concentric circles each with a
  different winding.

- Deprecate quadraticBezierTo() and relativeQuadraticBezierTo() in
  favor of quadraticTo() and relativeQuadraticTo(). In addition to
  being shorter names, these APIs are also more consistent with
  cubicTo() and relativeCubicTo().

- Add easier APIs for Path.op:
  - Union: p1 + p2
  - Union: p1 or p2
  - Intersection: p1 and p2
  - Difference: p1 - p2
  - Xor: p1 xor p2

- Add APIs to iterate over the segments of a Path using Path.iterate().
  The API is modeled after androidx-graphics:graphics-path. A key
  component of the API is the ability, turned on by default, to convert
  conic path segments into quadratic path segments.
  The new PathIterator API provides the ability to iterate over the
  segments in two distinct ways:
  - The "OOP" way with allocations, by returning a new PathSegment
    instance for every segment.
  - The "graphics" way by passing a float array that gets filled with
    the data required to describe the path segment.
  The first API is useful for readability/ease of use, while the second
  one is necessary for performance/to avoid allocations.
  To make this feature possible across all supported API levels, the
  compose-ui-graphics module now depends on graphics-path.

Note: this CL provides a Skiko implementation of path iteration albeit
with a limitation. The conics to quadratics part of the implementation
is missing because Skiko currently contains a bug preventing this
feature from being implemented properly. See the issue on GitHub for
more details: JetBrains/skiko#820

This CL also does a bit of cleanup throughout related code.

Test: PathIteratorTest
RelNote: Added Path.iterate() to iterate over Path segments
RelNote: Use Path.quadraticTo() instead of Path.quadraticBezierTo()
RelNote: You can now set the winding direction when calling Path.addOval(), addRect(), and addRoundRect()
Change-Id: If19bc174d2649ef9024943517395ccb0ef333f61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant