-
Notifications
You must be signed in to change notification settings - Fork 17
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
Polygon filling API - Boundary Fill #57
Polygon filling API - Boundary Fill #57
Conversation
Add RectanglePoints (JuliaImages#52)
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.
Don't rush, be patient, do less copy. Be careful about your format. Try to read, analyze and find out what the real issue is before asking for help.
I'm asking this because I hope I can spend more time reviewing and discussing more PR-related issues instead of those trivial things that you could have fixed by yourself.
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.
The test failure on Julia 1.0 should be fixed by #59 It's because pkg resolving in old Julia versions are not smart enough in some cases. (You can run git rebase master
to update your branch)
Mostly it looks good to me now. Hopefully, this is the last round review.
If you're not confident about the implementation, you can always add more tests to make sure of it.
…geDraw.jl into boundary-fill
@ashwani-rathee This PR a little bit long and old. Would you mind summarizing a little bit so that we can move forward quicker? |
@johnnychen94 In the latest improvements( 2 days back, before that were git rebase master changes), 12 tests were added and methods for cartesian index, point were added and bounds check was added for vertices and seed point. |
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.
If I don't miss anything from previous comments, this should be the last round of review. Once it's fixed/answered, it should be good to merge.
You're starting to write good tests, that's good news 🎉 Do keep in mind: all reliable functionality is supported by more lines of codes of test cases.
@@ -56,11 +56,10 @@ function draw!( | |||
) where N | |||
|
|||
# Boundscheck for verts | |||
@boundscheck a = map(x -> (checkbounds(Bool, img, x[1], x[2]) ? nothing : throw(DomainError(x, "Vertice $x outside the image array domain"))), verts) | |||
|
|||
@boundscheck map(x -> (checkbounds(Bool, img, x[1], x[2]) ? nothing : throw(DomainError(x, "Vertice $x outside the image array domain"))), verts) |
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.
Oh boy, you can also change this to
@boundscheck map(x -> (checkbounds(Bool, img, x[1], x[2]) ? nothing : throw(DomainError(x, "Vertice $x outside the image array domain"))), verts) | |
@boundscheck foreach(x->checkbounds(img, x), verts) |
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.
Yeah, I could have done that but that doesn't give information(in a concise way) to user what to correct 😕
Current expected error looks far more informative in my understanding I think.
Expected: DomainError(CartesianIndex(0, 6), "Vertice CartesianIndex(0, 6) outside the image array domain")
Thrown: BoundsError(ColorTypes.RGB[RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0); RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0); RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0); RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0); RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0); RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0); RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0) RGB{N0f8}(0.0,0.0,0.0)], (CartesianIndex(0, 6),))
Stacktrace:
Initial setup of the Polygon filling API
Linked issue :Polygon/Any closed path filling algorithms #53
Demonstration : https://github.com/ashwani-rathee/Demonstrations/blob/main/boundary-fill-api.jl
For some reason,the tests in poygon2d.jl isn't able to access boundaryfilll ,I am not sure on why as of now