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

Better Edge Detection API #1299

Merged
merged 7 commits into from
Aug 4, 2020
Merged

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

@dalingrin dalingrin 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.

@JimBobSquarePants JimBobSquarePants merged commit 586d99e into master Aug 4, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/better-edge-detection branch August 4, 2020 09:33
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.

3 participants