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

Ensure we don't lose precision when exporting to txt #2010

Merged
merged 10 commits into from Feb 13, 2022

Conversation

melhashash
Copy link
Contributor

Set precision when exporting to COLMAP text files to avoid precision loss. (mostly when the scene is in global coordinates)

@pmoulon
Copy link
Member

pmoulon commented Feb 11, 2022

Thank you for this contribution.
Could you add a variable in the binary int floating_point_precision_digit = 16; and use it as a parameter for each function.
This way our user community could chose whatever value they would like to have easily.

@pmoulon pmoulon self-assigned this Feb 11, 2022
@pmoulon pmoulon added this to the 2.1 milestone Feb 11, 2022
@melhashash
Copy link
Contributor Author

melhashash commented Feb 11, 2022

Sure, a global variable is OK or do you mean as an argument?

@pmoulon
Copy link
Member

pmoulon commented Feb 12, 2022

I would prefer as argument

@melhashash
Copy link
Contributor Author

@pmoulon updated by adding an argument option.
If not set, the default is 16.
If set < 6 we set to 6 which is the default value used by std for precision if not set at all.

@pmoulon
Copy link
Member

pmoulon commented Feb 12, 2022

LGTM, I think your code editor is using tabs, and not 2spaces as 'tab'

@@ -36,9 +36,13 @@ using namespace openMVG::features;

bool CreateLineCameraFile( const IndexT camera_id,
std::shared_ptr<openMVG::cameras::IntrinsicBase> intrinsic,
std::string & camera_linie)
std::string & camera_linie,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind add a commit to fix this typo camera_linie -> camera_line
Thanks ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, spent half of the day adjusting indentation :)


<< "[-o|--outdir] path where cameras.txt, images.txt and points3D.txt will be saved"
<< "\n[Optional]\n"
<< "[-p|--precision] sets the decimal precision to be used to format floating-point values (default = 16)";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally you would display the value of the variable here floating_point_precision_digit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@pmoulon
Copy link
Member

pmoulon commented Feb 12, 2022

Running Continous Integration build and unit test. But it LGTM
Thank you for your contribution and time to adjust the tabs ;-)

Copy link
Contributor Author

@melhashash melhashash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Showing the value of floating_point_precision_digit instead of hard coding it.


<< "[-o|--outdir] path where cameras.txt, images.txt and points3D.txt will be saved"
<< "\n[Optional]\n"
<< "[-p|--precision] sets the decimal precision to be used to format floating-point values (default = 16)";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@melhashash
Copy link
Contributor Author

Running Continous Integration build and unit test. But it LGTM Thank you for your contribution and time to adjust the tabs ;-)

Sure, anytime! You are the one to thank for this awesome amount of work!

@pmoulon pmoulon merged commit f4161ef into openMVG:develop Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants