Implement gradient brush similar to PathGradientBrush (#969) by mysticfall · Pull Request #989 · SixLabors/ImageSharp

mysticfall · 2019-08-25T12:10:45Z

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

A gradient brush implementation that works similar to System.Drawing.Drawing2D.PathGradientBrush.

This fixes #969, but only convex paths are supported for now.

A gradient brush implementation that works similar to System.Drawing.Drawing2D.PathGradientBrush. This fixes #969, but only convex paths are supported for now.

CLAassistant · 2019-08-25T12:10:50Z

All committers have signed the CLA.

mysticfall · 2019-08-25T12:28:09Z

To maintainers: I think I need some help about this PR. I wrote test cases for this feature, but I don't know what to do with the text fixture (reference output images).

Should I fork SixLabors/Imagesharp.Tests.Images also and make a separate PR? Or maybe I should just attach those images here (which I'll do it just in case)?

Sorry if it's something obvious, but it's the first time I've ever made a PR that spans across multiple Git submodules.

As to the code itself, I think it works as intended for simple convex shapes but it probably won't work for conclave ones, and I haven't compared its output with that of the original System.Drawing.Drawing2D.PathGradientBrush.

There's an implicit restriction to the ILineSegment[] lines constructor argument that it must constitute a closed path (polygon). I thought about adding a validation check, but realized other brush implementations don't seem to do any input validation, so I just leaved it as it is for consistency.

Still, I wanted to ensure that the lines argument would represent a closed path (so that users can omit the last segment). I thought invoking new Polygon(lines) would automatically make it a closed path, as the implementation of Path.AsClosedPath suggests. But it looks like it doesn't work that way, so I also leaved it as it is.

JimBobSquarePants · 2019-08-29T05:05:43Z

@mysticfall If you fork the Imagesharp.Tests.Images.Repo and make a separate PR there we can merge that and update the submodule reference in this PR accordingly.

We're going to simplify all this for the future, it's far too complicated and disjointed a process currently.

mysticfall · 2019-08-29T05:07:07Z

@JimBobSquarePants Sure, I can do that. Thank for the instruction :)

JimBobSquarePants · 2019-08-29T05:19:27Z

src/ImageSharp.Drawing/Processing/PathGradientBrush.cs

+
+                    PointF end = point + (PointF)(direction * this.maxDistance);
+
+                    (Edge edge, PointF intersection) = this.edges


Linq here will negatively affect performance.

Does it involve extra allocations or something similar? I'm not very familiar with internal details about LINQ but I'll try to replace it with a for-iteration, if it's indeed the case.

The performance impacts are coming from stuff like extra allocations, virtual calls, deeper stack etc.

@antonfirsov Thanks for the clarification. I just want to know exactly how and when using LINQ would affect the performance. I've made this PR to be used for my game project, so the knowledge would benefit me in that aspect too.

I'll try to dig deeper into that LINQ chain to see if there's significant overhead before rewriting it to simple for loops.

I'm still not sure if the original code would incur significant overhead, but I just replaced it with a plain foreach-loop just in case.

I'm not a seasoned C# developer, so if anyone could help me understand where the performance caveat lies, or how to find out such a problem, it would be much appreciated.

mysticfall

Submitted a pull request for test fixtures as SixLabors/Imagesharp.Tests.Images#11.

mysticfall · 2019-08-29T05:26:18Z

src/ImageSharp.Drawing/Processing/PathGradientBrush.cs

+
+                    PointF end = point + (PointF)(direction * this.maxDistance);
+
+                    (Edge edge, PointF intersection) = this.edges


Does it involve extra allocations or something similar? I'm not very familiar with internal details about LINQ but I'll try to replace it with a for-iteration, if it's indeed the case.

Test fixtures for SixLabors/ImageSharp#989

antonfirsov · 2019-08-30T16:08:48Z

@mysticfall merged your PR. If you push a submodule update, your tests should pass.

antonfirsov · 2019-08-30T16:15:28Z

src/ImageSharp.Drawing/Processing/PathGradientBrush.cs

+        private static Color CalculateCenterColor(Color[] colors) =>
+            new Color(colors.Select(c => c.ToVector4()).Aggregate((p1, p2) => p1 + p2) / colors.Length);
+
+        private static float DistanceBetween(PointF p1, PointF p2) => ((Vector2)(p2 - p1)).Length();


Squared distance is enough for your purpose. (dx^2 + dy^2 instead of sqrt( dx^2 + dy^2 ))

https://docs.microsoft.com/en-us/dotnet/api/system.numerics.vector4.lengthsquared

Upon inspection, I found that DistanceBetween cannot be modified that way without affecting the output, since the brush relies on a ratio of actual distances.

I replaced Length() used in FindIntersection method as suggested, however, as it's only used to find the closest edge.

antonfirsov · 2019-08-30T16:18:23Z

src/ImageSharp.Drawing/Processing/PathGradientBrush.cs

+                return this.buffer
+                    .Take(intersections)
+                    .Select(p => (point: p, distance: ((Vector2)(p - start)).Length()))
+                    .OrderBy(v => v.distance)


Another optimization hint:
Finding a closest intersection might be enough you don't need to order the whole ordered list.

Removed unnecessary OrderBy invocations. Thanks!

Use LengthSquared() instead of Length() when it's possible. Avoid unnecessary ordering of elements.

codecov · 2019-09-01T02:13:25Z

Codecov Report

Merging #989 into master will increase coverage by 0.02%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
+ Coverage   89.63%   89.66%   +0.02%     
==========================================
  Files        1098     1100       +2     
  Lines       48690    48906     +216     
  Branches     3433     3447      +14     
==========================================
+ Hits        43643    43851     +208     
- Misses       4293     4297       +4     
- Partials      754      758       +4

Impacted Files	Coverage Δ
...eSharp.Tests/Drawing/FillPathGradientBrushTests.cs	`100% <100%> (ø)`
...ImageSharp.Drawing/Processing/PathGradientBrush.cs	`90.9% <90.9%> (ø)`

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 a79c203...39af4c9. Read the comment docs.

antonfirsov

@mysticfall thanks for the changes!
Just one final point: we need arg check in public API-s. If we do it quickly, you'll have a chance to have this feature in beta7 ;)
If you are AFK at the moment I can help you out pushing the change.

Regarding performance impacts of LINQ:
I haven't forgotten your question, just give me a little time to gather my thoughts :) Since you're a Unity dev, I think it is really important for you to be aware of this stuff :)

antonfirsov · 2019-09-01T11:13:08Z

src/ImageSharp.Drawing/Processing/PathGradientBrush.cs

+        /// <param name="colors">Array of colors that correspond to each point in the polygon.</param>
+        /// <param name="centerColor">Color at the center of the gradient area to which the other colors converge.</param>
+        public PathGradientBrush(ILineSegment[] lines, Color[] colors, Color centerColor)
+        {


We need arg. check here:
lines, colors: not null, and counts are matching

Sure, I considered adding them but thought it wasn't a convention after looking at other brush implementations (see my first comment).

Would it be ok if I just throw ArgumentNullException / ArgumentOutOfRangeException as I see fit? And, should I also add associated tests?

P. S.: I'm a Godot developer by the way, so yeah I really should be aware of such things :)

It's done. Please let me know if you want me to change anything else.

…SixLabors#989) * Implement gradient brush similar to PathGradientBrush (SixLabors#969) A gradient brush implementation that works similar to System.Drawing.Drawing2D.PathGradientBrush. This fixes SixLabors#969, but only convex paths are supported for now. * Update submodule to add test fixtures for SixLabors#989 * Performance optimization Use LengthSquared() instead of Length() when it's possible. Avoid unnecessary ordering of elements. * Avoid using LINQ in a hotspot * Validate arguments for the public constructor

Implement gradient brush similar to PathGradientBrush (#969)

a01fef6

A gradient brush implementation that works similar to System.Drawing.Drawing2D.PathGradientBrush. This fixes #969, but only convex paths are supported for now.

JimBobSquarePants requested a review from tocsoft August 29, 2019 05:17

JimBobSquarePants added area:drawing enhancement labels Aug 29, 2019

JimBobSquarePants reviewed Aug 29, 2019

View reviewed changes

Merge branch 'master' into master

eaf39bd

mysticfall mentioned this pull request Aug 30, 2019

Test fixtures for SixLabors/ImageSharp#989 SixLabors/Imagesharp.Tests.Images#11

Merged

mysticfall commented Aug 30, 2019

View reviewed changes

antonfirsov added a commit to SixLabors/Imagesharp.Tests.Images that referenced this pull request Aug 30, 2019

Merge pull request #11 from mysticfall/master

99a2bc5

Test fixtures for SixLabors/ImageSharp#989

Merge branch 'master' into master

7ca829e

antonfirsov reviewed Aug 30, 2019

View reviewed changes

mysticfall added 2 commits August 31, 2019 22:11

Update submodule to add test fixtures for #989

3c62031

Performance optimization

200a428

Use LengthSquared() instead of Length() when it's possible. Avoid unnecessary ordering of elements.

Avoid using LINQ in a hotspot

6565d2b

antonfirsov requested changes Sep 1, 2019

View reviewed changes

Validate arguments for the public constructor

39af4c9

antonfirsov approved these changes Sep 1, 2019

View reviewed changes

JimBobSquarePants mentioned this pull request Sep 1, 2019

Beta7 #993

Merged

8 tasks

antonfirsov merged commit 43d8241 into SixLabors:master Sep 1, 2019


		PointF end = point + (PointF)(direction * this.maxDistance);

		(Edge edge, PointF intersection) = this.edges

Uh oh!

Conversation

mysticfall commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

CLAassistant commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mysticfall commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants commented Aug 29, 2019

Uh oh!

mysticfall commented Aug 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mysticfall Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mysticfall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Aug 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mysticfall Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mysticfall commented Aug 25, 2019 •

edited

Loading

CLAassistant commented Aug 25, 2019 •

edited

Loading

mysticfall commented Aug 25, 2019 •

edited

Loading

mysticfall Sep 1, 2019 •

edited

Loading

antonfirsov Aug 30, 2019 •

edited

Loading

mysticfall Sep 1, 2019 •

edited

Loading

codecov bot commented Sep 1, 2019 •

edited

Loading