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

grayscale morphology operators support #581

Merged

Conversation

Morgane55440
Copy link
Contributor

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

src/morphology.rs Outdated Show resolved Hide resolved
src/morphology.rs Outdated Show resolved Hide resolved
@Morgane55440
Copy link
Contributor Author

i have completed the documentation for all the user-facing functions, just have the testing left

@Morgane55440
Copy link
Contributor Author

i have a question regarding what happens when a mask intersected with the image is empty.
there are 2 case which can cause this :

  • Empty Mask
  • non-empty mask that does not include the center, on an edge/corner
    we can of course disalow empty masks, although i don't think it's necessary, but that wouldn't remove this case.
    what would remove this case would be forcing the center to always be part of the mask.

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.

@Morgane55440
Copy link
Contributor Author

different behaviors could be :

  • panic : a bit extreme, but that's an extreme case
  • return the original pixel value : that would have the benefit of not introducing new value, but that would make empty masks equivalent to a single pixel mask that contains it's center, which might be considered a downside

@Morgane55440
Copy link
Contributor Author

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

src/morphology.rs Outdated Show resolved Hide resolved
src/morphology.rs Outdated Show resolved Hide resolved
src/morphology.rs Outdated Show resolved Hide resolved
@Morgane55440
Copy link
Contributor Author

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 ?

@cospectrum
Copy link
Contributor

Why do you need a Mask in a public API?

@cospectrum
Copy link
Contributor

So It's actually an Image<LumaA<i16>> with helper methods.

@cospectrum
Copy link
Contributor

If the mask were a LumaA Image, the user would have more freedom and we would have less code I think

@Morgane55440
Copy link
Contributor Author

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

@Morgane55440
Copy link
Contributor Author

i will try to think more on a Maskless implementation though

@Morgane55440
Copy link
Contributor Author

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 :

  • the function signature :
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.

  • what happens when users actually want a custom mask : as i mentionned before, that's probably a very rare case. that being said, using an image instead of a 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.

@cospectrum
Copy link
Contributor

cospectrum commented Apr 30, 2024

3 masks makers can be functions that return an alpha image, for example

@cospectrum
Copy link
Contributor

Maybe you can keep struct if you think that custom masks are rare. Also struct protects some invariants

@Morgane55440
Copy link
Contributor Author

Morgane55440 commented Apr 30, 2024

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

src/morphology.rs Outdated Show resolved Hide resolved
src/morphology.rs Outdated Show resolved Hide resolved
src/morphology.rs Show resolved Hide resolved
@Morgane55440
Copy link
Contributor Author

Morgane55440 commented Apr 30, 2024

Maybe you can keep struct if you think that custom masks are rare. Also struct protects some invariants

that is definitely part of my thinking

@Morgane55440
Copy link
Contributor Author

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

@theotherphil
Copy link
Contributor

The docs need a bit of work but that can happen before the next release of this crate.

The behaviour looks correct.

Thanks!

@theotherphil theotherphil merged commit 6a08bd8 into image-rs:master May 5, 2024
15 checks passed
@Morgane55440 Morgane55440 deleted the greyscale_morphology_operators branch May 8, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants