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

Processors refactoring #983

Merged
merged 40 commits into from
Aug 25, 2019
Merged

Processors refactoring #983

merged 40 commits into from
Aug 25, 2019

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Aug 19, 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

This does some ground work for #967.
I'm creating this as a draft PR as I'm unsure about a couple of things, and there are a few lines that cause the current branch to fail to build. I've done most of the refactoring following step by step the instructions for the "Proposal to refactor current ImageProcessor logic" section.
Need some feedback to continue with this, and of course everyone else is free to just jump in and finish the remaining details as well 😄

What I did so far

  • Added Image<TPixel> Source and Rectangle SourceRectangle as protected properties to the ImageProcessor<TPixel> class
  • Also added a Configuration property (retrieved from the image)
  • Modified the ImageProcessor<TPixel> constructor to take an image and rectangle
  • Refactored constructors of all inherited processors to take and bubble up the image and rectangle
  • Modified CreatePixelSpecificProcessor to take target image and rectangle
  • Removed parameters from the Apply method, as they're now members
  • Did the same for the CloningImageProcessor<TPixel> and inherited processors
  • Removed duplicate parameters from all ImageProcessor<TPixel> methods (Apply, BeforeApply, AfterFrameApply and all the others)
  • Refactored the DefaultImageProcessingContext class to use the new architecture

Updated master from SixLabors/ImageSharp
@JimBobSquarePants
Copy link
Member

@Sergio0694 I'll do some work on this later to get it building. Then we'll see where we are.

It'll be wise to add a checkbox list to the description here to tick off what you have done so far.

@Sergio0694
Copy link
Member Author

Hey @JimBobSquarePants - I've just updated the PR description with all the info you requested, hope it helps! 😊

@JimBobSquarePants
Copy link
Member

@Sergio0694 I got the source building, test still not yet though. If you could get them building I could then crack on with implementing IDIsposable.

@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

Merging #983 into master will increase coverage by <.01%.
The diff coverage is 96.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
+ Coverage   89.71%   89.71%   +<.01%     
==========================================
  Files        1097     1097              
  Lines       48538    48576      +38     
  Branches     3421     3421              
==========================================
+ Hits        43546    43582      +36     
- Misses       4289     4291       +2     
  Partials      703      703
Impacted Files Coverage Δ
...ng/Processors/Convolution/EdgeDetectorProcessor.cs 100% <ø> (ø) ⬆️
...rs/Normalization/HistogramEqualizationProcessor.cs 83.87% <ø> (ø) ⬆️
...ing/Processors/Dithering/PaletteDitherProcessor.cs 100% <ø> (ø) ⬆️
...ocessors/Quantization/QuantizeProcessor{TPixel}.cs 0% <0%> (ø) ⬆️
src/ImageSharp/Advanced/AotCompilerTools.cs 0% <0%> (ø) ⬆️
...ssing/Processors/Quantization/QuantizeProcessor.cs 0% <0%> (ø) ⬆️
...ing/Processors/Transforms/CropProcessor{TPixel}.cs 100% <100%> (ø) ⬆️
...ogramEqualizationSlidingWindowProcessor{TPixel}.cs 100% <100%> (ø) ⬆️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 100% <100%> (ø) ⬆️
...ng/Processors/Overlays/BackgroundColorProcessor.cs 100% <100%> (ø) ⬆️
... and 86 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 79f5ff5...41470e8. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

Merging #983 into master will increase coverage by <.01%.
The diff coverage is 96.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
+ Coverage   89.71%   89.72%   +<.01%     
==========================================
  Files        1097     1097              
  Lines       48538    48573      +35     
  Branches     3421     3421              
==========================================
+ Hits        43546    43581      +35     
  Misses       4289     4289              
  Partials      703      703
Impacted Files Coverage Δ
...ng/Processors/Convolution/EdgeDetectorProcessor.cs 100% <ø> (ø) ⬆️
...rs/Normalization/HistogramEqualizationProcessor.cs 83.87% <ø> (ø) ⬆️
...ing/Processors/Dithering/PaletteDitherProcessor.cs 100% <ø> (ø) ⬆️
...ocessors/Quantization/QuantizeProcessor{TPixel}.cs 0% <0%> (ø) ⬆️
src/ImageSharp/Advanced/AotCompilerTools.cs 0% <0%> (ø) ⬆️
...ssing/Processors/Quantization/QuantizeProcessor.cs 0% <0%> (ø) ⬆️
...ing/Processors/Transforms/CropProcessor{TPixel}.cs 100% <100%> (ø) ⬆️
...ogramEqualizationSlidingWindowProcessor{TPixel}.cs 100% <100%> (ø) ⬆️
...s/Convolution/Convolution2PassProcessor{TPixel}.cs 100% <100%> (ø) ⬆️
...ng/Processors/Overlays/BackgroundColorProcessor.cs 100% <100%> (ø) ⬆️
... and 85 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 79f5ff5...88e97cb. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find any major issue.
Since the changes are consistent, we can merge this as is and continue work in an another PR. @JimBobSquarePants @Sergio0694 how do you want to proceed?

@Sergio0694 Sergio0694 marked this pull request as ready for review August 24, 2019 20:20
@Sergio0694
Copy link
Member Author

@antonfirsov alright, I've fixed that small leftover you mentioned and marked the PR as ready for review, so you're free to merge this now if you want 👍

@JimBobSquarePants
Copy link
Member

Ok, let’s merge this as-is then and I’ll open a new PR for using IDisposable since I’ve already made a start.

@JimBobSquarePants JimBobSquarePants merged commit 13989ac into SixLabors:master Aug 25, 2019
antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
* Added new properties to ImageProcessor<TPixel>

* Fixed constructors for convolution processors

* Fixed constructors for binarization processors

* Fixed constructor for dithering processor

* Fixed constructors for effects processors

* Fixed constructor for filter processor

* Fixed constructor for normalization processor

* Fixed constructors for overlay processors

* Fixed constructor for quantization processor

* Fixed constructors for transforms processors

* Updated CreatePixelSpecificProcessor<TPixel> definition

* Fixed convolution processors creation

* Fixed leftover dithering processor constructor

* Fixed another leftover dithering processor constructor

* Fixed dithering processors creation

* Fixed effects processors creation

* Fixed filters processor

* Fixed normalization processors creation

* Fixed overlays processors creation

* Fixed quantizer processor creation

* Fixed constructors for some remaining processors

* Fixed transform processors creation

* ImageProcessor class refactored

* Renamed some parameters

* Convolution processors refactored

* Refactored filters and dithering processors

* Refactored normalization processors

* Overlays processors refactored

* Renamed some parameters

* CloningImageProcessor class refactored

* Transforms processors refactored

* Updated DefaultImageProcessingContext class

* Src builds, tests still require updating.

* Fix tests

* Removed unnecessary local variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants