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

[sfmTransform] rewrite auto_from_cameras #1376

Merged
merged 2 commits into from
May 22, 2023
Merged

Conversation

servantftechnicolor
Copy link
Contributor

@servantftechnicolor servantftechnicolor commented Mar 13, 2023

SfmTransform have multiple options to "transform" the sfm cameras poses and associated point cloud.

"Auto_from_cameras" :

  • translates the sfm such that the mean of the cameras positions is (0,0,0).
  • scale the sfm such that the variance of the cameras positions is 1
  • Rotate the sfm such that :
  1. A plane is fitted to the cameras positions
  2. The scene is rotated such that the plane normal become n=(1,0,0).

Previous code was not giving the correct results for the rotation part (random) and I was not able to understand it as it had no geometric meaning to me. This code is a full rewrite of the rotation estimation part.

@simogasp
Copy link
Member

Could you please elaborate better in the description of the PR on what was not working in the previous version and the improvement you added with this PR?

@fabiencastan fabiencastan added this to the 3.1.0 milestone Apr 12, 2023
src/aliceVision/sfm/utils/alignment.cpp Outdated Show resolved Hide resolved
src/aliceVision/sfm/utils/alignment.cpp Outdated Show resolved Hide resolved
@mugulmd
Copy link
Contributor

mugulmd commented May 12, 2023

Could you please elaborate better in the description of the PR on what was not working in the previous version and the improvement you added with this PR?

Did some comparison tests on multiple scenes, the results are definitely in favor of the new method:

  • the new method never performs worst than the old one (sometimes the scene is upside-down, but when it is the case then it's upside-down for both methods)
  • the new method solves orientation issues on most scene where the old method failed (usually upside-down problems).

Also since the new method comes with a clear and concise paper explaining it (and proving how it works mathematically) I tend to prefer it.

@cbentejac cbentejac self-requested a review May 22, 2023 09:17
@mugulmd mugulmd dismissed simogasp’s stale review May 22, 2023 15:18

changes addressed

@mugulmd mugulmd merged commit 379df2e into develop May 22, 2023
@mugulmd mugulmd deleted the dev/auto_from_cameras branch May 22, 2023 15:19
@simogasp
Copy link
Member

Did some comparison tests on multiple scenes, the results are definitely in favor of the new method:

  • the new method never performs worst than the old one (sometimes the scene is upside-down, but when it is the case then it's upside-down for both methods)
  • the new method solves orientation issues on most scene where the old method failed (usually upside-down problems).

Also since the new method comes with a clear and concise paper explaining it (and proving how it works mathematically) I tend to prefer it.

Great.
It would be nice to make a dataset of all the cases you collected (just the sfmdata) and keep it somewhere, ideally for making functional/unit tests. So that in the future we can replicate the results and possibly compare again the results if a new method/heuristic is proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants