-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add connectedComponents to MagickImage #114
Conversation
Thanks for adding this. I will try to take a look at this on Friday. I am wonder if we should use another image here instead so we have different tests. And you might not need the |
I didn't really understand the different quantum types until I'd already written the tests. That makes sense.
Since
That's fair, will fix.
Don't worry, I'll help! #116 |
They should produce the same results but what we are really testing is ImageMagick so I would like to have a different testcase here so we are testing ImageMagick with two different images instead of using a single image and expect the same result. When it breaks in Magick.NET it will also break here. |
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.
Some more comments.
* @param connectivity The number of neighbors to visit (4 or 8). | ||
* @see {@link https://imagemagick.org/script/connected-components.php} | ||
* | ||
* @example |
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.
Not sure if I want to keep this here because that would also require me to maintain it. A description of the method and parameters and the @see
should be sufficient?
And should we not add this documentation to the interface 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.
Not sure if I want to keep this here because that would also require me to maintain it.
Yeah that's totally fair. Maintenance concerns are always a good reason to be wary of too much "extra" stuff.
I think @example
theoretically shouldn't be much of a maintenance concern though, at least in this case:
- the examples aren't very complex (just showing basic usage and return type)
- the
IMagickImage
api likely won't change drastically
A description of the method and parameters and the @see should be sufficient?
I feel like it's a good first-touch bit of context for users on how to use this in a js-specific setting. Descriptions tell you what it does, @see
gives extra information like the various defines available, but @example
shows how to use it in js specifically.
Just sharing my thoughts though! If it still feels like something you'd prefer to not have to think about, will gladly remove
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 will remove them myself after merging the PR. I think we should put the examples somewhere else.
25db9b7
to
ff87028
Compare
I rotated the existing test image 180º and updated the tests. |
* @param connectivity The number of neighbors to visit (4 or 8). | ||
* @see {@link https://imagemagick.org/script/connected-components.php} | ||
* | ||
* @example |
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 will remove them myself after merging the PR. I think we should put the examples somewhere else.
This PR adds
MagickImage#connectedComponents
. You can learn more from the ImageMagick connected-components docs.New things:
Threshold
for storing a min/max thresholdConnectedComponentsSettings
for storingconnectivity
and connected-components operation defines (and applying them to/removing them from an image with#_setArtifacts
/#_removeArtifacts
)ConnectedComponent
which represents a connected component and is automatically loaded from the componentinstance
Other things:
TestImages.connectedComponents
using the test image fromMagick.Net
toBeWithinRangeDelta
custom matcher to make it easier to expect a number to be within the range of ±deltajsdoc
for improved dx