Skip to content

Conversation

@JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Aug 1, 2020

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

As discussed in #1297 It's not currently possible to implement your own custom edge detection kernels.

This goes against the nature of the API and as such I deem it necessary to refactor the API to allow extensibility. This is a breaking change.

The API has been refactored to expose three structs representing three types of edge detection kernels.

  • EdgeDetectorKernel Contains a single 2D XY kernel.
  • EdgeDetector2DKernel Contains two separable 1D kernels.
  • EdgeDetectorCompassKernel Contains eight kernels representing each point on the compass.

A new static class KnownEdgeDetectorKernels replaces the current enum EdgeDetectionOperators.

Individual edge detection processors SobelProcessor etc have been replaced with versions representing each of the three kernel types.

The resulting code is much simpler and more powerful allowing users to implement any series of gradient operators they require.

@JimBobSquarePants JimBobSquarePants added API breaking Signifies a binary breaking change. labels Aug 1, 2020
@JimBobSquarePants JimBobSquarePants added this to the 1.0.0 milestone Aug 1, 2020
@JimBobSquarePants JimBobSquarePants requested a review from a team August 1, 2020 21:04
@JimBobSquarePants JimBobSquarePants requested a review from a team August 1, 2020 21:41
@codecov
Copy link

codecov bot commented Aug 1, 2020

Codecov Report

Merging #1299 into master will increase coverage by 0.01%.
The diff coverage is 97.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
+ Coverage   82.70%   82.71%   +0.01%     
==========================================
  Files         697      692       -5     
  Lines       30677    30718      +41     
  Branches     3470     3474       +4     
==========================================
+ Hits        25370    25409      +39     
+ Misses       4604     4603       -1     
- Partials      703      706       +3     
Flag Coverage Δ
#unittests 82.71% <97.53%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nvolution/Kernels/Implementation/KayyaliKernels.cs 100.00% <ø> (ø)
...onvolution/Kernels/Implementation/KirschKernels.cs 100.00% <ø> (ø)
...n/Kernels/Implementation/LaplacianKernelFactory.cs 100.00% <ø> (ø)
...olution/Kernels/Implementation/LaplacianKernels.cs 100.00% <ø> (ø)
...nvolution/Kernels/Implementation/PrewittKernels.cs 100.00% <ø> (ø)
...tion/Kernels/Implementation/RobertsCrossKernels.cs 100.00% <ø> (ø)
...volution/Kernels/Implementation/RobinsonKernels.cs 100.00% <ø> (ø)
...onvolution/Kernels/Implementation/ScharrKernels.cs 100.00% <ø> (ø)
...Convolution/Kernels/Implementation/SobelKernels.cs 100.00% <ø> (ø)
...onvolution/EdgeDetectorCompassProcessor{TPixel}.cs 95.23% <85.71%> (-0.32%) ⬇️
... and 17 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 f4f689c...f717bcf. Read the comment docs.

Copy link

@ehardesty ehardesty left a comment

Choose a reason for hiding this comment

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

I tested this for the scenario brought up in discussion #1297

This new API makes it extremely easy to do custom edge processing.

@JimBobSquarePants
Copy link
Member Author

Perfect @dalingrin I’m glad I was able to improve matters.

@JimBobSquarePants
Copy link
Member Author

@SixLabors/core I'm going to merge this unless anyone objects. I'm very happy with it.

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

Labels

API breaking Signifies a binary breaking change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants