Navigation Menu

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

Apple M1 changes #788

Closed
wants to merge 1 commit into from
Closed

Apple M1 changes #788

wants to merge 1 commit into from

Conversation

pierotofy
Copy link
Contributor

Hey all ✋

While building and running OpenSfM on an Apple M1 I encountered a strange bug, which I narrowed down to to geometry/triangulation.h:71. Debugging a reconstruction showed that for some reason the compiler decided to assume std::abs should be of type int instead of type T, causing values to be truncated to 0 at every computation.

@facebook-github-bot
Copy link
Contributor

@fabianschenk has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@@ -68,7 +68,11 @@ std::pair<bool, Eigen::Matrix<T, 3, 1>> TriangulateTwoBearingsMidpointSolve(

const T eps = T(1e-30);
const T det = A.determinant();
#ifdef __aarch64__
if (std::abs<T>(det) < eps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pierotofy
Thanks for your diff. Could you remove the #ifdef and simply use the templated version as default:

if (std::abs<T>(det) < eps)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @fabianschenk ✋ thanks for the review, I actually started out by just having the templated version, but Visual Studio doesn't like it (read: it doesn't compile) on Windows.

I could replace the define to a #ifndef __MSVC__ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @pierotofy ,
Thanks for the quick response and also for testing on Windows.
Then I'll land it like it is.

Thanks again for your contribution 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pleasure! OpenSfM is a great software.

@facebook-github-bot
Copy link
Contributor

@fabianschenk merged this pull request in d468471.

@pierotofy pierotofy mentioned this pull request Sep 8, 2021
facebook-github-bot pushed a commit that referenced this pull request Sep 15, 2021
Summary:
Closes #766 (which is also needed to compile on Apple M1 along with #788).

Pull Request resolved: #791

Reviewed By: paulinus

Differential Revision: D30900431

Pulled By: YanNoun

fbshipit-source-id: 8f2508d010d4ba93f59ca6d4d9ad6b635905739b
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

3 participants