Closed Morgane55440 closed 3 months ago
i have completed the documentation for all the user-facing functions, just have the testing left
i have a question regarding what happens when a mask intersected with the image is empty. there are 2 case which can cause this :
but in my opinion it isn't necessary, and some users might want a mask that doesn't include the center.
so we would need to define what happens when the intersection is empty.
currently grayscale_erode
sets the pixel to 0, and grayscale_dilate
to 255.
i could add that to the doc, or change the behavior.
different behaviors could be :
after thinking about it, i realized the answer to "what happens when the intersection of a mask with the image is empty" is obvious.
(u8, min) and (u8, max) are both groups, therefore min({}) and max({}) are well defined, and they are the neutral elements of min and max respectively.
therefore grayscale_erode
should set such a pixel to 255
, and grayscale_dilate
to 0
.
this also might be useful for optimization later on to be used as default values before the mask is computed.
i'm going to fix this and add tests concerning this aspect
i sometime struggle between returning a result and panicking.
Do you feel it would be reasonable to make a try_from_image
function as well that returns an Option<Mask>
?
other than that, i feel like, after it is fully reviewed, it would make sense to push this PR as it is, and make another on the subject of optimisation. what's your opinion ?
Why do you need a Mask in a public API?
So It's actually an Image<LumaA<i16>>
with helper methods.
If the mask were a LumaA Image, the user would have more freedom and we would have less code I think
i understand the idea, but the Mask is not just an Image<LumaA<i16>>
with helper methods.
it also requires a center (as the Mask::new
methods shows).
giving the users a Mask
allows to bundle the 'image' and the 'center' in one struct
i will try to think more on a Maskless implementation though
but the reasons i made the struct at the start are also because (1) it felt reasonable to have a distinct struct because mask aren't the same as images, and more importantly (2) that way we could have control on how the mask is laid out in memory, for optimization later on.
the first argument is very subjective i will admit, but the second one feel like it makes sense to me.
to be clear, from my (admittedly limited) experience working with mathematical morphology, it should be very rare for users to make masks from images, and almost unthinkable to make masks bigger than 20 by 20. so when the user writes
let eroded = grayscale_erode(&image, &Mask::diamond(3));
it doesn't really matter to them what kind of struct Mask is.
what is affected user-side from the existence or not of the Mask
struct is twofold :
pub fn grayscale_dilate(image: &GrayImage, mask: &Mask) -> GrayImage
feels cleaner to me than
pub fn grayscale_dilate(image: &GrayImage, mask: &GrayImage) -> GrayImage
but that's subjective and not a big deal.
Mask
would do allow you to use images of any size (512 pixels+), but it would prevent you from being able to choose the center yourself, forcing you to pad your image with empty pixels on some sides to move it around. so there are clearly pros and cons.dev-side, removing the struct would remove the from_image
and apply
functions, but the 3 mask makers would still be needed, and we would need to do preprocessing on the "mask" image inputs.
i think that's roughly a good view of the subject. i will admit i'm probably a bit biased toward wanting to keep the struct, but ultimately i'd understand if you feel it's best to remove it.
3 masks makers can be functions that return an alpha image, for example
Maybe you can keep struct
if you think that custom masks are rare. Also struct
protects some invariants
that is what i meant by "the 3 mask makers would still be needed if we removed the struct" the functions would still be there, just not in an impl block
Maybe you can keep
struct
if you think that custom masks are rare. Alsostruct
protects some invariants
that is definitely part of my thinking
would it be good to merge as-is ? it is functional as far as i can tell, and i've done quite a few tests. i think it'd be best to work on optimisation/internal mask representation in a new PR
The docs need a bit of work but that can happen before the next release of this crate.
The behaviour looks correct.
Thanks!
this PR has been made following the issue #576 : support grayscale morphology operators
this PR seeks to add new function to the
morphology
module that complement the current ones.the current function only support
binary morphology
, and the new functions will add support forgrayscale morphology
.to do so, i have added a new struct
Mask
, that can representations masks of arbitrary shape.at original time of making this PR, i have 1 commit, that implements the
Mask
struct and all the functions i originally thought of making. on the other hand, i have written very little documentation, and no tests, which is what i'm going to be working on next. i would really appreciate any feedback on the code i've written so far