Change existing gradient brushes to accept PointF

[lemoncove]

As it is, gradient brushes accept Point for coordinates. They're intrinsically built to handle real numbers, so changing the relevant fields and parameters is all that's necessary.

This also changes how gradient coordinates are sampled—it shifts them 0.5px in both x and y. This is to reflect the new paradigm of real numbers rather than just integral numbers. An example (6x6 images, white to blue gradient, 3px radius):

[antonfirsov]

@jongleur1983 any thoughts on this one?

[jongleur1983]

@antonfirsov thougts yes, but not completely sure about the details, yet.
I approve the idea and the general fix, but I'm unsure about this line:
https://github.com/SixLabors/ImageSharp/pull/865/files#diff-8bb7abb8c26857b45a4411700a7b78c7L84

As the image in the description of this PR above shows, the change affects where the coordinates are seen to be: shifting from the top left edge of a pixel to it's center.
But if I understand the code right, this only affects the pixels to be checked, not the definition of the gradients itself, so the gradient start/end is still the top left corner of the pixels described, while the detection checks the color for the center.
(Else the +0.5f would not be necessary).

Changing the datatypes alone, without that offset, would define the gradient to be from/to the middle of the defined pixels.

I might be wrong here, I'd have to check with manual tests, but might not find the time today.
Perhaps @Poyo-SSB can comment on it?

[lemoncove]

Previously, gradient points were effectively locked to the top-left corner of a pixel. This was fine because gradients were always sampled using the top-left corner as a reference. With the restriction removed, however, this no longer makes sense, as a gradient point in the corner now has practical meaning and pixels span one full unit—the result is a half-pixel offset.

[jongleur1983]

I still think there's an error in that explanation.
Your code keeps the DEFINING points of the gradient as they were before, but you shift the coordinates you use when checking the position on the gradient.

Thus, let's take a linear gradient from 0,0 to 100,0 from white to black.
The original code would have checked 0,0 for the top left pixel - that's the start and thus clear white.
Your code instead would check 0.5, 0.5 instead, which is not the start of the gradient any more, but an offset of 0.5 pixels from 100, thus the first pixel isn't clear white any more.

Switching from int to float as a datatype is okay, but the 0.5/0.5 offset for sampling IMHO is still wrong: we'd either need to use that offset everywhere (for sampling and definition) or nowhere as it was before.

[lemoncove]

Therein lies the problem with the semantic meaning of a float versus an int.

Consider this image of a #FFFFFF → #0000FF gradient starting at the top left corner and ending at the bottom right, at 300px² and at 6px².

In the right image, the value of the highlighted pixel is—indeed—not #FFFFFF (it's #F1F1FF). In the left image, the highlighted 50px² square corresponds to the same area on the right image. The color at the center of that square is also #F1F1FF. If we wanted a pure white at that location, we would want to start at (25px, 25px) on the left and (0.5px, 0.5px) on the right.

I think this change makes sense. Whether or not it's practical, however, is certainly up for consideration. I don't think I can be a good judge of that.

[JimBobSquarePants]

• What does CSS do?
• What does Skia do?
• What does System.Drawing do?

Let's compare our output to reference output and determine accordingly. @Poyo-SSB Your explanation above with example image appears to make sense to me but we should compare.

[jongleur1983]

Thanks for the images along the explanation - yes, it makes sense, and it may be the best way to approximate a correct result (and sorry to be sceptical before; I didn't get to the point to draw samples sophisticated enough to get to the crucial points).

[lemoncove]

Here are the results:

300px² sampled at (25, 25), 6px² sampled at (0, 0).

This PR matches SkiaSharp exactly and CSS very closely. Notably, System.Drawing only allows integer coordinates for its brushes, yielding the same problem that this PR intends to fix. 😉

(The discrepancy between this post and my previous example is because my previous example was made in Photoshop, not ImageSharp. Oops.)

---

CSS (Chrome):

background: linear-gradient(to bottom right, #FFFFFF, #0000FF);

SkiaSharp:

using (var paint = new SKPaint())
{
    paint.Shader = SKShader.CreateLinearGradient(
        new SKPoint(0, 0),
        new SKPoint(size, size),
        new SKColor[] { SKColors.White, SKColors.Blue },
        new float[] { 0, 1 },
        SKShaderTileMode.Clamp);

    canvas.DrawRect(rect, paint);
}

System.Drawing:

var brush = new LinearGradientBrush(
    new Point(0, 0),
    new Point(size, size),
    Color.White,
    Color.Blue);

g.FillRectangle(brush, 0, 0, size, size);

This PR:

var brush = new LinearGradientBrush<Rgba32>(
    new PointF(0, 0),
    new PointF(size, size),
    GradientRepetitionMode.None,
    new ColorStop<Rgba32>(0, Rgba32.White),
    new ColorStop<Rgba32>(1, Rgba32.Blue));

image.Mutate(x => x.Fill(brush));

[JimBobSquarePants]

@Poyo-SSB @jongleur1983 Ah this is fantastic!

Kudos to both of you fo A - Building the initial API which allows easy improvement, B- For figuring out how to improve upon it!

Let's get this building and merged. I'll have a proper look at the CI output later on.

[JimBobSquarePants]

Good to merge once built. It's too late for me to do it now.

[codecov]

Codecov Report

Merging #865 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
- Coverage   88.98%   88.98%   -0.01%     
==========================================
  Files        1017     1017              
  Lines       44466    44464       -2     
  Branches     3228     3228              
==========================================
- Hits        39568    39566       -2     
  Misses       4176     4176              
  Partials      722      722

Impacted Files	Coverage Δ
...harp.Tests/Drawing/FillLinearGradientBrushTests.cs	99.56% <ø> (-0.01%)	⬇️
...rawing/Processing/EllipticGradientBrush{TPixel}.cs	100% <ø> (ø)	⬆️
....Drawing/Processing/RadialGradientBrush{TPixel}.cs	100% <ø> (ø)	⬆️
....Drawing/Processing/LinearGradientBrush{TPixel}.cs	100% <100%> (ø)	⬆️
...rp.Drawing/Processing/GradientBrushBase{TPixel}.cs	95.34% <100%> (ø)	⬆️

---

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 a7f9a8e...ae95dac. Read the comment docs.