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

Convolution: Border wrapping modes #2060

Merged
merged 8 commits into from
Apr 25, 2022

Conversation

ynse01
Copy link
Contributor

@ynse01 ynse01 commented Mar 13, 2022

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 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

I went through the somewhat older issues and found #427 to be of my interest to implement.

This PR creates three modes in which to treat the border pixels in a Convolution operation. These modes are:

  1. Clamp (the current implementation)
  2. Wrap
  3. Mirror

I don't like the None name proposal in the original issue, as I feel you have to make a choice about your borders. And with the name Clamp we make clear what operation we perform.

Please advise on how to expose this in the public Convolution API's.
For example, please specify your preference on questions like:

  • On which class to expose the BorderWrappingMode property ?
  • Specify the modes of X and Y directions separately? Or just a single property for both directions?
  • Switch the default from Clamp to Mirror ?
  • Add this functionality to Resize also ?

Other feedback or suggestion are also welcome !

Sorry, something went wrong.

@JimBobSquarePants
Copy link
Member

Oh wow! This is a pleasant surprise! I never looked at this after refactoring convolution so thought the changes would still be really viral.

@JimBobSquarePants JimBobSquarePants modified the milestones: Future, 3.0.0 Mar 27, 2022
@ynse01
Copy link
Contributor Author

ynse01 commented Mar 30, 2022

Please let me know if there is anything missing in this PR.

@JimBobSquarePants
Copy link
Member

Will do. We’re currently investigating some access violation issues #2075 that we need to resolve before we merge any more code.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

I think we can get this merged now. Thanks for your patience @ynse01 this is an awesome addition to the library!

@JimBobSquarePants JimBobSquarePants merged commit 6061063 into SixLabors:main Apr 25, 2022
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.

None yet

3 participants