Pixel-agnostic Drawing processors and extensions#910
Conversation
# Conflicts: # src/ImageSharp/Color/Color.Conversions.cs
Codecov Report
@@ Coverage Diff @@
## master #910 +/- ##
==========================================
- Coverage 89.29% 89.13% -0.16%
==========================================
Files 1079 1083 +4
Lines 47631 47394 -237
Branches 3273 3293 +20
==========================================
- Hits 42530 42243 -287
- Misses 4387 4398 +11
- Partials 714 753 +39
Continue to review full report at Codecov.
|
tocsoft
left a comment
tocsoft
left a comment
There was a problem hiding this comment.
looks go to me, nothing jumping out to me
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Really great stuff. A massive improvement in the surface API.
I'm happy for this to go in, but have added a few questions for you to ponder over.
| /// <param name="ratio">Where should it be? 0 is at the start, 1 at the end of the Gradient.</param> | ||
| /// <param name="color">What color should be used at that point?</param> | ||
| public ColorStop(float ratio, TPixel color) | ||
| public ColorStop(float ratio, in Color color) |
There was a problem hiding this comment.
Love that we can now use readonly structs here.
| public static DenseMatrix<TPixel> ToPixelMatrix<TPixel>(this DenseMatrix<Color> colorMatrix, Configuration configuration) | ||
| where TPixel : struct, IPixel<TPixel> | ||
| { | ||
| DenseMatrix<TPixel> result = new DenseMatrix<TPixel>(colorMatrix.Columns, colorMatrix.Rows); |
| /// <typeparam name="TPixel">The type of the color.</typeparam> | ||
| public interface IPen<TPixel> : IPen | ||
| where TPixel : struct, IPixel<TPixel> | ||
| public interface IPen |
| Image<TPixel> image, | ||
| RectangleF region, | ||
| GraphicsOptions options, | ||
| bool shouldDisposeImage) |
There was a problem hiding this comment.
When do we need to dispose the source?
There was a problem hiding this comment.
If we draw an image of a different pixel type, we create a temporary copy, which should be disposed by the brush applicator.
| float distance = (float)Math.Sqrt( | ||
| Math.Pow(x4 - this.start.X, 2) | ||
| + Math.Pow(y4 - this.start.Y, 2)); | ||
| float distance = (float)Math.Sqrt(Math.Pow(x4 - this.start.X, 2) + Math.Pow(y4 - this.start.Y, 2)); |
There was a problem hiding this comment.
Hope I won't forget this next time I touch the code.
| /// </summary> | ||
| /// <typeparam name="TPixelBg">The pixel format of destination image.</typeparam> | ||
| /// <typeparam name="TPixelFg">The pixel format of source image.</typeparam> | ||
| internal class DrawImageProcessor<TPixelBg, TPixelFg> : ImageProcessor<TPixelBg> |
There was a problem hiding this comment.
This is an interesting one. I personally find Fg and Bg far less confusing than the compositing standard of Src and Dst but others might argue the inconsistency in language causes confusion.
They're wrong of course 😄
There was a problem hiding this comment.
That naming totally confused me, had to rename this in order to avoid making a mistake.
| where TPixel : struct, IPixel<TPixel> | ||
| => source.Fill(new SolidBrush<TPixel>(color), shape); | ||
| public static IImageProcessingContext | ||
| Fill(this IImageProcessingContext source, Color color, RectangleF shape) => |
There was a problem hiding this comment.
Should we incorporate the overload performance fixes from #838 here? Merging after could cause conflicts.
There was a problem hiding this comment.
Wow, did notice that. We should merge that PR ASAP, I'll have a look in the next few days.
There was a problem hiding this comment.
I'll try to create better overloads another day :)
| where TPixel : struct, IPixel<TPixel> | ||
| { | ||
| TPixel red = NamedColors<TPixel>.Red; | ||
| Color red = Color.Red; |
There was a problem hiding this comment.
You can clearly see how much nicer the API is to work with in the tests.
|
I'm merging this now, because I have more stuff in my queue. We can apply #838 later. |
Pixel-agnostic Drawing processors and extensions
4 participants
Prerequisites
Description
Continue work on #907 by getting rid of generics in drawing API-s:
IBrushandIPenare no longer generic. All pixel-specific work is delegated toBrushApplicator<TPixel>Coloris being used as color parameter for drawing processorsImageof different pixel type inImageBrush. (A clone image is created in these cases for the applicator.)IImageProcessor<T>usages have been dropped from tests and benchmarksNot in scope
Removing all obsolete generic types. A cleanup PR will follow.