Skip to content
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

Circle Hough Transform #1186

Merged
merged 28 commits into from
Aug 31, 2023
Merged

Conversation

rolalaro
Copy link

@rolalaro rolalaro commented Jul 3, 2023

Added a ViSP implementation of the Circle Hough Transform.
It permits to have more control on the algorithm parameter than the current OpenCV implementation

tutorial/imgproc/hough-transform/drawingHelpers.h Outdated Show resolved Hide resolved
tutorial/imgproc/hough-transform/drawingHelpers.cpp Outdated Show resolved Hide resolved
tutorial/imgproc/hough-transform/config/detector_img.json Outdated Show resolved Hide resolved
tutorial/imgproc/hough-transform/config/detector_half.json Outdated Show resolved Hide resolved
/**
* \brief Class that defines a 2D circle in an image.
*/
class vpCircle2D
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vpCircle2D class should be moved in module/core/include/visp3/vpCircle2D.h
Since we have for a point in an image the vpImagePoint class, instead of vpCircle2D class name and corresponding files, I think that we should call the class vpImageCircle to be homogeneous with existing code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header of the vpImageCircle.h should have something similar to:

#include <visp3/core/config.h>
#if defined(VISP_HAVE_OPENCV>
#include <opencv2/core.hpp>
#endif

class VISP_EXPORT vpImageCircle

/*!
* Constructor from an OpenCV vector that contains [center_x, center_y, radius].
*/
vpCircle2D(const cv::Vec3f &vec) : m_center(vec[1], vec[0]), m_radius(vec[2]) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructor should be protected

#if defined(VISP_HAVE_OPENCV)
vpImageCircle(const cv::Vec3f &vec) : m_center(vec[1], vec[0]), m_radius(vec[2]) { }
#endif 

There is no need to put #if defined(VISP_HAVE_OPENCV) && defined(HAVE_OPENCV_CORE) since when OpenCV is detected, core module is available.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d144f52

* \brief Class that gather the algorithm parameters.
*/
class CHTransformParameters
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this class doesn't conform ViSP style. Should be something like vpCircleHoughParameters

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 47a8982

@@ -0,0 +1,529 @@
#include <visp3/core/vpImageConvert.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ViSP copyright header is missing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 47a8982

modules/imgproc/src/vpCircleHoughTransform.cpp Outdated Show resolved Hide resolved
rlagneau added 2 commits August 9, 2023 15:55
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage: 7.65% and project coverage change: -0.43% ⚠️

Comparison is base (ad4cbc7) 54.75% compared to head (dc114dd) 54.32%.
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1186      +/-   ##
==========================================
- Coverage   54.75%   54.32%   -0.43%     
==========================================
  Files         401      405       +4     
  Lines       49398    49821     +423     
==========================================
+ Hits        27047    27065      +18     
- Misses      22351    22756     +405     
Files Changed Coverage Δ
modules/core/include/visp3/core/vpDisplay.h 0.00% <0.00%> (ø)
modules/core/include/visp3/core/vpImageCircle.h 0.00% <0.00%> (ø)
modules/core/src/display/vpDisplay_rgba.cpp 5.84% <0.00%> (-0.12%) ⬇️
modules/core/src/display/vpDisplay_uchar.cpp 14.64% <0.00%> (-0.29%) ⬇️
modules/core/src/image/vpImageCircle.cpp 0.00% <0.00%> (ø)
modules/core/src/math/misc/vpMath.cpp 95.68% <ø> (ø)
...roc/include/visp3/imgproc/vpCircleHoughTransform.h 0.00% <0.00%> (ø)
modules/imgproc/src/vpCircleHoughTransform.cpp 0.00% <0.00%> (ø)
modules/tracker/me/src/moving-edges/vpMeNurbs.cpp 0.00% <0.00%> (ø)
modules/core/src/image/vpImageDraw.cpp 67.17% <22.22%> (-1.25%) ⬇️
... and 3 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rlagneau added 3 commits August 30, 2023 11:12
… + computation of the Canny threshold if the user set a negaitve value because it is already performed in the vpImageFilter::canny method
@rolalaro rolalaro changed the title [DRAFT] Circle Hough Transform Circle Hough Transform Aug 30, 2023
@fspindle fspindle merged commit 138939f into lagadic:master Aug 31, 2023
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants