Add ClipMode to ClipPath/ClipRRect and PhysicalShape layers #5647

liyuqian · 2018-06-29T23:32:37Z

jason-simmons · 2018-07-02T21:52:05Z

flow/layers/layer.h

 namespace flow {

+// This should be an exact copy of the Clip enum in
+// flutter/package/flutter/lib/src/paining/basic_types.dart


typo: should be packages/flutter/lib/src/painting/basic_types.dart

We now moves the Clip enum to painting.dart.

jason-simmons · 2018-07-02T21:52:35Z

flow/layers/layer.h

+// This should be an exact copy of the Clip enum in
+// flutter/package/flutter/lib/src/paining/basic_types.dart
+//
+// We call it Clip in public dart API to provide our developers the shortest


capitalize "Dart"

jason-simmons · 2018-07-02T21:55:29Z

lib/ui/compositing.dart

-  void _pushClipRRect(Float32List rrect) native 'SceneBuilder_pushClipRRect';
+  /// See [pop] for details about the operation stack, and [Clip] for different clip modes.
+  /// By default, the clip will be anti-aliased (clipMode = 1 = Clip::antiAlias).
+  void pushClipRRect(RRect rrect, [int clipMode = 1]) => _pushClipRRect(rrect._value, clipMode);


Can clipMode be an enum defined in the engine Dart code instead of an int?

yeah the public Dart API should definitely not be using an int for this.

also, prefer named arguments please

Oh, I thought that only painting.dart is public and compositing.dart is private. If it's also public, then I shall definitely move the enum Clip definition from flutter/flutter's basic_types.dart into flutter/engine's painting.dart.

Hixie · 2018-07-03T00:06:06Z

lib/ui/painting.dart

+///
+/// TODO(liyuqian): Set it to Clip.none. (https://github.com/flutter/flutter/issues/18057)
+/// We currently have Clip.antiAlias to preserve our old behaviors.
+const Clip defaultClipBehavior = Clip.none;


we should mark this deprecated so that people don't rely on it (we presumably will remove this in the near future, right?).

Yes, I'll remove it once the default behavior switches to Clip.none. Marking as deprecated.

Hixie · 2018-07-03T00:06:18Z

lib/ui/painting.dart

+
+/// The global default value of whether and how to clip a widget.
+///
+/// TODO(liyuqian): Set it to Clip.none. (https://github.com/flutter/flutter/issues/18057)


TODOs shouldn't be in documentation (make this // not ///)

Hixie · 2018-07-03T00:06:47Z

lib/ui/painting.dart

+  /// See also:
+  ///
+  ///  * [antiAlias], which is much faster, and has similar clipping results.
+  antiAliasWithSaveLayer


trivial nit: missing trailing comma

Hixie · 2018-07-03T00:08:58Z

lib/ui/compositing.dart

-  void pushClipPath(Path path) native 'SceneBuilder_pushClipPath';
+  /// See [pop] for details about the operation stack. See [Clip] for different clip modes.
+  /// By default, the clip will be anti-aliased (clip = [Clip.antiAlias]).
+  void pushClipPath(Path path, {Clip clip = Clip.antiAlias}) => _pushClipPath(path, clip.index);


what happens if you push a clip path with Clip.none?

and does Clip.antiAliasWithSaveLayer do anything interesting here?

Push a clip layer (e.g., ClipPath, ClipRRect) with Clip.none will trigger assert: https://github.com/flutter/engine/pull/5647/files#diff-9930584db1fb28d6064ad30841935ac1R51

Clip.antiAliasWithSaveLayer will add a Canvas.saveLayer call: https://github.com/flutter/engine/pull/5647/files#diff-9930584db1fb28d6064ad30841935ac1R55

when you say "assert" do you mean "throw an AssertionError in Dart" or do you mean "crash the engine"?

It's a FXL_DCHECK(clip_mode_ != ClipMode::none) just like FXL_DCHECK(needs_painting()). I think that it'll crash the engine.

If I implement correctly, the developer should never see this crash. The dart code check whether it's Clip.none, and if it is, drop the ClipPath layer so the C++ side should never receive any ClipPath/ClipRRect layer with ClipMode::none. If the engine somehow receives it, it's probably a much bigger issue that can't be easily bypassed by hot-reload.

I'm confused. What stops someone from calling SceneBuilder.pushClipPath(path, clip = Clip.none) ?

The ClipPathLayer in Dart (https://github.com/flutter/flutter/pull/18576/files#diff-58600d3d2645a66835eb03f3c41b447bR725) prevents C++ engine to have Clip.none ClipPath.

But now I see that if someone is pushing a ClipPathLayer without using the Dart class ClipPathLayer, then it could be a problem. I'll print a warning and drop the layer instead of crashing then.

I would just assert here, just like we do in other methods in this file.

assert(clip != null); assert(clip != Clip.none); _pushClipPath(path, clip.index);

Hixie · 2018-07-03T00:09:49Z

lib/ui/compositing.dart

-  /// [elevation] is greater than 0.0, then a shadow is drawn around the layer.
-  /// [shadowColor] defines the color of the shadow if present and [color] defines the 
+  /// By default, the layer's content will not be clipped (clip = [Clip.none]).
+  /// If clipMode is nonzero (Clip.hardEdge, Clip.antiAlias, or Clip.antiAliasWithSaveLayer),


please put those values in square brackets

also, rather than "is nonzero" consider "has another value" since the enum values aren't zero.

Thanks! I clearly forgot to update the comment here when I changed int clipMode to Clip clip. Updating now.

liyuqian · 2018-07-09T21:19:02Z

@Hixie please let me know if the new changes look good.

Hixie · 2018-07-09T21:47:40Z

lib/ui/painting.dart

+// TODO(liyuqian): Set it to Clip.none. (https://github.com/flutter/flutter/issues/18057)
+// We currently have Clip.antiAlias to preserve our old behaviors.
+@deprecated // we'll eventually remove this and just use Clip.none.
+const Clip defaultClipBehavior = Clip.none;


the comment doesn't match the value? i'm not sure what the purpose of this is.

Consider using @Deprecated() so you can provide a custom message explaining what this is for.

Changed to @Deprecated() and Clip.antiAlias. Thanks for catching the mistake!

i'm worried that that didn't cause any tests to fail! is there no way to test this?

My impression is that the flutter/engine is really under-tested. The flutter/flutter CI should definitely catch this failure once we tried to merge the engine change into flutter. Maybe we should have a engine->flutter auto-roller that does the check? It would not only save this change, but many other changes that make engine roll difficult.

it'll be less undertested if you add tests. :-)

An autoroller would be very useful. You should definitely feel empowered to work on that.

@Hixie : I'm going to work on the autoroller and I'll rely on the golden tests in flutter/flutter to guard this kind of engine changes. Does that sound good to you? Other than that, any more comments on this CL?

I would definitely be happy to trade some unit tests here in order to get an autoroller plus relying on framework tests. :-)

Hixie · 2018-07-10T23:05:43Z

So we can roll engine before the flutter/flutter change

googlebot added the cla: yes label Jun 29, 2018

liyuqian force-pushed the new_clip_option branch 2 times, most recently from 728e080 to 1fed878 Compare July 2, 2018 18:07

liyuqian requested review from cbracken, chinmaygarde and jason-simmons July 2, 2018 18:08

jason-simmons reviewed Jul 2, 2018

View reviewed changes

Hixie reviewed Jul 3, 2018

View reviewed changes

Hixie reviewed Jul 9, 2018

View reviewed changes

liyuqian force-pushed the new_clip_option branch from 3e344e4 to dc25209 Compare July 9, 2018 22:18

liyuqian added 10 commits July 11, 2018 16:58

Add ClipMode to ClipPathLayer

b0248a1

Add ClipMode to ClipRRectLayer

df9c212

Add ClipMode to PhysicalShapeLayer

97c651f

Set default value of clipMode

947679b

So we can roll engine before the flutter/flutter change

Handle ClipMode::none in PhysicalShapeLayer

00aebf9

Move Clip enum to painting.dart

52e51a9

Small fixes

f15c2b5

Ignore deprecated_member_use

4e7e753

Use @deprecated

83e0583

Check clip in compositing.dart; add clip to pushClipRect

44aadcf

liyuqian force-pushed the new_clip_option branch from 5d0ccc4 to 44aadcf Compare July 12, 2018 00:00

Add ClipMode to ClipRectLayer

2836915

liyuqian merged commit e1cf837 into flutter:master Jul 13, 2018

liyuqian deleted the new_clip_option branch September 12, 2018 22:51

Add ClipMode to ClipPath/ClipRRect and PhysicalShape layers #5647

Add ClipMode to ClipPath/ClipRRect and PhysicalShape layers #5647

Uh oh!

Conversation

liyuqian commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 10, 2018

Uh oh!

Hixie Jul 9, 2018 •

edited

Loading

Hixie Jul 10, 2018 •

edited

Loading