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

Fourier descriptors #1216

Merged
merged 2 commits into from Oct 31, 2017

Conversation

@LaurentBerger
Copy link
Contributor

commented Jun 6, 2017

Fourier descriptors for optimal curve matching


#ifndef __OPENCV_FOURIERDESCRIPTORS_HPP__
#define __OPENCV_FOURIERDESCRIPTORS_HPP__
#ifdef __cplusplus

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jun 8, 2017

Contributor

I think we don't need such protections.

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jun 8, 2017

Author Contributor

Ok delete

@param rotOrigScale Standard vector angle origin and scale.
@param dist distance between src and dst after matching.
*/
ContourFitting(int ctr=1024,int fd=16):ctrSize(ctr),fdSize(fd){};

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jun 8, 2017

Contributor

Declaration of estimateTransformation should be right here, after the doxygen description.

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jun 8, 2017

Author Contributor

As you see doc is not ready at all!

//! @{

/**
* @brief Fourier descriptors for planed closed curves

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jun 8, 2017

Contributor

It seems like this documentation is taken from an other file.

phiMin = arg(a[1] / b[1]);
do
{
std::cout << alphaMin << "\t" << phiMin << "\t" << sMin << "\n";

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jun 8, 2017

Contributor

Algorithm in OpenCV shouldn't print anything by default.

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jun 8, 2017

Author Contributor

Of course : lines deleted

@param dist distance between src and dst after matching.
*/
ContourFitting(int ctr=1024,int fd=16):ctrSize(ctr),fdSize(fd){};
Mat estimateTransformation(InputArray src, InputArray dst, double *dist=0,bool fdContour=false);

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jun 8, 2017

Contributor

Why do you use Mat as return type here? Usually result is returned via OutputArray.

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jun 8, 2017

Author Contributor

OK I change it

@LaurentBerger

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

PR is not ready

@LaurentBerger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2017

PR is finished you can review it. I think I have got some problem with rebase

@sovrasov

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2017

It seems that rebase was failed because of c366da7. It contains
very strange changes and rebase is going mad.

namespace cv {
namespace ximgproc {

//! @addtogroup shape

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 7, 2017

Contributor

Use ximgproc_shape here and I don't see surrounding brackets for this group:

//! @addtogroup ximgproc_shape
//! @{

//group content

//! @} ximgproc_shape
*/
int getFDSize() { return fdSize; };
};
//! @addtogroup ximgproc_filters

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 7, 2017

Contributor

Why do you need this group here? I think all the code should be inside of ximgproc_shape.

@@ -0,0 +1,133 @@
/*

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 7, 2017

Contributor

Please use the new short license header for all new files:

// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.
@sa fourierDescriptor
*/
CV_EXPORTS void fourierDescriptor(InputArray _src, OutputArray _dst, int nbElt=-1,int nbFD=-1);
CV_EXPORTS void transform(InputArray _src, Mat t,OutputArray _dst, bool fdContour=true);

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 7, 2017

Contributor

Why does this function not documented? And please use Input or Output array instead of Mat t here.
You can also wrap these functions into python replacing CV_EXPORTS with CV_EXPORTS_W

//! @addtogroup shape
//! @{

/** @brief Abstract base class for ContourFitting algorithms.

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 7, 2017

Contributor

The class is not abstract and even public. So OpenCV users can create an instance of it. Did you suppose this usecase? If not, the class should be moved into a private header or .cpp.

@sovrasov

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2017

I forgot to say: ximgproc_shape group should be declared here

* @param nbElt number of rows in _dst or getOptimalDFTSize rows if nbElt=-1
* @param nbFD number of FD return in _dst _dst = [FD(1...nbFD/2) FD(nbFD/2-nbElt+1...:nbElt)]
*
* @example fourier_descriptors_demo.cpp

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 7, 2017

Contributor

You can get rid of these example commands and leave just one mention of fourier_descriptors_demo.cpp somewhere.

@LaurentBerger LaurentBerger force-pushed the LaurentBerger:FourierDescriptors branch from 5e662e8 to 1b78f38 Jul 10, 2017
*/
CV_EXPORTS_W void contourSampling(InputArray _src, OutputArray _out, int nbElt);

CV_EXPORTS_W Ptr<ContourFitting> createContourFitting(int ctr = 1024, int fd = 16);

This comment has been minimized.

Copy link
@sovrasov

sovrasov Jul 10, 2017

Contributor

Use create here instead of createContourFitting. ContourFitting::createContourFitting() looks redundant in user code.

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jul 10, 2017

Author Contributor

OK

@sovrasov

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2017

👍

@LaurentBerger LaurentBerger force-pushed the LaurentBerger:FourierDescriptors branch 2 times, most recently from 1c32ec4 to aebe216 Jul 31, 2017
@LaurentBerger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

a git reset -HARD and rewrite file and a git rebase : only one commit now

namespace ximgproc {

//! @addtogroup ximgproc_shape
//! @{

This comment has been minimized.

Copy link
@alalek

alalek Jul 31, 2017

Contributor

Please don't use indentation in namespaces.

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jul 31, 2017

Author Contributor

OK

cout << "******************** PRESS G TO MATCH CURVES *************\n";
do
{
code = waitKey(30)&0xFF;

This comment has been minimized.

Copy link
@alalek

alalek Jul 31, 2017

Contributor

&0xFF is not needed anymore for cv::waitKey

namespace cv {
namespace ximgproc {

//! @addtogroup ximgproc_shape

This comment has been minimized.

Copy link
@alalek

alalek Jul 31, 2017

Contributor

ximgproc_shape => ximgproc_fourier

This comment has been minimized.

Copy link
@LaurentBerger

LaurentBerger Jul 31, 2017

Author Contributor

OK

@param src Contour defining first shape.
@param dst Contour defining second shape (Target).
@param _alphaPhiST : \f$ \alpha \f$=_alphaPhiST(0,0), \f$ \phi \f$=_alphaPhiST(0,1) (in radian), s=_alphaPhiST(0,2), Tx=_alphaPhiST(0,3), Ty=_alphaPhiST(0,4) rotation center

This comment has been minimized.

Copy link
@alalek

alalek Jul 31, 2017

Contributor

Please eliminate underscores (_) from parameters names (in .hpp file).

@@ -0,0 +1,190 @@
// This file is part of OpenCV project.

This comment has been minimized.

Copy link
@alalek

alalek Jul 31, 2017

Contributor

We don't add license headers for "samples". Please remove it.

@sovrasov

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2017

@LaurentBerger could you please resolve the merge conflict?

@LaurentBerger

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

@sovrasov oops! may be something is wrong. I made pull upstream master in fourierdescriptors branch

@LaurentBerger LaurentBerger force-pushed the LaurentBerger:FourierDescriptors branch from 5d55d2b to d9062ae Oct 21, 2017
@sovrasov

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2017

You actually could do

git fetch upstream
git rebase upstream/master
<open an editor and resolve merge conflicts>
git add <edited files>
git rebase --continue

to avoid merge commit.

@LaurentBerger LaurentBerger force-pushed the LaurentBerger:FourierDescriptors branch from c75eb4b to d9062ae Oct 23, 2017
@LaurentBerger LaurentBerger force-pushed the LaurentBerger:FourierDescriptors branch from d9062ae to 8b733e0 Oct 23, 2017
@sovrasov

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2017

👍

@opencv-pushbot opencv-pushbot merged commit 8b733e0 into opencv:master Oct 31, 2017
1 check passed
1 check passed
default Required builds passed
Details
@LaurentBerger LaurentBerger deleted the LaurentBerger:FourierDescriptors branch Oct 31, 2017
std::vector<Point> c;
for (int j = 0; j<static_cast<int>(z.size()); j++)
c.push_back(Point2d(z[j].x, z[j].y));
Mat(z).copyTo(_dst);

This comment has been minimized.

Copy link
@alalek

alalek Nov 22, 2017

Contributor

Look like "c" is not used here.

static_cast(z.size())

Static code analyses dislike this (out of range check is expected) - consider to use size_t iterator type.

@LaurentBerger LaurentBerger referenced this pull request Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.