From f44003018a6def2cf59e97c02353e9e28cb10843 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 5 Nov 2024 20:07:39 +0200 Subject: [PATCH 01/10] chore: remove unnecessary NativeSingle --- src/Uno.UI/UI/Xaml/Shapes/Shape.layout.crossruntime.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Uno.UI/UI/Xaml/Shapes/Shape.layout.crossruntime.cs b/src/Uno.UI/UI/Xaml/Shapes/Shape.layout.crossruntime.cs index c813bb55ac9b..34ca886d2d90 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Shape.layout.crossruntime.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Shape.layout.crossruntime.cs @@ -14,11 +14,9 @@ #if __SKIA__ using NativePath = Microsoft.UI.Composition.SkiaGeometrySource2D; -using NativeSingle = System.Double; #elif __WASM__ using NativePath = Microsoft.UI.Xaml.Shapes.Shape; -using NativeSingle = System.Double; #endif namespace Microsoft.UI.Xaml.Shapes; @@ -37,9 +35,8 @@ private protected Size ArrangeAbsoluteShape(Size finalSize, NativePath? path, Fi var stroke = Stroke; var strokeThickness = stroke is null ? DefaultStrokeThicknessWhenNoStrokeDefined : StrokeThickness; var pathBounds = GetPathBoundingBox(path); // The BoundingBox shouldn't include the control points. - var pathSize = (Size)pathBounds.Size; - if (NativeSingle.IsInfinity(pathBounds.Right) || NativeSingle.IsInfinity(pathBounds.Bottom)) + if (IsInfinity(pathBounds.Right) || IsInfinity(pathBounds.Bottom)) { if (this.Log().IsEnabled(Uno.Foundation.Logging.LogLevel.Debug)) { From c432d0b6bb29e84755a6e452a555c0c0630753fa Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 5 Nov 2024 21:48:26 +0200 Subject: [PATCH 02/10] chore: remove SkiaGeometrySource2D.Geometry --- .../CompositionGeometricClip.skia.cs | 15 ++++------ .../CompositionSpriteShape.skia.cs | 25 +++++----------- .../Composition/SkiaGeometrySource2D.skia.cs | 30 +++++++++++++++---- src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs | 2 +- 4 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/Uno.UI.Composition/Composition/CompositionGeometricClip.skia.cs b/src/Uno.UI.Composition/Composition/CompositionGeometricClip.skia.cs index d87b3ac55057..c0d948bd9c0b 100644 --- a/src/Uno.UI.Composition/Composition/CompositionGeometricClip.skia.cs +++ b/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: diff --git a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs index 327198534c56..40f37fe044d8 100644 --- a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs +++ b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs @@ -10,7 +10,7 @@ namespace Microsoft.UI.Composition { public partial class CompositionSpriteShape : CompositionShape { - private SKPath? _geometryWithTransformations; + private SkiaGeometrySource2D? _geometryWithTransformations; internal override void Paint(in Visual.PaintingSession session) { @@ -45,7 +45,7 @@ internal override void Paint(in Visual.PaintingSession session) } else { - session.Canvas.DrawPath(geometryWithTransformations, fillPaint); + geometryWithTransformations.CanvasDrawPath(session.Canvas, fillPaint); } } @@ -80,7 +80,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); @@ -125,21 +125,12 @@ private protected override void OnPropertyChangedCore(string? propertyName, bool switch (propertyName) { case nameof(Geometry) or nameof(CombinedTransformMatrix): - if (Geometry?.BuildGeometry() is SkiaGeometrySource2D { Geometry: { } geometry }) + if (Geometry?.BuildGeometry() is SkiaGeometrySource2D geometrySource2D) { var transform = CombinedTransformMatrix; - SKPath geometryWithTransformations; - if (transform.IsIdentity) - { - geometryWithTransformations = geometry; - } - else - { - geometryWithTransformations = new SKPath(); - geometry.Transform(transform.ToSKMatrix(), geometryWithTransformations); - } - - _geometryWithTransformations = geometryWithTransformations; + _geometryWithTransformations = transform.IsIdentity + ? geometrySource2D + : geometrySource2D.Transform(transform.ToSKMatrix()); } else { @@ -168,7 +159,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; diff --git a/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs b/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs index e1872a244684..178787b698c7 100644 --- a/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs +++ b/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs @@ -8,15 +8,33 @@ 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); } - /// - /// DO NOT MODIFY THIS SKPath. CREATE A NEW SkiaGeometrySource2D INSTEAD. - /// This can lead to nasty invalidation bugs where the SKPath changes without notifying anyone. - /// - 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); + + #endregion } } diff --git a/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs b/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs index 5e9bd556f695..2cf143e0d3e3 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs @@ -26,7 +26,7 @@ public Shape() } private Rect GetPathBoundingBox(SkiaGeometrySource2D path) - => path.Geometry.TightBounds.ToRect(); + => path.TightBounds.ToRect(); private protected override ContainerVisual CreateElementVisual() => Compositor.GetSharedCompositor().CreateShapeVisual(); From 7e4982db7104c214f6320e8299d0df07ea72a2c1 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Thu, 7 Nov 2024 02:01:47 +0200 Subject: [PATCH 03/10] fix(Path): add support for Path.Data being a PathGeometry with some figures having IsFilled = false --- .../CompositionSpriteShape.skia.cs | 49 +++++++++++++++---- .../Composition/SkiaGeometrySource2D.skia.cs | 2 + src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs | 5 ++ src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs | 15 ++++-- src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs | 15 ++++++ src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs | 2 + 6 files changed, 76 insertions(+), 12 deletions(-) diff --git a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs index 40f37fe044d8..0682dd6f1bbe 100644 --- a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs +++ b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs @@ -10,13 +10,32 @@ namespace Microsoft.UI.Composition { public partial class CompositionSpriteShape : CompositionShape { + private CompositionGeometry? _negativeFillGeometry; + private SkiaGeometrySource2D? _geometryWithTransformations; + private SkiaGeometrySource2D? _finalFillGeometryWithTransformations; + + /// + /// 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 "negative geometry" that is subtracted from the to get the + /// final geometry 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 + /// negative geometry as a "negative mask". + /// + public CompositionGeometry? NegativeFillGeometry + { + get => _negativeFillGeometry; + set => SetProperty(ref _negativeFillGeometry, value); + } internal override void Paint(in Visual.PaintingSession session) { if (_geometryWithTransformations is { } geometryWithTransformations) { - if (FillBrush is { } fill) + if (FillBrush is { } fill && _finalFillGeometryWithTransformations 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 { - geometryWithTransformations.CanvasDrawPath(session.Canvas, fillPaint); + finalFillGeometryWithTransformations.CanvasDrawPath(session.Canvas, fillPaint); } } @@ -82,9 +101,9 @@ internal override void Paint(in Visual.PaintingSession session) // Get the stroke geometry, after scaling has been applied. geometryWithTransformations.GetFillPath(strokePaint, strokeFillPath); - stroke.UpdatePaint(fillPaint, strokeFillPath.Bounds); + stroke.UpdatePaint(strokePaint, strokeFillPath.Bounds); - session.Canvas.DrawPath(strokeFillPath, fillPaint); + session.Canvas.DrawPath(strokeFillPath, strokePaint); } } } @@ -124,17 +143,29 @@ private protected override void OnPropertyChangedCore(string? propertyName, bool switch (propertyName) { - case nameof(Geometry) or nameof(CombinedTransformMatrix): - if (Geometry?.BuildGeometry() is SkiaGeometrySource2D geometrySource2D) + case nameof(Geometry) or nameof(CombinedTransformMatrix) or nameof(NegativeFillGeometry): + if (Geometry?.BuildGeometry() is SkiaGeometrySource2D geometry) { var transform = CombinedTransformMatrix; _geometryWithTransformations = transform.IsIdentity - ? geometrySource2D - : geometrySource2D.Transform(transform.ToSKMatrix()); + ? geometry + : geometry.Transform(transform.ToSKMatrix()); + if (NegativeFillGeometry?.BuildGeometry() is SkiaGeometrySource2D negativeFillGeometry) + { + var finalFillGeometry = geometry.Op(negativeFillGeometry, SKPathOp.Difference); + _finalFillGeometryWithTransformations = transform.IsIdentity + ? finalFillGeometry + : finalFillGeometry.Transform(transform.ToSKMatrix()); + } + else + { + _finalFillGeometryWithTransformations = _geometryWithTransformations; + } } else { _geometryWithTransformations = null; + _finalFillGeometryWithTransformations = null; } break; } diff --git a/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs b/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs index 178787b698c7..a292ab77a093 100644 --- a/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs +++ b/src/Uno.UI.Composition/Composition/SkiaGeometrySource2D.skia.cs @@ -35,6 +35,8 @@ public SkiaGeometrySource2D Transform(SKMatrix matrix) 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 } } diff --git a/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs b/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs index ec2e7b9f3879..397d81aeed5b 100644 --- a/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs +++ b/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"); + /// + /// Note: Try not to depend on this. See the note in + /// + internal virtual SKPath GetUnfilledSKPath() => null; + internal virtual SkiaGeometrySource2D GetGeometrySource2D() => new SkiaGeometrySource2D(new SKPath(GetSKPath())); } } diff --git a/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs b/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs index f3f2b0a89538..05bccafd1387 100644 --- a/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs +++ b/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs @@ -5,15 +5,24 @@ namespace Microsoft.UI.Xaml.Media { partial class PathGeometry { - internal override SKPath GetSKPath() + internal override SKPath GetSKPath() => GetSKPath(false); + + internal override SKPath GetUnfilledSKPath() => GetSKPath(true); + + internal SKPath GetSKPath(bool skipFilled) { var path = new SKPath(); - foreach (PathFigure figure in Figures) + foreach (var figure in Figures) { + if (skipFilled && figure.IsFilled) + { + continue; + } + path.MoveTo((float)figure.StartPoint.X, (float)figure.StartPoint.Y); - foreach (PathSegment segment in figure.Segments) + foreach (var segment in figure.Segments) { if (segment is LineSegment lineSegment) { diff --git a/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs b/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs index a2396d6fb372..4aecbcd1260b 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs @@ -16,5 +16,20 @@ protected override Size ArrangeOverride(Size finalSize) => ArrangeAbsoluteShape(finalSize, GetPath()); private SkiaGeometrySource2D? GetPath() => Data?.GetGeometrySource2D(); + + private protected override void Render(SkiaGeometrySource2D? path, double? scaleX = null, double? scaleY = null, double? renderOriginX = null, + double? renderOriginY = null) + { + base.Render(path, scaleX, scaleY, renderOriginX, renderOriginY); + + if (Data?.GetUnfilledSKPath() is { } negativePath) + { + SpriteShape.NegativeFillGeometry = Visual.Compositor.CreatePathGeometry(new CompositionPath(new SkiaGeometrySource2D(negativePath))); + } + else + { + SpriteShape.NegativeFillGeometry = null; + } + } } } diff --git a/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs b/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs index 2cf143e0d3e3..f516d6adb362 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs @@ -11,6 +11,8 @@ partial class Shape private readonly CompositionSpriteShape _shape; private readonly CompositionPathGeometry _geometry; + protected CompositionSpriteShape SpriteShape => _shape; + public Shape() { var visual = Visual; From 40bb019529c59ae2146fb8fb7ec13a6904b9f80e Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Thu, 7 Nov 2024 02:53:33 +0200 Subject: [PATCH 04/10] chore: negative mask -> positive mask --- .../CompositionSpriteShape.skia.cs | 33 +++++++++---------- src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs | 2 +- src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs | 6 ++-- src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs | 4 +-- 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs index 0682dd6f1bbe..a9df0122a322 100644 --- a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs +++ b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs @@ -10,32 +10,32 @@ namespace Microsoft.UI.Composition { public partial class CompositionSpriteShape : CompositionShape { - private CompositionGeometry? _negativeFillGeometry; + private CompositionGeometry? _fillGeometry; private SkiaGeometrySource2D? _geometryWithTransformations; - private SkiaGeometrySource2D? _finalFillGeometryWithTransformations; + private SkiaGeometrySource2D? _fillGeometryWithTransformations; /// /// 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 "negative geometry" that is subtracted from the to get the - /// final geometry to be filled. cf. https://github.com/unoplatform/uno/issues/18694 + /// 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 - /// negative geometry as a "negative mask". + /// geometry as the mask. /// - public CompositionGeometry? NegativeFillGeometry + public CompositionGeometry? FillGeometry { - get => _negativeFillGeometry; - set => SetProperty(ref _negativeFillGeometry, value); + get => _fillGeometry; + set => SetProperty(ref _fillGeometry, value); } internal override void Paint(in Visual.PaintingSession session) { if (_geometryWithTransformations is { } geometryWithTransformations) { - if (FillBrush is { } fill && _finalFillGeometryWithTransformations is { } finalFillGeometryWithTransformations) + if (FillBrush is { } fill && _fillGeometryWithTransformations is { } finalFillGeometryWithTransformations) { using var fillPaintDisposable = GetTempFillPaint(session.Filters.OpacityColorFilter, out var fillPaint); @@ -143,29 +143,28 @@ private protected override void OnPropertyChangedCore(string? propertyName, bool switch (propertyName) { - case nameof(Geometry) or nameof(CombinedTransformMatrix) or nameof(NegativeFillGeometry): + case nameof(Geometry) or nameof(CombinedTransformMatrix) or nameof(FillGeometry): if (Geometry?.BuildGeometry() is SkiaGeometrySource2D geometry) { var transform = CombinedTransformMatrix; _geometryWithTransformations = transform.IsIdentity ? geometry : geometry.Transform(transform.ToSKMatrix()); - if (NegativeFillGeometry?.BuildGeometry() is SkiaGeometrySource2D negativeFillGeometry) + if (FillGeometry?.BuildGeometry() is SkiaGeometrySource2D fillGeometry) { - var finalFillGeometry = geometry.Op(negativeFillGeometry, SKPathOp.Difference); - _finalFillGeometryWithTransformations = transform.IsIdentity - ? finalFillGeometry - : finalFillGeometry.Transform(transform.ToSKMatrix()); + _fillGeometryWithTransformations = transform.IsIdentity + ? fillGeometry + : fillGeometry.Transform(transform.ToSKMatrix()); } else { - _finalFillGeometryWithTransformations = _geometryWithTransformations; + _fillGeometryWithTransformations = _geometryWithTransformations; } } else { _geometryWithTransformations = null; - _finalFillGeometryWithTransformations = null; + _fillGeometryWithTransformations = null; } break; } diff --git a/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs b/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs index 397d81aeed5b..aec9424a2741 100644 --- a/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs +++ b/src/Uno.UI/UI/Xaml/Media/Geometry.skia.cs @@ -14,7 +14,7 @@ partial class Geometry /// /// Note: Try not to depend on this. See the note in /// - internal virtual SKPath GetUnfilledSKPath() => null; + internal virtual SKPath GetFilledSKPath() => null; internal virtual SkiaGeometrySource2D GetGeometrySource2D() => new SkiaGeometrySource2D(new SKPath(GetSKPath())); } diff --git a/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs b/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs index 05bccafd1387..3ae5b844d3cb 100644 --- a/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs +++ b/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs @@ -7,15 +7,15 @@ partial class PathGeometry { internal override SKPath GetSKPath() => GetSKPath(false); - internal override SKPath GetUnfilledSKPath() => GetSKPath(true); + internal override SKPath GetFilledSKPath() => GetSKPath(true); - internal SKPath GetSKPath(bool skipFilled) + internal SKPath GetSKPath(bool skipUnfilled) { var path = new SKPath(); foreach (var figure in Figures) { - if (skipFilled && figure.IsFilled) + if (skipUnfilled && !figure.IsFilled) { continue; } diff --git a/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs b/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs index 4aecbcd1260b..c28a1948c041 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs @@ -24,11 +24,11 @@ private protected override void Render(SkiaGeometrySource2D? path, double? scale if (Data?.GetUnfilledSKPath() is { } negativePath) { - SpriteShape.NegativeFillGeometry = Visual.Compositor.CreatePathGeometry(new CompositionPath(new SkiaGeometrySource2D(negativePath))); + SpriteShape.FillGeometry = Visual.Compositor.CreatePathGeometry(new CompositionPath(new SkiaGeometrySource2D(negativePath))); } else { - SpriteShape.NegativeFillGeometry = null; + SpriteShape.FillGeometry = null; } } } From 151065def3e08e2633635e0d11575814eabc82a7 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Thu, 7 Nov 2024 03:38:31 +0200 Subject: [PATCH 05/10] chore: reduce allocations --- src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs b/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs index c28a1948c041..09cd20fbf3d7 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Path.skia.cs @@ -7,6 +7,8 @@ namespace Microsoft.UI.Xaml.Shapes { partial class Path : Shape { + private CompositionPathGeometry? _fillGeometry; + /// protected override Size MeasureOverride(Size availableSize) => MeasureAbsoluteShape(availableSize, GetPath()); @@ -22,13 +24,15 @@ private protected override void Render(SkiaGeometrySource2D? path, double? scale { base.Render(path, scaleX, scaleY, renderOriginX, renderOriginY); - if (Data?.GetUnfilledSKPath() is { } negativePath) + _fillGeometry ??= Visual.Compositor.CreatePathGeometry(); + SpriteShape.FillGeometry = _fillGeometry; + if (Data?.GetFilledSKPath() is { } filledPath) { - SpriteShape.FillGeometry = Visual.Compositor.CreatePathGeometry(new CompositionPath(new SkiaGeometrySource2D(negativePath))); + _fillGeometry.Path = new CompositionPath(new SkiaGeometrySource2D(filledPath)); } else { - SpriteShape.FillGeometry = null; + _fillGeometry.Path = null; } } } From 29bd847cdb650b745ce491b1af39dfd9d87d1e5d Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Thu, 7 Nov 2024 03:38:45 +0200 Subject: [PATCH 06/10] test: add tests with different brushes --- .../Helpers/ImageAssert.cs | 3 + .../Windows_UI_Xaml_Shapes/Given_Path.cs | 121 +++++++++++++++++- 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/src/Uno.UI.RuntimeTests/Helpers/ImageAssert.cs b/src/Uno.UI.RuntimeTests/Helpers/ImageAssert.cs index 92dc1453e474..6e35cf77f84b 100644 --- a/src/Uno.UI.RuntimeTests/Helpers/ImageAssert.cs +++ b/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; diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs index 9156a38d2004..d08556f411ec 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs @@ -1,17 +1,20 @@ 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.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 +26,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 +39,116 @@ 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")] + 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")] + public async Task When_PathGeometry_Figures_Not_Filled_ImageBrush() + { + var SUT = new Path + { + Fill = new ImageBrush { ImageSource = "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"); + } } } From df9c6c409cd8f02c3b1516df66bfad6f3e5d72e7 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Thu, 7 Nov 2024 03:51:28 +0200 Subject: [PATCH 07/10] chore: build error --- .../Tests/Windows_UI_Xaml_Shapes/Given_Path.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs index d08556f411ec..85a1e816a6da 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs @@ -4,6 +4,7 @@ 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; @@ -114,7 +115,7 @@ public async Task When_PathGeometry_Figures_Not_Filled_ImageBrush() { var SUT = new Path { - Fill = new ImageBrush { ImageSource = "ms-appx:///Assets/rect.png" }, + Fill = new ImageBrush { ImageSource = new BitmapImage(new Uri("ms-appx:///Assets/rect.png")) }, Data = new PathGeometry { Figures = new PathFigureCollection From 8df4a7149443e6085cce13ba6024a2b01bab6280 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 19 Nov 2024 13:31:30 +0200 Subject: [PATCH 08/10] chore: post rebase --- .../Composition/CompositionSpriteShape.skia.cs | 4 ++-- src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs | 6 +++--- src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs index a9df0122a322..83b89b666b8c 100644 --- a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs +++ b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs @@ -25,9 +25,9 @@ public partial class CompositionSpriteShape : CompositionShape /// a CompositionMaskBrush that (presumably) masks out certain areas. We compensate for this by using this /// geometry as the mask. /// - public CompositionGeometry? FillGeometry + internal CompositionGeometry? FillGeometry { - get => _fillGeometry; + private get => _fillGeometry; set => SetProperty(ref _fillGeometry, value); } diff --git a/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs b/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs index 3ae5b844d3cb..065f51a2a963 100644 --- a/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs +++ b/src/Uno.UI/UI/Xaml/Media/PathGeometry.skia.cs @@ -9,11 +9,11 @@ partial class PathGeometry internal override SKPath GetFilledSKPath() => GetSKPath(true); - internal SKPath GetSKPath(bool skipUnfilled) + private SKPath GetSKPath(bool skipUnfilled) { var path = new SKPath(); - foreach (var figure in Figures) + foreach (PathFigure figure in Figures) { if (skipUnfilled && !figure.IsFilled) { @@ -22,7 +22,7 @@ internal SKPath GetSKPath(bool skipUnfilled) path.MoveTo((float)figure.StartPoint.X, (float)figure.StartPoint.Y); - foreach (var segment in figure.Segments) + foreach (PathSegment segment in figure.Segments) { if (segment is LineSegment lineSegment) { diff --git a/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs b/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs index f516d6adb362..3ebae55b3d95 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Shape.skia.cs @@ -11,7 +11,7 @@ partial class Shape private readonly CompositionSpriteShape _shape; private readonly CompositionPathGeometry _geometry; - protected CompositionSpriteShape SpriteShape => _shape; + private protected CompositionSpriteShape SpriteShape => _shape; public Shape() { @@ -32,7 +32,7 @@ private Rect GetPathBoundingBox(SkiaGeometrySource2D path) private protected override ContainerVisual CreateElementVisual() => Compositor.GetSharedCompositor().CreateShapeVisual(); - private protected void Render(Microsoft.UI.Composition.SkiaGeometrySource2D? path, double? scaleX = null, double? scaleY = null, double? renderOriginX = null, double? renderOriginY = null) + private protected virtual void Render(Microsoft.UI.Composition.SkiaGeometrySource2D? path, double? scaleX = null, double? scaleY = null, double? renderOriginX = null, double? renderOriginY = null) { if (path is null) { From 7063ad76640a7d25d92a67a4d32b840f5aa9d5e9 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 19 Nov 2024 15:10:16 +0200 Subject: [PATCH 09/10] chore: fix tests --- .../Composition/CompositionSpriteShape.skia.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs index 83b89b666b8c..186dec21cc7f 100644 --- a/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs +++ b/src/Uno.UI.Composition/Composition/CompositionSpriteShape.skia.cs @@ -101,9 +101,9 @@ internal override void Paint(in Visual.PaintingSession session) // Get the stroke geometry, after scaling has been applied. geometryWithTransformations.GetFillPath(strokePaint, strokeFillPath); - stroke.UpdatePaint(strokePaint, strokeFillPath.Bounds); + stroke.UpdatePaint(fillPaint, strokeFillPath.Bounds); - session.Canvas.DrawPath(strokeFillPath, strokePaint); + session.Canvas.DrawPath(strokeFillPath, fillPaint); } } } From 70f42acbb36db72b29b518c24efffcf0ec8822a9 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Wed, 20 Nov 2024 14:05:52 +0200 Subject: [PATCH 10/10] chore: ignore tests on !__SKIA__ --- .../Tests/Windows_UI_Xaml_Shapes/Given_Path.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs index 85a1e816a6da..43f0e0fa10c2 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Shapes/Given_Path.cs @@ -43,6 +43,9 @@ public void Should_Not_Include_Control_Points_Bounds() [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 @@ -111,6 +114,9 @@ public async Task When_PathGeometry_Figures_Not_Filled_ColorBrush() [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