Support Path.addCircle

[dnfield]

For some reason this is missing currently (can be achieved with addOval, which is implemented in dart and which is how Skia implements it anyway on the C++ side).

I'd like to see this to support SVG rendering (particularly, making circles into paths).

See also flutter/flutter#15501

[Hixie]

This seems somewhat redundant with arcTo; is arcTo not sufficient?

[Hixie]

This should end with a period. Also, put backticks around references to arguments.

[Hixie]

If we add this, it would be nice if we could also find a way to test it. (I don't think we yet test much of this code, but we really should.)

[dnfield]

The following code all would result in the same end result in terms of drawing:

const cx = 20.0;
const cy = 20.0;
const r = 10.0;
Path c1 = new Path()..addCircle(cx, cy, r);
Path c2 = new Path()
  ..addOval(new Rect.fromLTRB(cx - r, cy - r, cx + r, cy + r));
Path c3 = new Path()
  ..addArc(new Rect.fromLTRB(cx - r, cy - r, cx + r, cy + r), 0.0, 360.0);

But addCircle is the clearest/most idiomatic way of doing it (rather than needing to do some math to realize the 'Oval' or 'Arc' you're adding is really a circle). It was also a bit surprising to see I could addOval but not addCircle in the SDK.

The dart code in painting.dart could be rewritten to be an alias to one of those (which is what Skia does in C++ - SkPath::addCircle calls SkPath::addOval using the formula above).

[dnfield]

RE Testing: there are some tests for painting in flutter/flutter, but nothing around paths. I'm not really sure how we could test Paths as they are today - there's no way to test for value equality or get any insight into the contents of a path right now.

[Hixie]

I was thinking more:

Path c4 = new Path()..arcTo(new Rect.fromCircle(center: new Offset(cx, cy), radius: r), 0, pi * 2.0);

...but as you say, there's at least two other ways to do it too. Do we really want to add a fourth?

[Hixie]

(Does the existence of Rect.fromCircle affect your opinion, by the way?)

[dnfield]

Yes, arcTo is fine - although I believe addCircle includes an implicit moveTo.

I don't see this as a particularly critical change - there are numerous work arounds. However, I think it will be an easy source of confusion for developers (particularly when trying to port existing path based logic from other languages/contexts). I think at the least it would make sense to support addCircle like so in painting.dart:

void addCircle(double centerX, double centerY, double radius) {
  _addOval(new Rect.fromCircle(center: new Offset(centerX, centerY), radius: radius));
}

I realize this is adding yet another way to achieve the same thing, but the existing alternatives are less immediately obvious if you just want to add a circle to a path and prone to introducing errors/increased need for unit testing.

[Hixie]

I would definitely support adding documentation to addOval and drawOval that talks about how if you want to draw a circle from a radius and a center point, you can use Rect.fromCircle to do it easily, but I don't see much value in adding more API surface. Keeping the engine API surface small is a goal.

[dnfield]

Ok. It may make sense to just create a Path drawing package in dartpub as well with "utility" methods like these. That should be entirely possible with no engine modifications.

Would you like me to modify this PR to update documentation on addOval as suggested?

[Hixie]

Sure. Thanks!

[dnfield]

I only updated Path.addOval - canvas already has an exposed method drawCircle.

[Hixie]

[Hixie]

When you said Canvas already had "drawCircle" I was like, wait, what? When did we add that. That's silly.
So I did some digging.
Turns out that drawCircle was in fact the first thing we added. The first version of the canvas API literally supporting nothing but drawCircle. -_-

https://github.com/flutter/engine/blame/1eb17996fbac62d075b8805f7fef7ac7ae551897/engine/core/painting/Canvas.idl