-
Notifications
You must be signed in to change notification settings - Fork 5.8k
new feature: update ellipse detector using projective invariant pruning #3322
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
Conversation
@sturkmen72 Hello, we have passed the test and the code has been modified according to your instructions. |
@MisakiCoca i am a common contributor. a developers team member will review. consider my additional remarks as follows :
just for a function I think no need to add a section in modules part https://pullrequest.opencv.org/buildbot/export/pr_contrib/3322/docs/df/d2d/group__ximgproc.html
yours : |
Thank you very much for your reply. We have completed the revisions for the first and second points based on your advice. Regarding the third point about our running speed: Our approach can be roughly summarized as enumerating the arcs in each quadrant and applying a pruning strategy. In thress realistic datasets with moderate amount of ellipses, the advantages of both the computational speed and the accuracy of our method are very obvious, as shown in the following Figure. Compared to some other arc-based approach, our computational speed has been improved significantly while retaining accuracy.
We have removed the |
@asmorkalov Thank you very much for your attention! What can i do for the macOS-ARM64 failure? |
It looks like CI issue or system configuration issue on our side. Please ignore it for now. |
@asmorkalov Hello, is there anything I can do to aid in the merging of this PR? |
@asmorkalov Hello, I have completed all the modifications you suggested. |
@asmorkalov Sorry, there were some issues with the previous testing session, but they have already been resolved. |
@asmorkalov The warning from clang in macOS caused by C.21 have been solved. |
@asmorkalov All of the CI/CD checks have passed, thank you for your continuous help. What else can I do for you? |
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.
Looks good to me in general besides srand
.
@asmorkalov Thank you so much for your continuous help! |
namespace opencv_test { namespace { | ||
|
||
|
||
TEST(FindEllipsesTest, EllipsesOnly) |
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.
In debug build mode there is catched out of boundary access issue:
[ RUN ] FindEllipsesTest.EllipsesOnly
unknown file: Failure
C++ exception with description "OpenCV(4.6.0-dev) /build/precommit-contrib_linux64_no_opt/4.x/opencv/modules/core/include/opencv2/core/mat.inl.hpp:1603: error: (-215:Assertion failed) 0 <= y && y < size.p[0] in function 'operator[]'
" thrown in the test body.
[ FAILED ] FindEllipsesTest.EllipsesOnly (18 ms)
@misakicoca Could have a chance to look on this?
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.
Ok, I have reproduced this issue locally so far and I will fix this bug in the following days.
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.
@alalek I have found the source of the error and found a way to fix it, how should I submit the code change?
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.
Thank you for looking on this!
Please open a new PR with the fix.
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.