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

Pose Graph rewritten without Ceres #2892

Merged
merged 51 commits into from
Apr 1, 2021
Merged

Conversation

savuor
Copy link
Contributor

@savuor savuor commented Mar 14, 2021

Pose graph from Large KinFu rewritten using manual analytic differentiation and custom LevMarq solver based on naive block sparse matrix implementation solved by Eigen library.

Connected test data PR: opencv/opencv_extra#869

Pull Request Readiness Checklist

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

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

@savuor savuor marked this pull request as ready for review March 21, 2021 23:42
@savuor
Copy link
Contributor Author

savuor commented Mar 23, 2021

@alalek Is there a good way to avoid unreachable code warnings? I disabled them with pragma for cases when there's no Eigen enabled.

#include <ceres/ceres.h>
// a very stupid workaround against unreachable code warning
#if defined(_MSC_VER)
#pragma warning(disable : 4702)
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to put PoseGraph::optimize() under #ifdef HAVE_EIGEN condition if Eigen is mandatory here.

BTW, Accurate condition should have && !defined(HAVE_EIGEN) too.

@savuor savuor self-assigned this Mar 24, 2021
@savuor
Copy link
Contributor Author

savuor commented Mar 24, 2021

👍

@savuor
Copy link
Contributor Author

savuor commented Mar 31, 2021

@alalek
I added test for pose graph + testing data in opencv/opencv_extra#869, so we don't have to keep old Ceres implementation along with the current one.
Everything but CN builders works fine. Looks like sometimes CN builders fail to fetch test data from the corresponding branch from opencv_extra.
How this can be solved?

Comment on lines 112 to 115
if (verbose)
{
cv::utils::logging::setLogLevel(cv::utils::logging::LogLevel::LOG_LEVEL_INFO);
}
Copy link
Member

Choose a reason for hiding this comment

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

Please do not touch logging level in tests.
It may be configured externally, e.g. OPENCV_LOG_LEVEL=INFO environment (so even verbose parameter is not needed and should be removed)

// Turn if off if you don't need log messages
static const bool verbose = true;

#if !defined(_DEBUG) && defined(HAVE_EIGEN)
Copy link
Member

Choose a reason for hiding this comment

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

Use this for test filtering:

  • Debug
applyTestTag(
    CV_TEST_TAG_LONG,  // 15sec in release
    CV_TEST_TAG_DEBUG_VERYLONG  // 400 sec in debug
)
  • Eigen
#ifdef HAVE_EIGEN
    ...
#else
    throw SkipTestException("Build with Eigen is required");
#endif

Comment on lines 126 to 127
if (writeToObjFile)
{
Copy link
Member

Choose a reason for hiding this comment

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

Use if (cvtest::debugLevel > 0) instead (to emit extra files).

See opencv/opencv#18955 for details.

@@ -0,0 +1,58 @@
#ifndef OPENCV_RGBD_GRAPH_NODE_H
Copy link
Member

Choose a reason for hiding this comment

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

License header.

std::string filename = cvtest::TS::ptr()->get_data_path() + "rgbd/sphere_bignoise_vertex3.g2o";
Ptr<kinfu::detail::PoseGraph> pg = readG2OFile(filename);

// You may change logging level to view detailed optimization report
Copy link
Member

Choose a reason for hiding this comment

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

Move #ifdef HAVE_EIGEN here to remove (void)(&readG2OFile);

Copy link
Member

@alalek alalek left a 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 👍

@alalek alalek merged commit 1341c05 into opencv:master Apr 1, 2021
@alalek alalek mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants