-
-
Notifications
You must be signed in to change notification settings - Fork 924
Feature: Add median filter option to blur node #3196
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
base: master
Are you sure you want to change the base?
Feature: Add median filter option to blur node #3196
Conversation
- Add median parameter to blur function for noise reduction - Implement median_filter_algorithm with efficient quickselect - Support gamma space calculations for median filtering - Preserve edges while removing noise, complementing existing blur options Feature in Issue: GraphiteEditor#912
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Keavon the code looks fine, but I have not checked if the functionality behaves as you would expect
fn median_quickselect(values: &mut [f32]) -> f32 { | ||
let mid: usize = values.len() / 2; | ||
// nth_unstable is like quickselect: average O(n) | ||
*values.select_nth_unstable_by(mid, |a, b| a.partial_cmp(b).unwrap()).1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still new to rust and this repo, so I might be wrong here. could unwrap()
potentially panic in some cases? If that happens the function wouldn't return f32
. Also, what happens if values
is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They would only panic if NAN values are compared, which should never happen since those are not valid colors. But it will panic on empty Images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it's good to check for that condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, what I meant to say is it will panic if the function is called with an empty range, but for an empty image it won't because the function will never be called so this is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about replacing partial_cmp(...).unwrap()
with f32::total_cmp
to avoid panics on NaN values?
Also, I think for even-length slices the current logic picks only the upper median (len/2
).
If accuracy matters, the median should be computed as the average of the two middle values instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work. Regarding the median averaging, I don't know what the expected result would be (pinging @Keavon) the disadvantage of averaging is that we can now create brightness values which never appear in the artwork. I don't know if that is important for anything. Computing the second value should be relatively cheap though, which is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it works fine. I tested it before making a PR, but I'm not sure about the performance, and maybe I didn't recognize something else. We should wait until @Keavon makes a last review of the code.
This PR adds median filtering capability to the existing blur node, providing users with a third filtering option alongside Gaussian and box blur.
Changes Made
median
boolean parameter to the blur nodemedian_filter_algorithm()
function that processes each color channel independentlymedian_quickselect()
helper function usingselect_nth_unstable_by()
for O(n) median calculationUse Case
Median filtering is particularly useful for:
Technical Notes
The implementation follows the same pattern as existing blur algorithms with proper gamma space handling and associated alpha processing.
Closes feature in Issue No. #912