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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gradient Brushes #542

Merged
merged 54 commits into from May 14, 2018

Conversation

Projects
None yet
5 participants
@jongleur1983
Copy link
Contributor

jongleur1983 commented Apr 19, 2018

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

Implementing Gradient Brushes:

  • Linear Gradient
  • Radial Gradient
  • Elliptical Gradient (supports size, height/length ratios and rotation of the ellipse)

Color Repetition Modes

  • DontFill: fill between start and end of the gradient, don't touch anything else.
  • None: fill between start and end of the gradient. Beyond those borders, stay with the same color
  • Repeat: repeat the gradient endlessly
  • Reflect: same as repeat, but each other repetition is inverted (=reflected).

What's missing

  • proper API documentation

What I'd like to add in future PRs:

  • gradienting modes (e.g. HSV-gradienting. According to @JimBobSquarePants he might implement the corresponding formulas/algorithms somewhere else, so it might just have to be utilized here.
  • performance improvements at least for the common cases
    • Linear Gradients: vertical lin gradients: each row is unicolored, so we have to calculate the color only once per row. Horizontal gradients: not sure how best to do it, but here each row is the same bunch of pixels, so we can cache it between the Apply calls.
    • Radial Gradients: from the center point towards the edges, each row is mirrored, so only half of the colors have to be calculated.
  • Use different BrushApplicators to speed up the special cases (e.g. Elliptic Brush with AxisRatio 1 is a RadialBrush)
  • Another gradient Brush: StarShapedGradient: Consider a Polygon and a point inside, where the direct connection between that point and any position along the outline does not intersect the outline before. The class of these polygons contains all convex polygons, simple stars and more. Given a Star-Shaped polygon and one point inside where the condition described above holds, it's possible to color each point in an unambiguous way referring to a gradient description. ColorRepetitionMode.Reflect and ColorRepetitionMode.Repeat would not be supported here as that get's ambiguous outside the polygon.
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Apr 19, 2018

CLA assistant check
All committers have signed the CLA.

@tocsoft tocsoft self-requested a review Apr 19, 2018

@tocsoft tocsoft self-assigned this Apr 19, 2018

@jongleur1983 jongleur1983 force-pushed the jongleur1983:GradientBrush branch 6 times, most recently from d284b76 to bc6e35c Apr 19, 2018

@jongleur1983 jongleur1983 force-pushed the jongleur1983:GradientBrush branch from bc6e35c to c02f9d8 Apr 19, 2018

@jongleur1983 jongleur1983 changed the title #86: started to work on Gradient Brushes: Linear gradient brush, #86: Linear gradient brush Apr 19, 2018


private readonly Point end;

private readonly Tuple<float, TPixel>[] colorStops;

This comment has been minimized.

@antonfirsov

antonfirsov Apr 19, 2018

Member

I suggest to define a small private struct instead of Tuple<float, TPixel>. It provides:

  • Better readability
  • Better memory locality (Tuple<>-s are classes on the heap)

This comment has been minimized.

@jongleur1983

jongleur1983 Apr 19, 2018

Contributor

good point. Thanks. Except that the struct can't be private as it's used as a parameter in the Brush constructor. Therefore I'll use a public nested struct instead.

@tocsoft
Copy link
Member

tocsoft left a comment

the reason you are getting a black screen is because of a bug in the base Apply method.

the test if (this.Options.BlendPercentage < 1) is incorrect and instead should read if (this.Options.BlendPercentage <= 1) i.e. less than or equal to instead of less than

Assert.Equal(Rgba32.Red, sourcePixels[0, 0]);
Assert.Equal(Rgba32.Red, sourcePixels[9, 9]);
Assert.Equal(Rgba32.Red, sourcePixels[199, 149]);
Assert.Equal(Rgba32.Red, sourcePixels[500, 500]);

This comment has been minimized.

@tocsoft

tocsoft Apr 19, 2018

Member

this need to be 499 not 500 as its one pixel wider than the buffer.

This comment has been minimized.

@jongleur1983

jongleur1983 Apr 19, 2018

Contributor

thanks, you're right. fixed.

@tocsoft tocsoft changed the title #86: Linear gradient brush [WIP] Linear gradient brush Apr 19, 2018

@tocsoft

This comment has been minimized.

Copy link
Member

tocsoft commented Apr 19, 2018

I've renamed the PR, we will happily keep reviewing it as you make improvements, once your fully happy and want a final review/merge just update the title again and drop the [WIP] prefix

jongleur1983 added some commits Apr 19, 2018

FIX bug in BrushApplicator when applying BlendPercentage
thanks @tocsoft for investigation and finding the bug.
He proposed to just replace < by <= in line 78, but that would only shift the problem to values > 1.
Those values should not be used from a semantic point, but it's not forbidden in a float value.
My fix here keeps the <, but adds an else path that uses the original value from scanline[i]. This adds an else to the code (which technically adds a jump after the then-part), but omits the multiplication.
The simple solution @tocsoft proposed might be faster, but should be preferred if and only if BlendPercentage can be guaranteed to be <=1.
#542: use struct for ColorStops
as proposed by @antonfirsov: improving readability and memory locality.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #542 into master will decrease coverage by 0.4%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   87.67%   87.26%   -0.41%     
==========================================
  Files         838      840       +2     
  Lines       34684    35107     +423     
  Branches     2533     2553      +20     
==========================================
+ Hits        30410    30637     +227     
- Misses       3524     3719     +195     
- Partials      750      751       +1
Impacted Files Coverage 螖
.../Processing/Drawing/Brushes/LinearGradientBrush.cs 100% <100%> (酶)
...harp.Tests/Drawing/FillLinearGradientBrushTests.cs 100% <100%> (酶)
...wing/Processing/Drawing/Brushes/BrushApplicator.cs 86.95% <100%> (+48.86%) 猬嗭笍
...mats/Generated/Rgba32.PixelOperations.Generated.cs 75% <0%> (-25%) 猬囷笍
...ats/Generated/PixelOperations{TPixel}.Generated.cs 80% <0%> (-20%) 猬囷笍
src/ImageSharp/PixelFormats/Bgr24.cs 78.26% <0%> (-19.04%) 猬囷笍
src/ImageSharp/PixelFormats/Rgb24.cs 75.55% <0%> (-18.89%) 猬囷笍
src/ImageSharp/PixelFormats/Alpha8.cs 82.5% <0%> (-17.5%) 猬囷笍
src/ImageSharp/PixelFormats/NormalizedByte2.cs 83.95% <0%> (-16.05%) 猬囷笍
src/ImageSharp/PixelFormats/NormalizedShort2.cs 82.71% <0%> (-15.82%) 猬囷笍
... and 23 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 07a811c...d29b601. Read the comment docs.

jongleur1983 added some commits Apr 20, 2018

#542: reduce test output image sizes
test images don't have to be that big for axial gradients.
It's sufficient to show they're constant across the axis and correct along the axis.
#542: fix implementation of non-axial gradients and add tests
the theory lacks color checks yet that should be added, but it produces output images for visual control
#542: add tests for multi-color gradients
somehow these don't look correct yet, containing hard edges sometimes.

JimBobSquarePants added some commits May 8, 2018

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 11, 2018

Where are we with this PR? I'm seeing lots of reference images missing.

https://ci.appveyor.com/project/six-labors/imagesharp/build/1.0.0-PullRequest00542001291/job/g37aqb2n440iun56

@jongleur1983

This comment has been minimized.

Copy link
Contributor

jongleur1983 commented May 11, 2018

@JimBobSquarePants you're right. To lower the burden on you all I wanted to self-review my test cases once again before asking to add the reference images.
Current state is: the tests are correct and the current images the code produce could be taken as reference images, but I have to review if the test set is reasonable (not too much redundant tests, no relevant test cases missing).
I hope I'll get to it in the evening today.

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 11, 2018

@jongleur1983 Ok no worries, I wanted to make sure you didn't need anything from us.

Looking forward to seeing this merged in, the output looks amazing!

@jongleur1983 jongleur1983 changed the title [WIP] Gradient Brushes Gradient Brushes May 11, 2018

@jongleur1983

This comment has been minimized.

Copy link
Contributor

jongleur1983 commented May 11, 2018

I'm not satisfied with the state of the documentation, but I fear I'm blind to what's needed for someone not familiar with the API.
Documentation is in place, but I'm not sure if it's descriptive enough.
Any hints on what to improve are welcome. If it's considered "good enough" feel free to merge now.

Reference Output of the test cases has changed slightly: Not the content of the individual images, but the images itself due to changed file names and slightly different test cases. These would have to be copied to the external repository again.

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 11, 2018

@jongleur1983 thanks!
Gonna fix up your tests a little bit and push the reference images.

antonfirsov added some commits May 11, 2018

@jongleur1983

This comment has been minimized.

Copy link
Contributor

jongleur1983 commented May 13, 2018

@antonfirsov thanks for the changes. I still have 71 failing tests locally. On the first sight those are the same we thought to fail due the Vector2 issue, and: the same tests fail on SixLabors/master, too here.

So I'm fine with merging the branch.

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 13, 2018

Everything looks good now @jongleur1983's local test running issues are unrelated to his PR.

I'm planning to merge this soon, let me know if there are blocker issues we've missed, so I shouldn't!

/// Base class for Gradient brushes
/// </summary>
/// <typeparam name="TPixel">The pixel format</typeparam>
public abstract class AbstractGradientBrush<TPixel> : IBrush<TPixel>

This comment has been minimized.

@antonfirsov

antonfirsov May 13, 2018

Member

Ok, one naming question mostly to @JimBobSquarePants :
Shouldn't we name this GradientBrushBase or simply GradientBrush to match our current naming patterns?

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 14, 2018

Member

Yeah this should be GradientBrushBase.

This comment has been minimized.

@jongleur1983

jongleur1983 May 14, 2018

Contributor

done (and renamed the BrushApplicator as well)

@antonfirsov antonfirsov merged commit dbe2b1b into SixLabors:master May 14, 2018

5 checks passed

codecov/patch 98.98% of diff hit (target 88.63%)
Details
codecov/project 88.78% (+0.15%) compared to ad57692
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 14, 2018

Finally merged!
@jongleur1983 thanks for pushing the rename!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment