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

Updates to vicalib #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Updates to vicalib #56

wants to merge 2 commits into from

Conversation

dmirota
Copy link

@dmirota dmirota commented Apr 1, 2020

A collection of changes including improvements in grid detection, build automation, and cross-platform compilation.

@crheckman
Copy link
Contributor

@mcguire-steve: could you please test this in linux? I'll test in Mac.

@dmirota thank you for your PR!

@mcguire-steve
Copy link
Collaborator

Initial review / summary of things that I had to do to get to a point where I could build:
CMakeLists:
Changing case really does matter here - as well as the CONFIG directive.
I had to add OpenCV4 support here
Adding HAL_INCLUDE_DIRS to PROJ_INCLUDE_DIRS
Pangolin is incorrectly added to LINK_LIBS, not PROJ_INCLUDE_DIRS
With HAL_EXPORT, exporting via the local CMake project cache no longer works and requires something like +add_definitions(-DHAL_EXPORT=) to build on Linux

include/vicalib/gl-line-strip.h:
Pangolin is not built against the features/new_pangolin branch (pangolin/gl.h become pangolin/gl/gl.h)

vicalib-engine.cc:
CV_BGR2GRAY becomes cv::COLOR_BGR2GRAY

vicalib-task.cc:
Same thing with Pangolin

Lastly, I think there might have been changes to Calibu as well:
/home/smcguire/test/vicalib/src/vicalib-task.cc: In member function ‘void visual_inertial_calibration::VicalibTask::AddImageMeasurements(const std::vector&)’:
/home/smcguire/test/vicalib/src/vicalib-task.cc:263:76: error: no matching function for call to ‘calibu::ConicFinder::Find(gnu_cxx::alloc_traits<std::allocatorcalibu::ImageProcessing, calibu::ImageProcessing>::value_type&, std::shared_ptr<calibu::CameraInterface >&)’
263 | conic_finder
[ii].Find(image_processing
[ii], input_cameras
[ii].camera);
| ^
In file included from /home/smcguire/test/vicalib/include/vicalib/vicalib-task.h:28,
from /home/smcguire/test/vicalib/src/vicalib-task.cc:1:
/home/smcguire/research/Calibu/include/calibu/conics/ConicFinder.h:55:10: note: candidate: ‘void calibu::ConicFinder::Find(const calibu::ImageProcessing&)’
55 | void Find(const ImageProcessing& imgs);
| ^~~~
/home/smcguire/research/Calibu/include/calibu/conics/ConicFinder.h:55:10: note: candidate expects 1 argument, 2 provided
/home/smcguire/test/vicalib/src/vicalib-task.cc:320:49: error: too many arguments to function ‘std::vector calibu::PosePnPRansac(std::shared_ptr<calibu::CameraInterface >, const std::vector<Eigen::Matrix<double, 2, 1>, Eigen::aligned_allocator<Eigen::Matrix<double, 2, 1> > >&, const std::vector<Eigen::Matrix<double, 3, 1>, Eigen::aligned_allocator<Eigen::Matrix<double, 3, 1> > >&, const std::vector&, int, float, Sophus::SE3d*)’
320 | &t_cw[ii], calib_frame_ == -1);
| ^
In file included from /home/smcguire/test/vicalib/src/vicalib-task.cc:8:
/home/smcguire/research/Calibu/include/calibu/pose/Pnp.h:30:22: note: declared here
30 | std::vector PosePnPRansac(

Is this built against Calibu master?

@crheckman
Copy link
Contributor

crheckman commented Apr 26, 2020 via email

@dmirota
Copy link
Author

dmirota commented Apr 28, 2020

The linux build is work on Travis https://travis-ci.com/github/dmirota/vicalib/jobs/324614396 . I'm still working on Windows and Mac

@dmirota
Copy link
Author

dmirota commented Apr 30, 2020

Ubuntu 16.04 gcc, macOS 14 clang, and Windows Server, version 1809 using Visual Studio 2015 are all building Travis https://travis-ci.com/github/dmirota/vicalib/builds/163036825 . I will be merging the changes into my master.

@schmidtp1
Copy link

What is the status of this PR? @crheckman, @mcguire-steve? thanks!

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.

4 participants