Enable line join styles and miter limit. #3777

gspencergoog · 2017-06-15T20:03:50Z

This fixes flutter/flutter#7199 and also enables setting of the miter limit.

A separate pull request for the flutter/flutter repo will enable it there once this is committed.

This adds StrokeJoin as a type, and the Paint.strokeJoin property, so that the caller can set the join type for paths and rectangles. Canvas.drawPoints is unaffected by this property (unfortunately, in my opinion) when drawing points as lines.

Bring my fork up-to-date.

Updating from flutter/engine

gspencergoog · 2017-06-15T20:14:03Z

FYI, The associated flutter/flutter PR is flutter/flutter#10742

Hixie · 2017-06-15T21:09:02Z

lib/ui/painting.dart

@@ -271,6 +271,7 @@ enum FilterQuality {

 /// Styles to use for line endings.
 ///
+/// Must be in the same order as the SkPaint::Cap enum.


This shouldn't be a dartdoc, it won't make sense to our users. It should be a regular comment (and indeed there already is such a comment two lines later).

Hixie · 2017-06-15T21:09:43Z

lib/ui/painting.dart

@@ -286,6 +287,26 @@ enum StrokeCap {
  square,
 }

+/// Styles to use for line joins.
+///
+/// Note that this only affects line joins for polygons drawn by


Please see https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#documentation-comments-dartdocs
In particular, avoid "Note that".

OK, noted. :-) Sorry, I did read that, I just forgot.

Hixie · 2017-06-15T21:09:56Z

lib/ui/painting.dart

+/// [Canvas.drawPath] and rectangles, not points drawn as lines with
+/// [Canvas.drawPoints].
+///
+/// Must be in the same order as the SkPaint::Join enum.


this should be a regular comment, matching the style of the other enums here.

Hixie · 2017-06-15T21:10:14Z

lib/ui/painting.dart

+  /// line segments to give a beveled appearance.
+  bevel,
+}
+


these docs are great.

Hixie · 2017-06-15T21:11:32Z

lib/ui/painting.dart

+  /// The limit for miters to be drawn on segments when the join is set to
+  /// [StrokeJoin.miter] and the [style] is set to [PaintingStyle.stroke]. If
+  /// this limit is exceeded, then a [StrokeJoin.bevel] join will be drawn
+  /// instead.  Note that this may cause some 'popping' of the corners of a


nit: single space after period, not double
avoid "Note that" (in this case you can probably replace ". Note that" with just a semicolon).

Hixie · 2017-06-15T21:12:09Z

lib/ui/painting.dart

+  ///
+  /// Defaults to zero, which means that a reasonable value will be picked
+  /// instead of using zero as the limit, which would draw a bevel all the
+  /// time.


Can you add some discussion about what the "reasonable value" is based on?

Well, it's actually always 4. But I didn't want to give a specific value, since it's defined in Skia, and might change. Well, probably not, but still, it could theoretically change.

https://github.com/google/skia/blob/master/src/core/SkPaintDefaults.h#L32

In thinking about this, though, if zero is specified for the miter limit, then we don't set the value in Flutter, and Skia substitutes 4.0 sort of magically and unpredictably.

So I changed it: Flutter now has its own default value (4.0, duh. :-)) and then I check the passed value against 4.0 in C++ to decide whether or not we actually set it. I assume it's not OK to include SkPaintDefaults.h directly, since it's not a public header (because then I'd compare it against SkPaintDefaults_MiterLimit directly).

This makes the whole thing more "normal" in my mind: it has a valid, documentable default that's 4.0, and you actually can set a miter limit of zero which you couldn't before (which basically means do bevel all the time), and the C++ code is using the actual Skia default to decide if it should actually set it on the SkPaint, so it's still efficient.

I also updated the two other "magic" defaults in there (color and blend mode) to use constants so they could be better documented.

cc @abarth who may have opinions here.
Maybe Skia should expose these constants? It seems like a valid use case.

Yeah, Skia should probably document the default.

Hixie · 2017-06-15T21:14:41Z

lib/ui/painting.dart

+      if (strokeJoin != StrokeJoin.miter)
+        result.write(' $strokeJoin');
+      if (strokeMiterLimit != 0.0)
+        result.write(' $strokeMiterLimit');


just the number on its own will be weird. I would recommend:

if (strokeJoin == StrokeJoin.miter) { if (strokeMiterLimit != 0.0) result.write(' $strokeJoin up to $strokeMiterLimit'); } else { result.write(' $strokeJoin'); }

...or some such.

Hixie · 2017-06-15T21:15:03Z

gspencergoog · 2017-06-16T00:04:46Z

Ian, please take another look at the defaults changes, and then commit if you like it.

Thanks for the review!

Hixie · 2017-06-16T06:26:47Z

lib/ui/painting.dart

@@ -552,20 +615,26 @@ class Paint {
        result.write(' hairline');
      if (strokeCap != StrokeCap.butt)
        result.write(' $strokeCap');
+      if (strokeJoin == StrokeJoin.miter) {
+        if (strokeMiterLimit != _kStrokeMiterLimitDefault)
+          result.write('$strokeJoin up to $strokeMiterLimit');


While we're here (and sorry to make you change this oven and over), can we actually make a slight tweak to this?
Printing doubles ends up with ugly strings of digits. What we normally do is print them to 1dp.

This would mean changing the strokeWidth case above to:

result.write(' ${strokeWidth.toStringAsFixed(1)}');

...and this case to:

result.write(' $strokeJoin up to ${strokeMiterLimit.toStringAsFixed(1)}');

also I just noticed you're missing a space before $strokeJoin.

Good idea, and good catch on the space. Done. Also, no problem about changing it until it's right.

Hixie · 2017-06-16T06:29:05Z

lib/ui/painting.dart

@@ -375,6 +398,7 @@ class Paint {
    _data.setInt32(_kIsAntiAliasOffset, encoded, _kFakeHostEndian);
  }

+  // Must be kept in sync with the default in paint.cc


nitty nit nit: You don't have a period here at the end of the comment, but you do in the C++ comment. I'd add a period here.

Hixie · 2017-06-16T20:55:05Z

lib/ui/painting.dart

@@ -399,6 +423,7 @@ class Paint {
    _data.setInt32(_kColorOffset, encoded, _kFakeHostEndian);
  }

+  // Must be kept in sync with the default in paint.cc


ok i feel like i'm just yanking your chain now but you put the period on the comment above and didn't here...

LOL. I also found another one, just to spare you the task of commenting on each and every one. [Sorry, I should have done a search the first time.]

Hixie · 2017-06-16T21:15:59Z

Thanks :-)

Will land on green.

This reverts commit 5403f65.

…3785) * Revert "Enable line join styles and miter limit. (#3777)" This reverts commit 5403f65. * Revert "Revert "Update switches to use StringView." (#3784)" This reverts commit 80f039f. * Revert "Initial integration of libtxt with Flutter alongside Blink. (#3771)" This reverts commit c548c65.

gspencergoog and others added 6 commits June 14, 2017 13:22

Merge pull request #1 from flutter/master

e4ea86b

Bring my fork up-to-date.

Add support for join types and miter limits.

a22f35d

Merge pull request #2 from flutter/master

0e6538a

Updating from flutter/engine

Fix miter limit data type.

c4d197c

Adding some clearer documentation to painting.dart

477fb76

Clarifying more documentation.

a517156

googlebot added the cla: yes label Jun 15, 2017

gspencergoog mentioned this pull request Jun 15, 2017

Enabling StrokeJoin for paths and rectangles. flutter/flutter#10742

Merged

Hixie reviewed Jun 15, 2017

View reviewed changes

Code review changes

5dcc5ba

Hixie reviewed Jun 16, 2017

View reviewed changes

Fixed debug printing of stroke width and miter limit

8d2c2b8

Hixie reviewed Jun 16, 2017

View reviewed changes

Adding missing periods at the ends of sentences.

e37b108

Hixie merged commit 5403f65 into flutter:master Jun 16, 2017

ianloic added a commit that referenced this pull request Jun 16, 2017

Revert "Enable line join styles and miter limit. (#3777)"

a7e5998

This reverts commit 5403f65.

Hixie mentioned this pull request Jun 17, 2017

Roll engine flutter/flutter#10808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable line join styles and miter limit. #3777

Enable line join styles and miter limit. #3777

gspencergoog commented Jun 15, 2017

gspencergoog commented Jun 15, 2017

Hixie Jun 15, 2017

gspencergoog Jun 15, 2017

Hixie Jun 15, 2017

gspencergoog Jun 15, 2017

Hixie Jun 15, 2017

gspencergoog Jun 15, 2017

Hixie Jun 15, 2017

gspencergoog Jun 15, 2017

Hixie Jun 15, 2017 •

edited

Loading

gspencergoog Jun 15, 2017

Hixie Jun 15, 2017

gspencergoog Jun 15, 2017

Hixie Jun 16, 2017

abarth Jun 16, 2017

Hixie Jun 15, 2017

gspencergoog Jun 15, 2017

Hixie commented Jun 15, 2017

gspencergoog commented Jun 16, 2017

Hixie Jun 16, 2017 •

edited

Loading

Hixie Jun 16, 2017

gspencergoog Jun 16, 2017 •

edited

Loading

Hixie Jun 16, 2017

gspencergoog Jun 16, 2017

Hixie Jun 16, 2017

gspencergoog Jun 16, 2017

Hixie commented Jun 16, 2017

Enable line join styles and miter limit. #3777

Enable line join styles and miter limit. #3777

Conversation

gspencergoog commented Jun 15, 2017

gspencergoog commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Jun 15, 2017

gspencergoog commented Jun 16, 2017

Hixie Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gspencergoog Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Jun 16, 2017

Hixie Jun 15, 2017 •

edited

Loading

Hixie Jun 16, 2017 •

edited

Loading

gspencergoog Jun 16, 2017 •

edited

Loading