Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a non-generic Image base class #904

Merged
merged 36 commits into from
May 10, 2019

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented May 5, 2019

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

// The non-generic Image.Load overloads no longer return Image<Rgba32>
using (Image image = Image.Load("foo.jpg"))
{
    image.Mutate(x => x.Resize(100, 100);
    image.SaveAsPng(someStream);
}
  • Touches Make ImageProcessor-s public #621 by making most of the non-generic processors public. (Exception: Histogram processors, because I'm not entirely happy with all namings.)
  • Fix a few doc issues while doing the refactor.
  • While implementing better test coverage for Image.Load variants, discovered and added a few missing overloads

Hope I haven't missed any of the important processors and / or extensions!

To enable extensions using color values as parameters on Image, we need to introduce a pixel-agnostic color type. I have a proposal for this, planning to PR it within the next week.

add suppression of SA1413 to AssemblyInfo.cs
@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #904 into master will decrease coverage by 0.31%.
The diff coverage is 95.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
- Coverage   89.55%   89.23%   -0.32%     
==========================================
  Files        1031     1060      +29     
  Lines       46115    46028      -87     
  Branches     3261     3268       +7     
==========================================
- Hits        41297    41074     -223     
- Misses       4108     4205      +97     
- Partials      710      749      +39
Impacted Files Coverage Δ
...ssing/Processors/Filters/AchromatopsiaProcessor.cs 100% <ø> (ø) ⬆️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 100% <ø> (ø)
.../Processing/Processors/Transforms/SkewProcessor.cs 84.61% <ø> (ø) ⬆️
...ocessing/Processors/Filters/BlackWhiteProcessor.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Image.WrapMemory.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Processing/VignetteExtensions.cs 83.33% <ø> (ø) ⬆️
src/ImageSharp/Processing/DiffuseExtensions.cs 66.66% <ø> (ø) ⬆️
src/ImageSharp/Processing/GlowExtensions.cs 83.33% <ø> (ø) ⬆️
...ocessing/Processors/Filters/TritanopiaProcessor.cs 100% <ø> (ø) ⬆️
...p/Processing/Processors/Filters/InvertProcessor.cs 75% <ø> (ø) ⬆️
... and 291 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 89f3de4...b563167. Read the comment docs.

@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #904 into master will increase coverage by 0.04%.
The diff coverage is 94.6%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #904      +/-   ##
=========================================
+ Coverage   89.55%   89.6%   +0.04%     
=========================================
  Files        1031    1060      +29     
  Lines       46115   46542     +427     
  Branches     3261    3268       +7     
=========================================
+ Hits        41297   41702     +405     
- Misses       4108    4129      +21     
- Partials      710     711       +1
Impacted Files Coverage Δ
...ssing/Processors/Filters/AchromatopsiaProcessor.cs 100% <ø> (ø) ⬆️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 100% <ø> (ø)
.../Processing/Processors/Transforms/SkewProcessor.cs 84.61% <ø> (ø) ⬆️
...ocessing/Processors/Filters/BlackWhiteProcessor.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Image.WrapMemory.cs 100% <ø> (ø) ⬆️
src/ImageSharp/Processing/VignetteExtensions.cs 83.33% <ø> (ø) ⬆️
src/ImageSharp/Processing/DiffuseExtensions.cs 66.66% <ø> (ø) ⬆️
src/ImageSharp/Processing/GlowExtensions.cs 83.33% <ø> (ø) ⬆️
...ocessing/Processors/Filters/TritanopiaProcessor.cs 100% <ø> (ø) ⬆️
...p/Processing/Processors/Filters/InvertProcessor.cs 75% <ø> (ø) ⬆️
... and 247 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 44c4763...390bda8. Read the comment docs.

@JimBobSquarePants
Copy link
Member

I'm very happy for the most part with this thus far. I'd like to know more about your concerns with the Histogram naming and would like to help there.

@antonfirsov
Copy link
Member Author

AdaptiveHistEqualizationSWProcessor - too many abbrevations.

On the other hand, AdaptiveHistogramEqualizationSlidingWindowProcessor would be way too long. I don't know what is the good solution.

@JimBobSquarePants
Copy link
Member

I think we’ve still got a long way to go before we hit the dizzy heights of https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs#L100

So I’m ok with the longer form

@antonfirsov
Copy link
Member Author

Ok, went with the longer names. All processors without color parameters should be public (sealed) now.

@antonfirsov
Copy link
Member Author

I'm merging this to go on with the rest of #907. If I made a mistake, I will be able to fix it in the follow up PR-s.

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

Successfully merging this pull request may close these issues.

2 participants