Merge pull request #18725 from ramezgerges/Path_figure_isfilled

fix(Path): add support for Path.Data being a PathGeometry with some Figures having IsFilled = false

src/Uno.UI.Composition/Composition/CompositionGeometricClip.skia.cs

@@ -13,7 +13,7 @@ partial class CompositionGeometricClip
 		switch (Geometry)
 		{
 			case CompositionPathGeometry { Path.GeometrySource: SkiaGeometrySource2D geometrySource }:
-				return geometrySource.Geometry.TightBounds.ToRect();
+				return geometrySource.TightBounds.ToRect();
 
 			case CompositionPathGeometry cpg:
 				throw new InvalidOperationException($"Clipping with source {cpg.Path?.GeometrySource} is not supported");
@@ -31,15 +31,10 @@ internal override void Apply(SKCanvas canvas, Visual visual)
 		switch (Geometry)
 		{
 			case CompositionPathGeometry { Path.GeometrySource: SkiaGeometrySource2D geometrySource }:
-				var path = geometrySource.Geometry;
-				if (!TransformMatrix.IsIdentity)
-				{
-					var transformedPath = new SKPath();
-					path.Transform(TransformMatrix.ToSKMatrix(), transformedPath);
-					path = transformedPath;
-				}
-
-				canvas.ClipPath(path, antialias: true);
+				var path = TransformMatrix.IsIdentity
+					? geometrySource
+					: geometrySource.Transform(TransformMatrix.ToSKMatrix());
+				path.CanvasClipPath(canvas, antialias: true);
 				break;
 
 			case CompositionPathGeometry cpg:

src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs

@@ -10,13 +10,32 @@ namespace Microsoft.UI.Composition
 {
 	public partial class CompositionSpriteShape : CompositionShape
 	{
-		private SKPath? _geometryWithTransformations;
+		private CompositionGeometry? _fillGeometry;
+
+		private SkiaGeometrySource2D? _geometryWithTransformations;
+		private SkiaGeometrySource2D? _fillGeometryWithTransformations;
+
+		/// <summary>
+		/// This is largely a hack that's needed for MUX.Shapes.Path with Data set to a PathGeometry that has some
+		/// figures with IsFilled = False. CompositionSpriteShapes don't have the concept of a "selectively filled
+		/// geometry". The entire Geometry is either filled (FillBrush is not null) or not. To work around this,
+		/// we add this "fill geometry" which is only the subgeomtry to be filled.
+		/// cf. https://github.com/unoplatform/uno/issues/18694
+		/// Remove this if we port Shapes from WinUI, which don't use CompositionSpriteShapes to begin with, but
+		/// a CompositionMaskBrush that (presumably) masks out certain areas. We compensate for this by using this
+		/// geometry as the mask.
+		/// </summary>
+		internal CompositionGeometry? FillGeometry
+		{
+			private get => _fillGeometry;
+			set => SetProperty(ref _fillGeometry, value);
+		}
 
 		internal override void Paint(in Visual.PaintingSession session)
 		{
 			if (_geometryWithTransformations is { } geometryWithTransformations)
 			{
-				if (FillBrush is { } fill)
+				if (FillBrush is { } fill && _fillGeometryWithTransformations is { } finalFillGeometryWithTransformations)
 				{
 					using var fillPaintDisposable = GetTempFillPaint(session.Filters.OpacityColorFilter, out var fillPaint);
 
@@ -26,7 +45,7 @@ internal override void Paint(in Visual.PaintingSession session)
 					}
 					else
 					{
-						fill.UpdatePaint(fillPaint, geometryWithTransformations.Bounds);
+						fill.UpdatePaint(fillPaint, finalFillGeometryWithTransformations.Bounds);
 					}
 
 					if (fill is CompositionBrushWrapper wrapper)
@@ -45,7 +64,7 @@ internal override void Paint(in Visual.PaintingSession session)
 					}
 					else
 					{
-						session.Canvas.DrawPath(geometryWithTransformations, fillPaint);
+						finalFillGeometryWithTransformations.CanvasDrawPath(session.Canvas, fillPaint);
 					}
 				}
 
@@ -80,7 +99,7 @@ internal override void Paint(in Visual.PaintingSession session)
 					using (SkiaHelper.GetTempSKPath(out var strokeFillPath))
 					{
 						// Get the stroke geometry, after scaling has been applied.
-						strokePaint.GetFillPath(geometryWithTransformations, strokeFillPath);
+						geometryWithTransformations.GetFillPath(strokePaint, strokeFillPath);
 
 						stroke.UpdatePaint(fillPaint, strokeFillPath.Bounds);
 
@@ -124,26 +143,28 @@ private protected override void OnPropertyChangedCore(string? propertyName, bool
 
 			switch (propertyName)
 			{
-				case nameof(Geometry) or nameof(CombinedTransformMatrix):
-					if (Geometry?.BuildGeometry() is SkiaGeometrySource2D { Geometry: { } geometry })
+				case nameof(Geometry) or nameof(CombinedTransformMatrix) or nameof(FillGeometry):
+					if (Geometry?.BuildGeometry() is SkiaGeometrySource2D geometry)
 					{
 						var transform = CombinedTransformMatrix;
-						SKPath geometryWithTransformations;
-						if (transform.IsIdentity)
+						_geometryWithTransformations = transform.IsIdentity
+							? geometry
+							: geometry.Transform(transform.ToSKMatrix());
+						if (FillGeometry?.BuildGeometry() is SkiaGeometrySource2D fillGeometry)
 						{
-							geometryWithTransformations = geometry;
+							_fillGeometryWithTransformations = transform.IsIdentity
+								? fillGeometry
+								: fillGeometry.Transform(transform.ToSKMatrix());
 						}
 						else
 						{
-							geometryWithTransformations = new SKPath();
-							geometry.Transform(transform.ToSKMatrix(), geometryWithTransformations);
+							_fillGeometryWithTransformations = _geometryWithTransformations;
 						}
-
-						_geometryWithTransformations = geometryWithTransformations;
 					}
 					else
 					{
 						_geometryWithTransformations = null;
+						_fillGeometryWithTransformations = null;
 					}
 					break;
 			}
@@ -168,7 +189,7 @@ internal override bool HitTest(Point point)
 
 						using (SkiaHelper.GetTempSKPath(out var hitTestStrokeFillPath))
 						{
-							strokePaint.GetFillPath(geometryWithTransformations, hitTestStrokeFillPath);
+							geometryWithTransformations.GetFillPath(strokePaint, hitTestStrokeFillPath);
 							if (hitTestStrokeFillPath.Contains((float)point.X, (float)point.Y))
 							{
 								return true;

src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs

@@ -8,15 +8,35 @@ namespace Microsoft.UI.Composition
 {
 	internal class SkiaGeometrySource2D : IGeometrySource2D
 	{
+		private readonly SKPath _geometry;
+
 		public SkiaGeometrySource2D(SKPath source)
 		{
-			Geometry = source ?? throw new ArgumentNullException(nameof(source));
+			_geometry = source ?? throw new ArgumentNullException(nameof(source));
+		}
+
+		#region SKPath read-only passthrough methods
+
+		public SkiaGeometrySource2D Transform(SKMatrix matrix)
+		{
+			var path = new SKPath();
+			_geometry.Transform(matrix, path);
+			return new SkiaGeometrySource2D(path);
 		}
 
-		/// <remarks>
-		/// DO NOT MODIFY THIS SKPath. CREATE A NEW SkiaGeometrySource2D INSTEAD.
-		/// This can lead to nasty invalidation bugs where the SKPath changes without notifying anyone.
-		/// </remarks>
-		public SKPath Geometry { get; }
+		public SKRect Bounds => _geometry.Bounds;
+		public SKRect TightBounds => _geometry.TightBounds;
+
+		public void CanvasDrawPath(SKCanvas canvas, SKPaint paint) => canvas.DrawPath(_geometry, paint);
+
+		public void CanvasClipPath(SKCanvas canvas, SKClipOperation operation = SKClipOperation.Intersect, bool antialias = false) => canvas.ClipPath(_geometry, operation, antialias);
+
+		public bool GetFillPath(SKPaint paint, SKPath dst) => paint.GetFillPath(_geometry, dst);
+
+		public bool Contains(float x, float y) => _geometry.Contains(x, y);
+
+		public SkiaGeometrySource2D Op(SkiaGeometrySource2D other, SKPathOp op) => new(_geometry.Op(other._geometry, op));
+
+		#endregion
 	}
 }

src/Uno.UI.RuntimeTests/Helpers/ImageAssert.cs

@@ -131,6 +131,9 @@ public static void DoesNotHaveColorAt(RawBitmap screenshot, float x, float y, st
 	public static void DoesNotHaveColorAt(RawBitmap screenshot, float x, float y, Color excludedColor, byte tolerance = 0, [CallerLineNumber] int line = 0)
 		=> DoesNotHaveColorAtImpl(screenshot, (int)x, (int)y, excludedColor, tolerance, line);
 
+	public static void DoesNotHaveColorAt(RawBitmap screenshot, Point p, Color excludedColor, byte tolerance = 0, [CallerLineNumber] int line = 0)
+		=> DoesNotHaveColorAtImpl(screenshot, (int)p.X, (int)p.Y, excludedColor, tolerance, line);
+
 	private static void DoesNotHaveColorAtImpl(RawBitmap screenshot, int x, int y, Color excludedColor, byte tolerance, int line)
 	{
 		var bitmap = screenshot;

src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs

@@ -1,17 +1,21 @@
 using System;
+using System.Threading.Tasks;
 using Windows.Foundation;
+using Microsoft.UI;
 using Microsoft.UI.Xaml.Markup;
 using Microsoft.UI.Xaml.Media;
+using Microsoft.UI.Xaml.Media.Imaging;
 using Microsoft.UI.Xaml.Shapes;
 using SamplesApp.UITests;
+using Uno.UI.RuntimeTests.Helpers;
 
 namespace Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml_Shapes
 {
 	[TestClass]
+	[RunsOnUIThread]
 	public class Given_Path
 	{
 		[TestMethod]
-		[RunsOnUIThread]
 		[UnoWorkItem("https://github.com/unoplatform/uno/issues/6846")]
 		public void Should_not_throw_if_Path_Data_is_set_to_null()
 		{
@@ -23,14 +27,9 @@ public void Should_not_throw_if_Path_Data_is_set_to_null()
 		}
 
 		[TestMethod]
-		[RunsOnUIThread]
 		public void Should_Not_Include_Control_Points_Bounds()
 		{
-#if WINAPPSDK
 			var SUT = new Path { Data = (Geometry)XamlBindingHelper.ConvertValue(typeof(Geometry), "M 0 0 C 0 0 25 25 0 50") };
-#else
-			var SUT = new Path { Data = "M 0 0 C 0 0 25 25 0 50" };
-#endif
 
 			SUT.Measure(new Size(300, 300));
 
@@ -41,5 +40,122 @@ public void Should_Not_Include_Control_Points_Bounds()
 			Assert.IsTrue(Math.Abs(50 - SUT.DesiredSize.Height) <= 1, $"Actual size: {SUT.DesiredSize}");
 #endif
 		}
+
+		[TestMethod]
+		[UnoWorkItem("https://github.com/unoplatform/uno/issues/18694")]
+#if !__SKIA__
+		[Ignore("PathFigure.IsFilled's interaction with Path is only implemented on Skia.")]
+#endif
+		public async Task When_PathGeometry_Figures_Not_Filled_ColorBrush()
+		{
+			var SUT = new Path
+			{
+				Fill = new SolidColorBrush(Microsoft.UI.Colors.Red),
+				Data = new PathGeometry
+				{
+					Figures = new PathFigureCollection
+					{
+						new PathFigure
+						{
+							StartPoint = new Point(0, 0),
+							IsFilled = true,
+							Segments = new PathSegmentCollection
+							{
+								new LineSegment { Point = new Point(100, 0) },
+								new LineSegment { Point = new Point(100, 100) },
+								new LineSegment { Point = new Point(0, 100) },
+							}
+						},
+						new PathFigure // this is an unfilled rectangle without a stroke, should be useless
+						{
+							StartPoint = new Point(0, 0),
+							IsFilled = false,
+							Segments = new PathSegmentCollection
+							{
+								new LineSegment { Point = new Point(50, 0) },
+								new LineSegment { Point = new Point(50, 50) },
+								new LineSegment { Point = new Point(0, 50) },
+							}
+						},
+						new PathFigure
+						{
+							StartPoint = new Point(0, 100),
+							IsFilled = false,
+							Segments = new PathSegmentCollection
+							{
+								new LineSegment { Point = new Point(100, 100) },
+								new LineSegment { Point = new Point(100, 200) },
+								new LineSegment { Point = new Point(0, 200) },
+							}
+						},
+						new PathFigure
+						{
+							StartPoint = new Point(0, 200),
+							IsFilled = true,
+							Segments = new PathSegmentCollection
+							{
+								new LineSegment { Point = new Point(100, 200) },
+								new LineSegment { Point = new Point(100, 300) },
+								new LineSegment { Point = new Point(0, 300) },
+							}
+						}
+					}
+				}
+			};
+
+			await UITestHelper.Load(SUT);
+
+			var screenShot = await UITestHelper.ScreenShot(SUT);
+			ImageAssert.HasColorAt(screenShot, new Point(25, 25), Microsoft.UI.Colors.Red);
+			ImageAssert.HasColorAt(screenShot, new Point(50, 50), Microsoft.UI.Colors.Red);
+			ImageAssert.DoesNotHaveColorAt(screenShot, new Point(50, 150), Microsoft.UI.Colors.Red);
+			ImageAssert.HasColorAt(screenShot, new Point(50, 250), Microsoft.UI.Colors.Red);
+		}
+
+		[TestMethod]
+		[UnoWorkItem("https://github.com/unoplatform/uno/issues/18694")]
+#if !__SKIA__
+		[Ignore("PathFigure.IsFilled's interaction with Path is only implemented on Skia.")]
+#endif
+		public async Task When_PathGeometry_Figures_Not_Filled_ImageBrush()
+		{
+			var SUT = new Path
+			{
+				Fill = new ImageBrush { ImageSource = new BitmapImage(new Uri("ms-appx:///Assets/rect.png")) },
+				Data = new PathGeometry
+				{
+					Figures = new PathFigureCollection
+					{
+						new PathFigure
+						{
+							StartPoint = new Point(0, 0),
+							IsFilled = true,
+							Segments = new PathSegmentCollection
+							{
+								new LineSegment { Point = new Point(100, 0) },
+								new LineSegment { Point = new Point(100, 100) },
+								new LineSegment { Point = new Point(0, 100) },
+							}
+						},
+						new PathFigure // this is an unfilled rectangle, so the ImageBrush should fill the PathFigure above with the entire image (i.e. as if this PathFigure doesn't exist)
+						{
+							StartPoint = new Point(100, 0),
+							IsFilled = false,
+							Segments = new PathSegmentCollection
+							{
+								new LineSegment { Point = new Point(200, 0) },
+								new LineSegment { Point = new Point(200, 100) },
+								new LineSegment { Point = new Point(100, 100) },
+							}
+						}
+					}
+				}
+			};
+
+			await UITestHelper.Load(SUT);
+
+			var screenShot = await UITestHelper.ScreenShot(SUT);
+			ImageAssert.HasColorAt(screenShot, new Point(90, 50), "#FF38FF52");
+		}
 	}
 }

src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs

@@ -11,6 +11,11 @@ partial class Geometry
 		// this class doesn't have public constructors in UWP, which makes it not-inheritable either way.
 		internal virtual SKPath GetSKPath() => throw new NotSupportedException($"Geometry {this} is not supported");
 
+		/// <remarks>
+		/// Note: Try not to depend on this. See the note in <see cref="CompositionSpriteShape.NegativeFillGeometry"/>
+		/// </remarks>
+		internal virtual SKPath GetFilledSKPath() => null;
+
 		internal virtual SkiaGeometrySource2D GetGeometrySource2D() => new SkiaGeometrySource2D(new SKPath(GetSKPath()));
 	}
 }

src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs

@@ -5,12 +5,21 @@ namespace Microsoft.UI.Xaml.Media
 {
 	partial class PathGeometry
 	{
-		internal override SKPath GetSKPath()
+		internal override SKPath GetSKPath() => GetSKPath(false);
+
+		internal override SKPath GetFilledSKPath() => GetSKPath(true);
+
+		private SKPath GetSKPath(bool skipUnfilled)
 		{
 			var path = new SKPath();
 
 			foreach (PathFigure figure in Figures)
 			{
+				if (skipUnfilled && !figure.IsFilled)
+				{
+					continue;
+				}
+
 				path.MoveTo((float)figure.StartPoint.X, (float)figure.StartPoint.Y);
 
 				foreach (PathSegment segment in figure.Segments)