Expose JointStyle and EndCapStyle on IPen #185

jtjackson · 2021-12-18T03:29:38Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

My implementation of exposing two properties on IPen. Addresses #158
I still need to add some tests to cover the new properties exposed here.

This is my first attempt at contributing to ImageSharp so be patient with me. 🙂

Any feedback is appreciated.

CLAassistant · 2021-12-18T03:29:42Z

All committers have signed the CLA.

codecov · 2021-12-22T05:47:03Z

Codecov Report

Merging #185 (fb7d82c) into master (879f3cf) will increase coverage by 0%.
The diff coverage is 96%.

@@          Coverage Diff          @@
##           master   #185   +/-   ##
=====================================
  Coverage      70%    70%           
=====================================
  Files          87     87           
  Lines        5116   5117    +1     
  Branches     1062   1062           
=====================================
+ Hits         3595   3628   +33     
+ Misses       1307   1279   -28     
+ Partials      214    210    -4

Flag	Coverage Δ
unittests	`70% <96%> (+<1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ImageSharp.Drawing/Shapes/OutlinePathExtensions.cs	`96% <90%> (-1%)`	⬇️
src/ImageSharp.Drawing/Processing/Pen.cs	`100% <100%> (ø)`
...Processing/Processors/Drawing/DrawPathProcessor.cs	`100% <100%> (ø)`
...ssing/Processors/Text/DrawTextProcessor{TPixel}.cs	`97% <100%> (ø)`
...arp.Drawing/Shapes/PolygonClipper/ClipperOffset.cs	`100% <100%> (+12%)`	⬆️
src/ImageSharp.Drawing/Shapes/Clipper/clipper.cs	`50% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 879f3cf...fb7d82c. Read the comment docs.

JimBobSquarePants · 2021-12-22T05:47:25Z

src/ImageSharp.Drawing/Processing/IPen.cs

+        /// <summary>
+        /// Gets the stroke joint style
+        /// </summary>
+        public JointStyle StrokeJoint { get; }


I tend to favor using the actual enum type name as the property name. Means one less thing to remember...

Yeah, usually, I keep the enum type name and property name separate, but as it seems we are only using this once, I'm fine with matching them up here. I'm not feeling creative right now. 🙂

JimBobSquarePants · 2021-12-22T06:16:17Z

src/ImageSharp.Drawing/Shapes/OutlinePathExtensions.cs

+        /// <param name="patternSectionCapStyle">The style to render between sections of the specified pattern.</param>
+        /// <returns>A new path representing the outline.</returns>
+        /// <exception cref="ClipperException">Couldn't calculate offset.</exception>
+        public static IPath GenerateOutline(this IPath path, float width, ReadOnlySpan<float> pattern, JointStyle jointStyle, EndCapStyle patternSectionCapStyle)


We'll need to review these extension methods to ensure that the method chaining covers all sensible combinations. I've done others in the library but not this one yet I think.

That's what I've been trying to figure out. I'm still navigating the tests that we have and determining what I need to do to add to the test coverage.

I know that there's a GenerateOutline usage that you referred to in #155 (comment):

ImageSharp.Drawing/samples/DrawShapesWithImageSharp/Program.cs

Lines 196 to 204 in 3ce36e0

private static void OutputStarOutlineDashed(int points, float inner = 10, float outer = 20, float width = 5, JointStyle jointStyle = JointStyle.Miter, EndCapStyle cap = EndCapStyle.Butt)

{

// center the shape outerRadii + 10 px away from edges

float offset = outer + 10;

var star = new Star(offset, offset, points, inner, outer);

IPath outline = star.GenerateOutline(width, new float[] { 3, 3 }, false, jointStyle, cap);

outline.SaveImage("Stars", $"StarOutlineDashed_{points}_{jointStyle}_{cap}.png");

}

So, I'm trying to wrap my head around how we can use these new extension methods. Should I add more to the sample program?

I think if they inherit well enough then coverage will be implicit. I'll pull down your branch and have a look.

jtjackson · 2022-01-03T17:32:28Z

My apologies. I didn't see the comments until now. :(

I'll follow up tonight. Thanks!

JimBobSquarePants · 2022-01-04T00:45:19Z

My apologies. I didn't see the comments until now. :(

No worries mate! We should all have been taking some time off anyway 😄

…to use the new GenerateOutline extension.

jtjackson · 2022-01-17T13:23:16Z

I added some tests for the new properties. I also took the liberty of updating the sample code so it will use the updated GenerateOutline extension method.

The PR is ready for review. If there is anything I may have missed or overlooked, please let me know. :-)

jtjackson · 2022-01-17T16:17:40Z

Looks like checks failed due to code coverage. I am not familiar with CodeCov here, so can someone clarify what is needed to make this pass?

antonfirsov · 2022-01-17T17:11:21Z

@jtjackson you need to authenticate CodeCov with github to see the coverage change report. Coverage for a GenerateOutline overload call chain has been lost because you are no longer calling the top level overload. It's our fault that we don't have any direct tests for GenerateOutline, so don't worry about it

antonfirsov

We need tests validating the end-to end capability, meaning that the test should actually draw lines with custom JointStyle and EndCapStyle and demonstrate we can solve problems like #155.

You can take a look at DrawPathTests.cs. When running in Debug, the RunValidatingProcessorTestmethod should save debug output to tests/Images/ActualOutput/Drawing/DrawLinesTests/. Initially it will fail. If you are fine with the result output you can copy the images to the matching ReferenceOutput folder, making them pass. You should push the reference output to GIT, but make sure the images are not crazy huge.

antonfirsov · 2022-01-17T17:15:33Z

src/ImageSharp.Drawing/Processing/Pen.cs

+        public JointStyle JointStyle { get; }
+
+        /// <inheritdoc/>
+        public EndCapStyle EndCap { get; }


I don't see any ways to change these properties.

I was following what was used for StokeWidth and StrokePattern properties. We can set the properties via extension methods like GenerateOutline.

Would it be ideal to be able to update the properties in a way like this:

Pen pen = new Pen(); pen.JointStyle = JointStyle.Miter;

Something like that?

We initialize StokeWidth and StrokePattern via costructor. If we want to follow existing patterns, we need to initialize it with in a new constructor overload.

Though I'm tempted to change the whole thing, make Pen non-readonly, removing all the constructors, and making every property settable.

@JimBobSquarePants @tocsoft thoughts?

Pro: no constructor bloat
Con: loosing API symmetry with brushes which are readonly.

@jtjackson don't be blocked by our API design discussion, just pick whatever solution you like, and go ahead with the tests. When we make our decision we can easily refactor.

Yeah... I wanna do a usability review to make sure the APIs are where we need them to be. We can change things there.

Opened #195. Even if it's not where we land in the end, @jtjackson I recommend to go with the settable properties for JointStyle and EndCapStyle for simplicity.

Wasn't sure if to reply here or create a new comment to help with the flow. I had made a comment but decided to move it to the reply here:

Just made the new properties settable. We can change this as needed based on the discussion in #195.

I also uploaded the reference images for the tests in DrawLinesTests.cs. I don't think it's too big (6 KB) but let me know if that's an issue.

It would be pretty too easy for me to go crazy and add a lot more tests (once a QA Engineer, always a QA engineer 🙂) but I think it's a good start unless we think we should add more. Right now, we can see where the JointStyle and EndCap properties are rendered based on the reference images (it's more clear with anti-alias turned on, I would think).

No need for more tests, but I would make the lines somewhat ticker for the JointStyle + EndCap tests for tow reasons:

Make the results more visible + auditable (currently the difference between JointStyle = Round vs Square is not visible)

Make sure it fails when there is a bug, even if the comparison is tolerant

I just updated the images and tests so that we should be able to discern the difference between styles. Let me know if it stands out to you.

One thing I noticed is that some tests failed locally but I can't tell if it's the change or it's my machine. I did try testing against master and it passed. 🤔 I'll debug some more when I get the chance.

Would it be possible for you to pull my changes and run tests on your machine? I suspect that the CI check will also let us know whether my changes passed muster.

jtjackson · 2022-02-02T05:52:30Z

I finally figured out why some of the tests were failing. It was because I added EndCapStyle parameter here:

ImageSharp.Drawing/src/ImageSharp.Drawing/Shapes/OutlinePathExtensions.cs

Lines 88 to 93 in ce91687

    
           public static IPath GenerateOutline(this IPath path, float width, ReadOnlySpan<float> pattern, bool startOff, JointStyle jointStyle = JointStyle.Square, EndCapStyle patternSectionCapStyle = EndCapStyle.Butt) 
        
           { 
        
               if (pattern.Length < 2) 
        
               { 
        
                   return path.GenerateOutline(width, jointStyle: jointStyle, endCapStyle: patternSectionCapStyle); 
        
               }

Which would have a conflict with the call below because the "default" parameter appears to be EndCapStyle.Square:

ImageSharp.Drawing/src/ImageSharp.Drawing/Shapes/OutlinePathExtensions.cs

Lines 200 to 206 in ce91687

    
           public static IPath GenerateOutline(this IPath path, float width, JointStyle jointStyle = JointStyle.Square, EndCapStyle endCapStyle = EndCapStyle.Square) 
        
           { 
        
               var offset = new ClipperOffset(MiterOffsetDelta); 
        
               offset.AddPath(path, jointStyle, endCapStyle); 
        
               return offset.Execute(width); 
        
           }

According to the enum, the default is actually EndCapStyle.Butt. Any reason why it's square instead of butt in the last GenerateOutline method here?

I just removed the EndCapStyle parameter so that it would allow the tests to pass, but I want to make sure that to understand what's happening here. The reference images also have been updated and the comparison looks good but let me know.

…ink_A150_T5.png,tests/Images/ReferenceOutput/Drawing/DrawBezierTests/DrawBeziers_HotPink_A255_T5.png,tests/Images/ReferenceOutput/Drawing/DrawBezierTests/DrawBeziers_Red_A255_T3.png,tests/Images/ReferenceOutput/Drawing/DrawBezierTests/DrawBeziers_White_A255_T1.5.png,tests/Images/ReferenceOutput/Drawing/DrawBezierTests/DrawBeziers_White_A255_T15.png,tests/Images/ReferenceOutput/Drawing/DrawingRobustnessTests/LargeGeoJson_Mississippi_Lines_PixelOffset(0).png,tests/Images/ReferenceOutput/Drawing/DrawingRobustnessTests/LargeGeoJson_Mississippi_Lines_PixelOffset(5500).png,tests/Images/ReferenceOutput/Drawing/DrawLinesTests/DrawLines_Simple_Bgr24_Yellow_A(1)_T(10).png,tests/Images/ReferenceOutput/Drawing/DrawLinesTests/DrawLines_Simple_Rgba32_White_A(0.6)_T(10).png,tests/Images/ReferenceOutput/Drawing/DrawLinesTests/DrawLines_Simple_Rgba32_White_A(1)_T(2.5).png,tests/Images/ReferenceOutput/Drawing/DrawLinesTests/DrawLines_Simple_Rgba32_White_A(1)_T(5)_NoAntialias.png,tests/Images/ReferenceOutput/Drawing/DrawPathTests/PathExtendingOffEdgeOfImageShouldNotBeCropped.png: convert to Git LFS

JimBobSquarePants · 2022-02-08T11:19:25Z

I just removed the EndCapStyle parameter so that it would allow the tests to pass, but I want to make sure that to understand what's happening here. The reference images also have been updated and the comparison looks good but let me know.

@jtjackson You've found a mistake. The default should always match the default in the enumeration.

I've pulled down your fork and have given OutlinePathExtensions a go-over to fix up the method inheritance and documentation. I've also fixes various LFS issues where images were not stored as LFS pointers.

I'll need to get push access to your fork to fix the LFS issues properly though so if you can do that please I can get this completed and merged.

tocsoft · 2022-02-08T12:03:16Z

The default needs to be square, if my memory serves it to match the default style from System.Drawing. The enum isn't one we originally created and that would be why the defaults miss match. However we should probably just change the enum to match the method.

JimBobSquarePants · 2022-02-08T12:13:03Z

Skia uses the same order as we do though.
https://docs.microsoft.com/en-us/dotnet/api/skiasharp.skstrokecap?view=skiasharp-2.80.2

System.Drawing defaults to Flat by the look of things.
https://docs.microsoft.com/en-us/dotnet/api/system.drawing.drawing2d.linecap?view=dotnet-plat-ext-6.0

jtjackson · 2022-02-08T16:46:07Z

I'll need to get push access to your fork to fix the LFS issues properly though so if you can do that please I can get this completed and merged.

@JimBobSquarePants I'm still learning more about how to do PRs on Github (I've done it on private repos, but not public) but it seems that if I checked Allow edit by maintainers, that should give you a push access to my fork?

At any rate, I've added you as a collaborator to my forked repo. Let me know if you have any issues. 🙂

JimBobSquarePants · 2022-02-08T23:14:01Z

@jtjackson You did everything just fine, it's just there's known issues with Git LFS and GitHub that can in some scenarios be worked around but adding new LFS pointers seems not to be one of those scenarios. Only full repo access seems to provide what I need to push the new pointers.

Anyway, I'm very happy with these changes and the new tests look really good also. Thanks very much for your help 👍

Expose JointStyle and EndCapStyle on IPen

7a6e64d

Merge branch 'master' into expose_new_properties_on_ipen

8436ee9

JimBobSquarePants reviewed Dec 22, 2021

View reviewed changes

jtjackson added 3 commits January 6, 2022 00:37

Update property name to match the enum.

60cf9a4

Added tests for the new properties. Also modified the sample program …

d7049c5

…to use the new GenerateOutline extension.

Merge branch 'master' into expose_new_properties_on_ipen

6cd2308

jtjackson marked this pull request as ready for review January 17, 2022 13:20

antonfirsov reviewed Jan 17, 2022

View reviewed changes

antonfirsov mentioned this pull request Jan 18, 2022

Revisit Pen API #195

Closed

jtjackson added 10 commits January 19, 2022 01:18

Make properties settable

36495e7

Add EndCapStyle parameter to the GenerateOutline method call.

c6da73c

Add tests for EndCapStyle and JointStyle properties

b5b6008

Add reference images for the tests.

cbd8875

Fix the summary

13e051a

Add reference images

6700967

Update testss.

824ddc1

Merge branch 'master' into expose_new_properties_on_ipen

ce91687

Remove EndCapStyle parameter as it was causing the tests to break.

f60b60f

Update reference images

40d4f77

JimBobSquarePants added 2 commits February 8, 2022 22:04

Fix method inheritance and update samples

26cc63e

Update build-and-test.yml

fb7d82c

JimBobSquarePants approved these changes Feb 8, 2022

View reviewed changes

JimBobSquarePants merged commit a1b8097 into SixLabors:master Feb 8, 2022

renovate bot mentioned this pull request Mar 7, 2022

Update dependency SixLabors.ImageSharp.Drawing to v1.0.0-beta14 - autoclosed RononDex/Astrobot#76

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose JointStyle and EndCapStyle on IPen #185

Expose JointStyle and EndCapStyle on IPen #185

jtjackson commented Dec 18, 2021 •

edited

Loading

CLAassistant commented Dec 18, 2021 •

edited

Loading

codecov bot commented Dec 22, 2021 •

edited

Loading

JimBobSquarePants Dec 22, 2021

jtjackson Jan 6, 2022

JimBobSquarePants Dec 22, 2021

jtjackson Jan 6, 2022

JimBobSquarePants Jan 17, 2022

jtjackson commented Jan 3, 2022

JimBobSquarePants commented Jan 4, 2022

jtjackson commented Jan 17, 2022

jtjackson commented Jan 17, 2022

antonfirsov commented Jan 17, 2022

antonfirsov left a comment

antonfirsov Jan 17, 2022

jtjackson Jan 17, 2022

antonfirsov Jan 18, 2022 •

edited

Loading

antonfirsov Jan 18, 2022

antonfirsov Jan 18, 2022 •

edited

Loading

JimBobSquarePants Jan 18, 2022

antonfirsov Jan 18, 2022

jtjackson Jan 20, 2022

antonfirsov Jan 20, 2022

jtjackson Jan 26, 2022

jtjackson commented Feb 2, 2022

JimBobSquarePants commented Feb 8, 2022

tocsoft commented Feb 8, 2022

JimBobSquarePants commented Feb 8, 2022

jtjackson commented Feb 8, 2022

JimBobSquarePants commented Feb 8, 2022

	private static void OutputStarOutlineDashed(int points, float inner = 10, float outer = 20, float width = 5, JointStyle jointStyle = JointStyle.Miter, EndCapStyle cap = EndCapStyle.Butt)
	{
	// center the shape outerRadii + 10 px away from edges
	float offset = outer + 10;

	var star = new Star(offset, offset, points, inner, outer);
	IPath outline = star.GenerateOutline(width, new float[] { 3, 3 }, false, jointStyle, cap);
	outline.SaveImage("Stars", $"StarOutlineDashed_{points}_{jointStyle}_{cap}.png");
	}

Expose JointStyle and EndCapStyle on IPen #185

Expose JointStyle and EndCapStyle on IPen #185

Conversation

jtjackson commented Dec 18, 2021 • edited Loading

Prerequisites

Description

CLAassistant commented Dec 18, 2021 • edited Loading

codecov bot commented Dec 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtjackson commented Jan 3, 2022

JimBobSquarePants commented Jan 4, 2022

jtjackson commented Jan 17, 2022

jtjackson commented Jan 17, 2022

antonfirsov commented Jan 17, 2022

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtjackson commented Feb 2, 2022

JimBobSquarePants commented Feb 8, 2022

tocsoft commented Feb 8, 2022

JimBobSquarePants commented Feb 8, 2022

jtjackson commented Feb 8, 2022

JimBobSquarePants commented Feb 8, 2022

jtjackson commented Dec 18, 2021 •

edited

Loading

CLAassistant commented Dec 18, 2021 •

edited

Loading

codecov bot commented Dec 22, 2021 •

edited

Loading

antonfirsov Jan 18, 2022 •

edited

Loading

antonfirsov Jan 18, 2022 •

edited

Loading