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

Expose optional minAmpl parameter of blur operation #4172

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

marcosc90
Copy link
Contributor

following #4168, this PR adds minAmpl option to .blur(), completing the exposure of all options of vips_gaussblur().

@lovell
Copy link
Owner

lovell commented Jul 23, 2024

Thank you for the PR Marcos.

I usually prefer for the parameter names in sharp to match libvips for consistency as you've done here but I feel "ampl" might be better expanded in the external API to "amplitude" to remove any ambiguity, perhaps as minAmplitude.

It might also be good to include the libvips description of this property in the JSDocs, something like "A smaller value will generate a larger, more accurate mask".

https://github.com/libvips/libvips/blob/81b6b783f09fd7acd0e0ede1a28fef906ce810e9/libvips/convolution/gaussblur.c#L186

@marcosc90
Copy link
Contributor Author

I usually prefer for the parameter names in sharp to match libvips for consistency as you've done here but I feel "ampl" might be better expanded in the external API to "amplitude" to remove any ambiguity, perhaps as minAmplitude.

I had the same thought, I preferred minAmplitude but went with minAmpl to mimic libvips. I'll push the changes in a bit.

@marcosc90
Copy link
Contributor Author

@lovell I left internal API with minAmpl, let me know if you want this changed as well.

@lovell lovell merged commit 735fee7 into lovell:main Jul 23, 2024
15 checks passed
@lovell
Copy link
Owner

lovell commented Jul 23, 2024

Thank you very much!

@lovell lovell added this to the v0.33.5 milestone Jul 23, 2024
@marcosc90 marcosc90 deleted the feat-min-ampl branch July 23, 2024 10:33
lovell added a commit that referenced this pull request Jul 23, 2024
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