danini / magsac

The MAGSAC algorithm for robust model fitting without using an inlier-outlier threshold
Other
415 stars 65 forks source link

Cmake problems with CREATE_SAMPLE_PROJECT var, C++ #11

Closed amy-tabb closed 4 years ago

amy-tabb commented 4 years ago

Hello Daniel,

I saw your talk at EECVC and I tried to clone your C++ MAGSAC project and get it to run on Ubuntu 18.04. I was unable to do so w/o some alterations, some notes:

1- it is not obvious from the README that one has to clone and then copy the src folder from your graph-cut-ransac repo to the folder in magsac/graph-cut-ransac (which is empty when cloning magsac). By reading through the CMakeLists.txt file I figured it out and was able to configure/generate with Cmake and then build.

2- I set the CREATE_SAMPLE_PROJECT var as follows set(CREATE_SAMPLE_PROJECT ON)` and Cmake generation failed:

CMake Error at CMakeLists.txt:105 (target_link_libraries):
  Target "MAGSAC" of type EXECUTABLE may not be linked into another target.
  One may link only to INTERFACE, OBJECT, STATIC or SHARED libraries, or to
  executables with the ENABLE_EXPORTS property set.

I changed the target_link_libraries() section in CREATE_SAMPLE_PROJECT and it generated, built, and I can run it now:

target_link_libraries(SampleProject 
        ${OpenCV_LIBS}
        Eigen3::Eigen
    )

A question, I see that you are linking with OpenMP, but I didn't see anywhere in the paper where you mention parallelization or implementation details (but I only did a high-level read). Is the OpenMP for speeding up the OpenCV parts of the code, or do you use parallelization currently in the MAGSAC++ algo?

danini commented 4 years ago

Hm, I have tested it on Ubuntu and it compiled with no problem.

  1. The copying is done automatically if you clone the repo recursively. I will make this clearer in the README file.
  2. I think this problem is connected to the previous one. Everything should compile with no problem when cloned recursively. Could you please try again with that flag and let me know if there still is some problem?

Parallelization: MAGSAC++, no. The original MAGSAC algorithm uses it (it needs a flag when instantiating the object). E.g. MAGSAC<cv::Mat, DefaultEssentialMatrixEstimator> magsac_original(MAGSAC<cv::Mat, DefaultEssentialMatrixEstimator>::MAGSAC_ORIGINAL); vs. MAGSAC<cv::Mat, DefaultEssentialMatrixEstimator> magsac_plus_plus(MAGSAC<cv::Mat, DefaultEssentialMatrixEstimator>::MAGSAC_PLUS_PLUS);

amy-tabb commented 4 years ago
  1. ok, yes that worked. I have been using git for a while but don't use the recursive flag. So I didn't know 'clone or download this repository and, also, the sub-modules' -> git clone <url> --recursive.
  2. I still get the Cmake error (using cmake version 3.14.0 on Ubuntu 18.4) when enabling CREATE_SAMPLE_PROJECT. As follows:
CMake Error at CMakeLists.txt:104 (target_link_libraries):
  Target "MAGSAC" of type EXECUTABLE may not be linked into another target.
  One may link only to INTERFACE, OBJECT, STATIC or SHARED libraries, or to
  executables with the ENABLE_EXPORTS property set.

-- Configuring incomplete, errors occurred!

If I change the CMakeLists.txt using my fix from above, I can generate and make.

Thanks for the response, and the answer about OpenMP.

salahkhan94 commented 4 years ago

You could compile magsac as a library as opposed to an executable then link it with the sample project.

Make this change in the CMakeLists.

add_library(${PROJECT_NAME}
    ${HDRS_MAGSAC}
    ${SRCS_MAGSAC}
)
amy-tabb commented 4 years ago

Ahh, yes of course @salahkhan94 that did it. It now compiles + links and I get an .a file for libMAGSAC. Strange that @danini could also get it to compile with the current listing as an executable.