Remove Region and add new Clip Extension #150

JimBobSquarePants · 2021-07-10T04:39:34Z

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

Fixes #105

Removes all references to Region and inheriting types.
Adds new Clip(IImageProcessingContext, IPath, Action<IImageProcessingContext>) extension with functionality copied from samples

codecov · 2021-07-10T04:42:44Z

Codecov Report

Merging #150 (9a6a0b1) into master (1bf8714) will increase coverage by 0.00%.
The diff coverage is 98.43%.

@@           Coverage Diff           @@
##           master     #150   +/-   ##
=======================================
  Coverage   69.86%   69.87%           
=======================================
  Files          90       88    -2     
  Lines        5141     5135    -6     
  Branches     1049     1048    -1     
=======================================
- Hits         3592     3588    -4     
+ Misses       1338     1337    -1     
+ Partials      211      210    -1

Flag	Coverage Δ
unittests	`69.87% <98.43%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/ImageSharp.Drawing/Processing/ImageBrush.cs	`87.75% <93.10%> (+0.25%)`	⬆️
...p.Drawing/Processing/Extensions/ClearExtensions.cs	`100.00% <100.00%> (ø)`
...awing/Processing/Extensions/ClearPathExtensions.cs	`100.00% <100.00%> (ø)`
.../Processing/Extensions/ClearRectangleExtensions.cs	`100.00% <100.00%> (ø)`
...rawing/Processing/Extensions/ClipPathExtensions.cs	`100.00% <100.00%> (ø)`
...rp.Drawing/Processing/Extensions/FillExtensions.cs	`100.00% <100.00%> (ø)`
...Processing/Extensions/FillPathBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...rawing/Processing/Extensions/FillPathExtensions.cs	`100.00% <100.00%> (ø)`
...Processing/Processors/Drawing/ClipPathProcessor.cs	`100.00% <100.00%> (ø)`
...ng/Processors/Drawing/ClipPathProcessor{TPixel}.cs	`100.00% <100.00%> (ø)`
... and 10 more

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 1bf8714...9a6a0b1. Read the comment docs.

src/ImageSharp.Drawing/Processing/Processors/Text/DrawTextProcessor{TPixel}.cs

src/ImageSharp.Drawing/Processing/Extensions/FillPathExtensions.cs

src/ImageSharp.Drawing/Processing/Processors/Drawing/RecursiveImageProcessor.cs

…s.cs Co-authored-by: Scott Williams <[email protected]>

…ImageProcessor.cs Co-authored-by: Scott Williams <[email protected]>

src/ImageSharp.Drawing/Processing/Processors/Drawing/RecursiveImageProcessor.cs

enable image brush to be only draw a portion of the source image

antonfirsov

Implementation and tests look good.

We need to clean up the terminology used in API naming and docs, because currently it feels confusing to me. The problem is that IPath can be used for many things in the Drawing library, it's name doesn't carry the purpose (which I personally dislike, but it's a common practice in drawing libraries). My recommendation is to use the term "Region" (or an equivalent unambiguously descriptive term) in parameter/property names & docs for IPath defining a region to fill, or boundary for a recursive processor.

We also need to find a better name for the new Clip method.

Update: Taking another look, I don't dislike Clip that much anymore, but I think we should still consider alternatives.

antonfirsov · 2021-07-24T12:12:47Z

src/ImageSharp.Drawing/Processing/Extensions/ClearPathExtensions.cs

+        public static IImageProcessingContext Clear(
+            this IImageProcessingContext source,
+            Color color,
+            IPath path) =>


I would consider renaming path to region everywhere.

src/ImageSharp.Drawing/Processing/Extensions/ClearPathExtensions.cs

antonfirsov · 2021-07-24T12:24:44Z

src/ImageSharp.Drawing/Processing/Processors/Drawing/FillPathProcessor.cs

@@ -32,9 +36,9 @@ public FillPathProcessor(DrawingOptions options, IBrush brush, IPath shape)
        public IBrush Brush { get; }

        /// <summary>
-        /// Gets the region that this processor applies to.
+        /// Gets the logic path that this processor applies to.


Suggested change

/// Gets the logic path that this processor applies to.

/// Gets the <see cref="Path"/> defining the region to fill.

antonfirsov · 2021-07-24T12:25:52Z

src/ImageSharp.Drawing/Processing/Processors/Drawing/FillPathProcessor.cs

        /// </summary>
-        public IPath Shape { get; }
+        public IPath Path { get; }


I think "region" is a better name when it's about filling:

Suggested change

public IPath Path { get; }

public IPath Region { get; }

antonfirsov · 2021-07-24T12:32:34Z

src/ImageSharp.Drawing/Processing/Processors/Drawing/RecursiveImageProcessor.cs

+    /// <summary>
+    /// Allows the recursive application of processing operations against an image.
+    /// </summary>
+    public class RecursiveImageProcessor : IImageProcessor


ProcessInsideRegionProcessor?

antonfirsov · 2021-07-24T12:35:17Z

src/ImageSharp.Drawing/Processing/Extensions/ClipPathExtensions.cs

+        /// <param name="path">The target path to operate within.</param>
+        /// <param name="operation">The operation to perform on the source.</param>
+        /// <returns>The <see cref="IImageProcessingContext"/> to allow chaining of operations.</returns>
+        public static IImageProcessingContext Clip(


I think Clip is a very generic overloaded and ambiguous name, doesn't help API discoverability in this case.

I wouldn'be afraid of going verbose here, for example ProcessInsideRegion would be a better name for this.

src/ImageSharp.Drawing/Processing/Processors/Drawing/RecursiveImageProcessor.cs

…ns.cs Co-authored-by: Anton Firszov <[email protected]>

…arp.Drawing into js/region-clip

JimBobSquarePants · 2021-07-25T15:27:22Z

@antonfirsov I've applied a lot of renaming and updated XML docs based on your suggestions. I found that I was straying into #94 territory though as I waded through each connected method so have stopped where I am for now.

I maintained Clip as the method name (I think people will get it) and renamed the processor ClipPathProcessor

JimBobSquarePants · 2021-07-30T15:06:07Z

I'm merging this. We can do a final API review before RC1

JimBobSquarePants added 2 commits July 10, 2021 01:36

Rip out region.

dfba96f

Add new Clip extension and tests

b69365e

JimBobSquarePants added the API label Jul 10, 2021

JimBobSquarePants added this to the 1.0.0-rc.1 milestone Jul 10, 2021

JimBobSquarePants requested review from tocsoft and antonfirsov July 10, 2021 07:43

Update DrawTextProcessor{TPixel}.cs

80508c7

JimBobSquarePants commented Jul 10, 2021

View reviewed changes

src/ImageSharp.Drawing/Processing/Processors/Text/DrawTextProcessor{TPixel}.cs Outdated Show resolved Hide resolved

JimBobSquarePants added 2 commits July 15, 2021 01:15

Merge branch 'master' into js/region-clip

a4c39f2

Merge branch 'master' into js/region-clip

04dd529

tocsoft reviewed Jul 16, 2021

View reviewed changes

JimBobSquarePants and others added 6 commits July 16, 2021 18:21

Update src/ImageSharp.Drawing/Processing/Extensions/FillPathExtension…

3b8732f

…s.cs Co-authored-by: Scott Williams <[email protected]>

Update src/ImageSharp.Drawing/Processing/Extensions/FillPathExtension…

08c741a

…s.cs Co-authored-by: Scott Williams <[email protected]>

Update src/ImageSharp.Drawing/Processing/Processors/Drawing/Recursive…

08d8550

…ImageProcessor.cs Co-authored-by: Scott Williams <[email protected]>

Avoid crop

fe045b5

Fix comment

f4be2f7

Better tests

07c3bf1

tocsoft reviewed Jul 16, 2021

View reviewed changes

src/ImageSharp.Drawing/Processing/Processors/Drawing/RecursiveImageProcessor.cs Outdated Show resolved Hide resolved

antonfirsov self-assigned this Jul 22, 2021

enable image brush to be only draw a portion of the source image

5743bf9

tocsoft mentioned this pull request Jul 23, 2021

enable image brush to be only draw a portion of the source image #154

Merged

Merge pull request #154 from SixLabors/sw/self-cliping-image-brush

7686e91

enable image brush to be only draw a portion of the source image

antonfirsov reviewed Jul 24, 2021

View reviewed changes

JimBobSquarePants and others added 5 commits July 25, 2021 23:30

Update src/ImageSharp.Drawing/Processing/Extensions/ClearPathExtensio…

b543d83

…ns.cs Co-authored-by: Anton Firszov <[email protected]>

Update src/ImageSharp.Drawing/Processing/Extensions/ClearPathExtensio…

dfd59bb

…ns.cs Co-authored-by: Anton Firszov <[email protected]>

Cleanup

5d5cf96

Merge branch 'js/region-clip' of https://github.com/SixLabors/ImageSh…

86348a0

…arp.Drawing into js/region-clip

More clean up and update tests

9a6a0b1

JimBobSquarePants merged commit b58a245 into master Jul 30, 2021

JimBobSquarePants deleted the js/region-clip branch July 30, 2021 15:06

JimBobSquarePants added the breaking label Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Region and add new Clip Extension #150

Remove Region and add new Clip Extension #150

JimBobSquarePants commented Jul 10, 2021

codecov bot commented Jul 10, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov Jul 24, 2021 •

edited

Loading

antonfirsov Jul 24, 2021

antonfirsov Jul 24, 2021

antonfirsov Jul 24, 2021

antonfirsov Jul 24, 2021

JimBobSquarePants commented Jul 25, 2021

JimBobSquarePants commented Jul 30, 2021

	/// Gets the logic path that this processor applies to.
	/// Gets the <see cref="Path"/> defining the region to fill.

Remove Region and add new Clip Extension #150

Remove Region and add new Clip Extension #150

Conversation

JimBobSquarePants commented Jul 10, 2021

Prerequisites

Description

codecov bot commented Jul 10, 2021 • edited Loading

Codecov Report

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov Jul 24, 2021 • edited Loading

Choose a reason for hiding this comment

antonfirsov Jul 24, 2021

Choose a reason for hiding this comment

antonfirsov Jul 24, 2021

Choose a reason for hiding this comment

antonfirsov Jul 24, 2021

Choose a reason for hiding this comment

antonfirsov Jul 24, 2021

Choose a reason for hiding this comment

JimBobSquarePants commented Jul 25, 2021

JimBobSquarePants commented Jul 30, 2021

codecov bot commented Jul 10, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov Jul 24, 2021 •

edited

Loading