-
Notifications
You must be signed in to change notification settings - Fork 107
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
Split Filter Contours #627
base: master
Are you sure you want to change the base?
Split Filter Contours #627
Conversation
…ion for calculating the ratio.
Current coverage is 57.85% (diff: 44.57%)@@ master #627 diff @@
==========================================
Files 196 197 +1
Lines 6165 6238 +73
Methods 0 0
Messages 0 0
Branches 561 566 +5
==========================================
+ Hits 3573 3609 +36
- Misses 2427 2464 +37
Partials 165 165
|
Nice. Though might it be a good a idea to use best-fit bounding boxes instead of the simple x-y-oriented bounding boxes? Take a look at this image. The current bounding box we use to calculate the ratio will be the blue rectangle, which has a very different ratio than the object (the oblong rectangle) |
Best fit bounding boxes are useful but may not be what all users want. There could be an option for it but that might complicate the step. |
Yeah, it would be hard to change the existing stuff because it's already in use. I'm reluctant to add more stuff to the filter operation because there's just so much crap packed into it already and it makes the UI a mess. @JLLeitschuh what are your thoughts? |
Uhhhhh... An |
We could just make a separate advanced filter contours and simple filter contours operation. The simple would just have area and perimeter, while the advanced could have everything. |
Bytedeco RotatedRect is broken so we can't actually do the best fit bounding boxes. |
How are they broken? I got them working fine when I was playing around with them. |
They don't give a list of points for the vertices and the bounding rectangle wasn't even remotely correct so I couldn't use that either. |
Edit: take a look here |
final opencv_core.RotatedRect bb = minAreaRect(contour); | ||
opencv_core.Point2f points = new opencv_core.Point2f(4); | ||
bb.points(points); | ||
final double rotatedWidth = Math.sqrt( Math.pow(points.position(0).x() - points.position(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.
Get rid of the space in Math.sqrt( Math.pow...
. Should also use a sensical scheme of line breaks, right now it's hard to keep track of what's going on. I'd put the Math.pow
statements each on their own line.
…into FilterContoursRatio
This seems like a breaking change. Old |
The integer division problem was fixed by #618. The rest of this PR will split up filter contours to make it easier to use. |
@osulacam Is this PR ready for review? |
@AustinShalit Yes |
I'll take a look at this tonight |
Is it still a breaking change or has that been resolved? |
Because this technically creates a two new operations to get rid of an old one it has to be a breaking change. It is still a necessary change that shouldn't break anything significantly. |
Adds best fit bounding boxes and splits filter contours into two operations.