Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isStroked overload for IGeometryContext #12617

Closed
wants to merge 3 commits into from
Closed

Conversation

emmauss
Copy link
Contributor

@emmauss emmauss commented Aug 21, 2023

What does the pull request do?

Adds an IGeomtryContextEx interface, with a LineTo method that allow disabling stroke on a line.

What is the current behavior?

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0038648-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

/// </summary>
/// <param name="point">The destination point.</param>
/// <param name="isStroked">Whether the segment is stroked</param>
public void LineTo(Point point, bool isStroked)
Copy link
Contributor

Choose a reason for hiding this comment

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

"hasStroke", "drawStroke" or "includeStroke" might be better names for the parameter. I do prefer "Is" when naming bool; however, "stroked" isn't a standard word. "isStroked" has good symmetry with "isFilled" though... so just ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name was taken from wpf's, but if there's better wording for it, I could change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, if naming is coming over from WPF its fine and I don't have concerns other than noted below (line should always have a stroke I think).

@robloo
Copy link
Contributor

robloo commented Aug 21, 2023

Fundamentally, I'm not sure I understand this PR. In WPF, lines have no Fill -- they are only stroke. Therefore, having an isStroked parameter may be a mistake in API and missing something fundamental.

Since a line has no area, the Path object's Fill is not specified; instead the Stroke and StrokeThickness properties are used.

https://learn.microsoft.com/en-us/dotnet/desktop/wpf/graphics-multimedia/how-to-create-a-line-using-a-linegeometry?view=netframeworkdesktop-4.8

Setting the Fill element for a line has no effect, because a line has no interior.

https://learn.microsoft.com/en-us/dotnet/desktop/wpf/graphics-multimedia/how-to-draw-a-line?view=netframeworkdesktop-4.8

Additionally, Skia doesn't have line fill either.

Path contains three Contours: Line, Circle, and Quad. Line is stroked but not filled. Circle is stroked and filled; Circle stroke forms a loop. Quad is stroked and filled, but since it is not closed, Quad does not stroke a loop.

https://skia.org/docs/user/api/skpath_overview/

@kekekeks kekekeks self-assigned this Aug 21, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0038653-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

In WPF individual parts of the path can be skipped from being filled or stroked. You can ask BeginFigure to not fill the next figure or ask LineTo to not stroke the next path segment.
So it can essentially have two different paths in one geometry instance.
e. g.

            var geo = new StreamGeometry();
            using (var c = geo.Open())
            {
                c.BeginFigure(new Point(10,10), false, true);
                c.LineTo(new Point(20,10), true, true);
                c.LineTo(new Point(20, 20), true, true);
                c.BeginFigure(new Point(40,10), true, true);
                c.LineTo(new Point(50, 10), true, true);
                c.LineTo(new Point(50, 20), false, true);
                c.Close();
            }
            

            var drawing = new GeometryDrawing(Brushes.Blue, new Pen(Brushes.Red, 1), geo);
            Content = new Image
            {
                Source = new DrawingImage(drawing)
            };

results in:
image

We implement that by maintaining two separate SKPath instances per geometry if paths are actually different.

@robloo
Copy link
Contributor

robloo commented Aug 22, 2023

@kekekeks Thanks for the explanation. I understand now and especially since this follows WPF I have no concerns.

@jmacato
Copy link
Member

jmacato commented Nov 17, 2023

Closing this PR temporarily due to inactivity. Please ping me if this needs to be reopened.

@jmacato jmacato closed this Nov 17, 2023
@robloo
Copy link
Contributor

robloo commented Nov 17, 2023

Hi @jmacato I mentioned this last time but etiquette is to comment on the PR asking for status and blockers before closing. Just closing suddenly is very bad practice.

Some PRs are blocked for reasons other than the initial author too I believe.

@timunie
Copy link
Contributor

timunie commented Nov 20, 2023

@robloo I somehow agree for some of the closed PRs. But a PR opened by a team member can be discussed internally. And @emmauss could re-open it on his own.

@robloo
Copy link
Contributor

robloo commented Nov 20, 2023

@timunie Yes, sorry, I don't mean for this PR specifically. It was just the last notification I received so commented here.

They are general comments that perhaps are best left somewhere else. Apologies as I do know you discuss these internally as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants