Export SfM data to MVE format by implementing an MVEUndistorter class. #282
Conversation
|
I will have a look these days. Looks great on first sight though. Thanks. |
| @@ -96,6 +96,10 @@ double Camera::FocalLength() const { | |||
| return params_[idxs[0]]; | |||
| } | |||
|
|
|||
| double Camera::NormalizedFocalLength() const { | |||
There was a problem hiding this comment.
Please add a unit test for this base/camera_test.cc, same for the new methods below.
src/exe/image_undistorter.cc
Outdated
| @@ -62,9 +62,13 @@ int main(int argc, char** argv) { | |||
| undistorter.reset(new CMPMVSUndistorter(undistort_camera_options, | |||
| reconstruction, *options.image_path, | |||
| output_path)); | |||
| } else if (output_type == "MVE") { | |||
| undistorter.reset(new MVEUndistorter(undistort_camera_options, | |||
There was a problem hiding this comment.
Also add this option to the ui/undistortion_widget.cc.
src/base/undistortion.cc
Outdated
| } | ||
|
|
||
| point2D_t MVEUndistorter::ChooseLowestReprojectionErrorPoint( | ||
| image_t image_id, point3D_t point3d_id, |
There was a problem hiding this comment.
Make these arguments const please.
There was a problem hiding this comment.
Fix these please and I will merge. Thanks!
There was a problem hiding this comment.
Thats not the usual c++ style (I follow google coding style guide) to make pass by value args const. I can do it if you insist for codebase consistency reason.
abhishek-carbon3d
force-pushed
the
export2mve
branch
from
276e80f
to
4c9c9f0
Compare
|
Anything else to be taken care of in this pull request? |
|
No, it all looks good. Just haven't had time to test it myself. Can you confirm that this works and the dense reconstruction using MVE works with this exporter? |
|
On the one sample that I tested, MVE failed to register any features, which
makes me think something is wrong in the export of the camera parameters.
…On Tue, Jan 30, 2018 at 11:21 PM, Johannes Schönberger < ***@***.***> wrote:
No, it all looks good. Just haven't had time to test it myself. Can you
confirm that this works and the dense reconstruction using MVE works with
this exporter?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#282 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJNkXhDw1FdSABo5ODBl84ObN3Yff-ugks5tQBRogaJpZM4Roq-P>
.
--
-Abhishek
|
|
Okay, so before merging this, we would have to resolve that issue of course. I am a little busy these days, so it will take some time for me to look into this. Maybe you can investigate a little further and fix the problem?
|
|
Okay. I will dig through some more and report back.
…On Tue, Jan 30, 2018 at 11:35 PM, Johannes Schönberger < ***@***.***> wrote:
Okay, so before merging this, we would have to resolve that issue of
course. I am a little busy these days, so it will take some time for me to
look into this. Maybe you can investigate a little further and fix the
problem?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#282 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJNkXmpm3vHHKZMbnTZvnTv1tH4uF60Kks5tQBfCgaJpZM4Roq-P>
.
--
-Abhishek
|
|
MVE allows now to import Colmap models/workspaces |
This is not quite working yet on my sample data. MVE is rejecting all the exported features because apparently the exported cameras don't see them.
I will appreciate if you can take a look and spot any issues. For reference here is more info about the mve camera model.
https://github.com/simonfuhrmann/mve/wiki/Math-Cookbook
https://github.com/simonfuhrmann/mve/wiki/MVE-File-Format