Blur using sigma and document as radius#2848
Blur using sigma and document as radius#2848RunDevelopment wants to merge 2 commits intoimage-rs:mainfrom
Conversation
|
I'd prefer to keep name "radius" as it's actually much more common for casual or other platforms users. I believe anyone who actually wants to investigate will understand that "radius" means "sigma" in any case, and both groups will get the same result.
Probably, yes. Last time we changed the implementation, we broke it for some users, and now we're going to do so again silently. There isn't any obvious solution, but it might be nice if we break the API in a way that at least forces them to take a look on it. It’s probably another matter, but we could change the type to f64 just to achieve that. This is rather remainder to think, or for another PR if you get sign off for this one. |
|
I really don't know if it's a good idea. Having a constructor that is defined via a kernel size, or 3-sigma, still makes more sense to me than saying I'd rather deprecate the float-parameter version entirely or make it an alias to |
|
I don't super like the idea of only I see the argument for silent breakage, but I'm only comfortable making this change because the next version is 1.0. Breaking changes are to be expected there, and a change that brings
Sounds like you're proposing If we go with this, then I think we should at least
Speaking of other APIs, what are other libraries doing? So I did a little survey. I asked Gemini to give me a list of popular image processing libraries in different programming languages. Probably not the best way of doing this, but I simply don't know many such libraries across languages. Survey detailsSo here's the list and what they are doing for (Gaussian) blurring:
Lastly, I also want to include ImageMagick. Its In summary:
Overall, the trend is clearly. Sigma is the most common way of specifying the parameters of a Gaussian blur. |
|
I’m not arguing that sigma is a common way. I just want to point out that this is a bit less dense than what Gemini states.
|
|
On the 0.25.x series we use sigma as the argument for all the blur methods. We previously merged an API breaking change to instead using radius for the 1.0 series. But based on all the discussion so far here, it isn't sounding like switching would be worth the disruption? |
|
I mostly want to make sure the main API is easy and obvious to use. If everyone agrees that using sigma with proper documentation is fine for that purpose, then I don't see a problem. Both are fine with me. I personally think radius might be a bit more intuitive, though sigma is also fine. |
|
Is there even API breakage? As @fintelia said, 0.25.x uses sigma. Even 0.25.10 still does. This PR basically reverts the breaking change (=switching to radius using |
|
|
|
Ah, no, it seems it’s actually still sigma. Then, there’s probably no API breakage. I actually thought radius was published. |
|
Yep, just tested it and 0.25.10 uses sigma and produces the exact same output as |
Resolves #2842
Changes:
f32parameterblurandDynamicImage::bluris now the standard deviation of the Gaussian blur, aka. sigma. The parameter is calledsigmato be consistent with other functions. The documentation of all blurring-related functionality has been changed to describe thesigmaparameter as the "radius of the blur". This is heavily inspired by the MDN docs forblur.blur_advanced(GaussianBlurParameters::new_from_sigma(x))have been replaced withblur(x).Since the motivation for not using sigma was UX, I hope that more documentation solves the same issue without needing a separate new definition of "radius." I think the new documentation should adequately convey how to use
sigmaparameters even if users don't know what a standard deviation or normal distribution is.@awxkee Do you think this is enough, or should I rename all relevant
sigmaparameters toradiusas well?Also, should I remove
GaussianBlurParameters::new_from_radius? It's unused now. If we go with parameter renaming, I think it should definitely be removed, because it's going to be confusing otherwise.