opencv_contrib icon indicating copy to clipboard operation
opencv_contrib copied to clipboard

new feature: update ellipse detector using projective invariant pruning

Open haku-huang opened this issue 3 years ago • 5 comments

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • [x] I agree to contribute to the project under Apache 2 License.
  • [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • [x] The PR is proposed to the proper branch
  • [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name.
  • [x] The feature is well documented and sample code can be built with the project CMake

haku-huang avatar Aug 09 '22 07:08 haku-huang

@sturkmen72 Hello, we have passed the test and the code has been modified according to your instructions.

haku-huang avatar Aug 10 '22 00:08 haku-huang

@MisakiCoca i am a common contributor. a developers team member will review.

consider my additional remarks as follows :

  1. ellipseDetector() seems a class name. maybe something like findEllipses will be better.
  2. you can see the preview of the documentation https://pullrequest.opencv.org/buildbot/export/pr_contrib/3322/docs/d1/d05/group__ximgproc__ellipse__detector.html#ga4f98330bb312dd8f42046105e646b390

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

  1. I did not test your code but what is your opinion about

yours : EllipseDetectorTest.ManySmallEllipses (101787 ms) and existing ellipse finding EdgeDrawing : test ximgproc_ED.ManySmallCircles (875 ms)

sturkmen72 avatar Aug 10 '22 09:08 sturkmen72

@MisakiCoca i am a common contributor. a developers team member will review.

consider my additional remarks as follows :

  1. ellipseDetector() seems a class name. maybe something like findEllipses will be better.
  2. you can see the preview of the documentation https://pullrequest.opencv.org/buildbot/export/pr_contrib/3322/docs/d1/d05/group__ximgproc__ellipse__detector.html#ga4f98330bb312dd8f42046105e646b390

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

  1. I did not test your code but what is your opinion about

yours : EllipseDetectorTest.ManySmallEllipses (101787 ms) and existing ellipse finding EdgeDrawing : test ximgproc_ED.ManySmallCircles (875 ms)

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. Thus when there are too many arcs, our speed will be slower than algorithms such as Hough, who are based on point sampling. In such situation, however, our method will have higher accuracy.

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. Figure.1 Where theRHT and Qi et al., denotes the Random Hough Transform and ours, respectively.

Compared to some other arc-based approach, our computational speed has been improved significantly while retaining accuracy.

Figure.2 Where the Jia denotes our method. Pre, Rec are abbreviations of precision and recall.

We have removed the ManySmallEllipses from tests, we would be honored if these descriptions could answer your questions.

haku-huang avatar Aug 10 '22 11:08 haku-huang

@asmorkalov Thank you very much for your attention! What can i do for the macOS-ARM64 failure?

haku-huang avatar Aug 12 '22 09:08 haku-huang

It looks like CI issue or system configuration issue on our side. Please ignore it for now.

asmorkalov avatar Aug 12 '22 11:08 asmorkalov

@asmorkalov Hello, is there anything I can do to aid in the merging of this PR?

haku-huang avatar Oct 11 '22 12:10 haku-huang

@asmorkalov Hello, I have completed all the modifications you suggested.

haku-huang avatar Oct 19 '22 14:10 haku-huang

@asmorkalov Sorry, there were some issues with the previous testing session, but they have already been resolved.

haku-huang avatar Oct 20 '22 07:10 haku-huang

@asmorkalov The warning from clang in macOS caused by C.21 have been solved.

haku-huang avatar Oct 20 '22 10:10 haku-huang

@asmorkalov All of the CI/CD checks have passed, thank you for your continuous help. What else can I do for you?

haku-huang avatar Oct 20 '22 13:10 haku-huang

@asmorkalov Thank you so much for your continuous help!

haku-huang avatar Oct 24 '22 14:10 haku-huang