Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Enhancement] Shapes #9218

Merged
merged 35 commits into from Jun 8, 2020
Merged

[Enhancement] Shapes #9218

merged 35 commits into from Jun 8, 2020

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Jan 15, 2020

Description of Change

Shapes support is added:

  • Ellipse
  • Rectangle
  • Polygon
  • Line
  • Polyline

Issues Resolved

API Changes

Added:

public class Shape 
{
    public Color Fill { get; set; }

    public Color Stroke { get; set; }

    public double StrokeThickness { get; set; }

    public DoubleCollection StrokeDashArray { get; set; }

    public double StrokeDashOffset { get; set; }

    public Stretch Aspect { get; set; }
}

Ellipse

public sealed class Ellipse : Shape
{

}

Rectangle

public sealed class Rectangle : Shape
{
    public double RadiusX { get; set; }

    public double RadiusY { get; set; }
}

Polygon

public sealed class Polygon : Shape
{
    public PointCollection Points { get; set; }

    public FillRule FillRule { get; set; }
}

Line

public sealed class Line : Shape
{
    public double X1 { get; set; }

    public double Y1 { get; set; }

    public double X2 { get; set; }

    public double Y2 { get; set; }
}

Polyline

public sealed class Polyline : Shape
{
    public PointCollection Points { get; set; }

    public FillRule FillRule { get; set; }
}

Also added:

  • PenLineJoin
  • PenLineCap

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • macOS
  • UWP
  • WPF

Behavioral/Visual Changes

None

Screenshots

ellipse-ios

line-ios

polygon-ios

polyline-ios

rect-ios

linejoin

linecap

shapes-wpf

shapes-macOS

Testing Procedure

Launch Core Gallery and navigate to the new Shapes Gallery.

Notes

  • A Path is the most versatile Shape because can use it to define an arbitrary geometry. But with this versatility comes complexity. Path include a big amount of auxiliary classes (Geometry, etc), etc. To prevent this PR from being very very large (and difficult to review), I am going to divide the Shapes support into two PRs. On the one hand this PR where Ellipse, Rectangle, Line, Polyline and Polygon are added and then another PR where Path will be added.
  • We already had a class called Rectangle. I have called the class that define the Rectangle Shape, Rect. What is the best approach in this case?, rename the original Rectangle class to Rect?, etc.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@programmation
Copy link

I note that the implementation creates a pile of new renderers. How about instead creating a ShapeRenderer<T> where T can provide its own drawing code? Otherwise if I want a Rectangle and an Ellipse I'm going to need 2 Views + their renderers, which is only going to compound the performance problems we have (especially on Android) where we're already using too many Views. I realise that Path may alleviate some of these issues, but that says to me that the other shapes should be implemented in terms of Path rather than leaving Path to last.

@jsuarezruiz jsuarezruiz mentioned this pull request Jan 20, 2020
2 tasks
@bcaceiro
Copy link

@jsuarezruiz Will it be possible to animate the drawing of such shapes? for example, animating drawing a circle or whatsoever. that would be huge in my opinion.

@mattleibow
Copy link
Contributor

Will this support gradients and images for background and borders?

@jsuarezruiz
Copy link
Contributor Author

@bcaceiro Yes, it will be possible to apply animations or transformations to Shapes.
@mattleibow The support for Brushes is work in progress here #9220. After merge that PR (before this one), the Fill and Stroke properties would become a Brush.
About supporting images, do you mean something like ImageBrush: https://www.c-sharpcorner.com/uploadfile/mahesh/using-imagebrush-in-wpf/?. I have done some tests, and it would be possible to have it.

@charlesroddie
Copy link

SkiaSharp is a gigantic lib (12Mb on android)

It's 5MB. Maybe you are not using aab.

@roubachof
Copy link

I'm just looking at the dll size in the nuget package.

image

@samhouts samhouts added this to Done in 4.7.0 May 28, 2020
@samhouts samhouts moved this from Done to In Review in 4.7.0 May 28, 2020
@samhouts samhouts requested a review from rmarinho May 28, 2020 17:21
@samhouts samhouts moved this from In progress to Ready for Review (PRs) in Sprint 171 Jun 2, 2020
Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only reviewed the Core API. it's strange to have some APIs taking start end, and other shapes taking the full space of the view (to be consistent, Line should draw a line from top-left to bottom-right, or all shapes should have positioning and sizing inside of their bounding boxes).

I'm ok to 👍 this as it's behind an experimental flag, but it needs to be addressed at some point.

{
public Ellipse()
{
Aspect = Stretch.Fill;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're not setting a default property here, but a strongly set one, preventing styling. Use defaultValueCreator for that

public static readonly BindableProperty X2Property =
BindableProperty.Create(nameof(X2), typeof(double), typeof(Line), 0.0d);

public static readonly BindableProperty Y2Property =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's quite inconsistent to have Line defined with 2 points, and other shapes with width and height

{
public override object ConvertFromInvariantString(string value)
{
string[] points = value.Split(new char[] { ' ', ',' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we so far only use ',' as list separator in xaml

namespace Xamarin.Forms.Shapes
{
[RenderWith(typeof(_RectangleRenderer))]
public sealed class Rectangle : Shape
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename the file

public static readonly BindableProperty RadiusYProperty =
BindableProperty.Create(nameof(RadiusY), typeof(double), typeof(Rectangle), 0.0d);

public double RadiusX
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't have CornerRadius instead of RX, RY ?

@samhouts samhouts merged commit 9efe531 into 4.7.0 Jun 8, 2020
4.7.0 automation moved this from In Review to Done Jun 8, 2020
Sprint 171 automation moved this from Ready for Review (PRs) to Done Jun 8, 2020
@samhouts samhouts deleted the shapes branch June 8, 2020 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
4.7.0
  
Done
Sprint 170
  
Continued in next sprint
Sprint 171
  
Done
Development

Successfully merging this pull request may close these issues.

[Spec] Shapes & Paths Xamarin.Forms Drawing Spec
10 participants