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.

Sign up for GitHub

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

Jump to bottom

Fix ceres-solver deprecations #1249

Merged
merged 9 commits into from
Oct 7, 2022
Merged

Fix ceres-solver deprecations #1249

merged 9 commits into from
Oct 7, 2022

Conversation

p12tic
Copy link
Contributor

@p12tic p12tic commented Sep 30, 2022

ceres-solver deprecated LocalParameterization in favor of Manifold in v2.1. v2.2 will not contain LocalParameterization anymore. This PR bumps ceres-solver dependency to at least v2.1 which has been released on 2022-03-28.

This PR also fixes an ODR violation that was caused by two local classes named alicevision::sfm::IntrinsicsManifold present in two different translation units.

@servantftechnicolor
Copy link
Contributor

This PR will conflict with the branch https://github.com/alicevision/AliceVision/tree/dev/mergeCalibration

@p12tic
Copy link
Contributor Author

p12tic commented Oct 1, 2022 •
edited
Loading

@servantftechnicolor I can rebase the mergeCalibration branch and solve the conflicts.

@p12tic
Copy link
Contributor Author

p12tic commented Oct 1, 2022

This PR is much smaller change so it makes sense to get it in first and then larger, more risky changes on top.

@p12tic p12tic mentioned this pull request Oct 1, 2022
Closed
@p12tic p12tic force-pushed the ceres-deprecations branch 2 times, most recently from c1a5b72 to f0f821c Compare October 1, 2022 22:31
@p12tic
Copy link
Contributor Author

p12tic commented Oct 1, 2022

I've updated the PR to work with both new and old ceres. It required some compatibility wrappers, but at least it was possible to avoid preprocessor macro soup to support both APIs.

@p12tic p12tic force-pushed the ceres-deprecations branch 4 times, most recently from 79ef714 to 9a68856 Compare October 2, 2022 10:44
@fabiencastan fabiencastan added this to the 2.5.0 milestone Oct 6, 2022
p12tic added 9 commits October 7, 2022 02:19
@p12tic
[utils] Add compatibility Manifold wrapper for Ceres 2.0 and older
b5e6b94
@p12tic
[sfm] Convert SO3::LocalParameterization to ceres::Manifold
7b90c90 
This upgrades to ceres::Manifold API which replaces
ceres::LocalParameterization API in ceres v2.1.
ceres::LocalParameterization will be removed in ceres v2.2
@p12tic
[sfm] Convert IntrinsicsParameterization to ceres::Manifold
1e13778 
This upgrades to ceres::Manifold API which replaces
ceres::LocalParameterization API in ceres v2.1.
ceres::LocalParameterization will be removed in ceres v2.2
@p12tic
[sfm] Convert symbolic IntrinsicsParameterization to ceres::Manifold
e313d68 
This upgrades to ceres::Manifold API which replaces
ceres::LocalParameterization API in ceres v2.1.
ceres::LocalParameterization will be removed in ceres v2.2
@p12tic
[sfm] Convert SE3::LocalParameterization to ceres::Manifold
438ef1e 
This upgrades to ceres::Manifold API which replaces
ceres::LocalParameterization API in ceres v2.1.
ceres::LocalParameterization will be removed in ceres v2.2
@p12tic
[sfm] Convert SO2::LocalParameterization to ceres::Manifold
0806822 
This upgrades to ceres::Manifold API which replaces
ceres::LocalParameterization API in ceres v2.1.
ceres::LocalParameterization will be removed in ceres v2.2
@p12tic
[sfm] Replace uses of SubsetParameterization with SubsetManifold
6f78efb
@p12tic
[calibration] Replace uses of SubsetParameterization with SubsetManifold
3a9393d
@p12tic
[sfm] Rename IntrinsicsManifold to avoid ODR violation
24fe94b 
There is another IntrinsicsManifold class in BundleAdjustmentCeres.cpp
file. Having both classes is undefined behavior and may break at any
time.

One avenue of breakage is when both classes expose inline member
functions. These functions will have weak linkage, which means that if
the functions are not inlined, then one of the functions will be lost as
the final executable will contain only one function with specific
mangled name. The calls to the lost function will call the other
function which will break in various ways.
@p12tic p12tic force-pushed the ceres-deprecations branch from 9a68856 to 24fe94b Compare October 6, 2022 23:19
@fabiencastan fabiencastan merged commit 0780cd6 into alicevision:develop Oct 7, 2022
@fabiencastan
Copy link
Member

@p12tic Could you propose a rebase of mergeCalibration?

@p12tic p12tic deleted the ceres-deprecations branch October 7, 2022 12:32
@p12tic
Copy link
Contributor Author

p12tic commented Oct 7, 2022

@fabiencastan merge-calibration branch has been rebased to https://github.com/p12tic/AliceVision/tree/dev/mergeCalibration-rebased.

Original branch did not compile. I've fixed simple errors, e.g. missing <memory> include, but for example original code removed bool estimate(std::shared_ptr<camera::Pinhole> & ... ...) overload without updating tests, so code fails to compile. By commenting out 10 or so lines of offending code the rebased branch compiles fine.

The PR change can be inspected by running:

git diff -w ca64bd7de26..dev/mergeCalibration > original.diff
git diff -w d3ad668d85a..dev/mergeCalibration-rebased > rebased.diff
diff -u original.diff rebased.diff > changes.diff

and then inspecting changes.diff file.

There was a change to rename IntrinsicsParameterization to IntrinsicsSymbolicParameterization to avoid ODR violation that is solved by #1249 PR. Since this is already solved in develop branch, I have kept the current name.

@p12tic
Copy link
Contributor Author

p12tic commented Oct 7, 2022

Also, it's worth noting that dev/mergeCalibration uses boost.json, which means we will need to upgrade boost dependency to 1.75 at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabiencastan fabiencastan fabiencastan approved these changes

@servantftechnicolor servantftechnicolor Awaiting requested review from servantftechnicolor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@p12tic @servantftechnicolor @fabiencastan