Skip to content

Conversation

@blackball
Copy link

@blackball blackball commented Sep 29, 2017

Fixed the segmentation fault issue when the tracking is lost (got a invalid pose).
Fixed the operators (==, !=) codes for Matrix.
Added a HasNaN() function for matrix types.

@sgolodetz sgolodetz self-assigned this Sep 30, 2017
minimizeLM(*this, currentPara);
}

const auto paraM = currentPara.GetM();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please can you replace auto with the actual type? We don't want to force an unnecessary C++11 dependency.

Copy link
Author

Choose a reason for hiding this comment

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

I can do it. But according to the newest cmakefile (cmake/Flags.cmake), C++11 is already enabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's only enabled by default on Linux - we build with it there, but it's not a compulsory dependency. The change proposed will break the build on other platforms.


M_d = trackingState->pose_d->GetM(); M_d.inv(invM_d);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please can you get rid of the whitespace changes?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I can do it.


_CPU_AND_GPU_CODE_ inline friend bool operator == (const Matrix4 &lhs, const Matrix4 &rhs) {
bool r = lhs[0] == rhs[0];
bool r = lhs.m[0] == rhs.m[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were these changes motivated by your compiler having trouble with anonymous structs?

Copy link
Author

Choose a reason for hiding this comment

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

This is not about anonymous struct. The Matrix class simply doesn't provide a subscript operator. I think this part of codes were not tested before.

ITMRenderState_VH *renderState_vh = (ITMRenderState_VH*)renderState;

M_d = trackingState->pose_d->GetM();
if (HasNaN(M_d)) return ; // TODO: means the pose is invalid, need to fix that pose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where was the segmentation fault occurring out of interest? Is it reproducible? This change here is only in the CPU version of the reconstruction engine - is there a corresponding one needed in the CUDA version?

Copy link
Author

Choose a reason for hiding this comment

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

You can simply set one of M_d's element to be std::nan to see if it will happen on your machine. It's certainly reproducible on my machine. I can not test a fix on GPU now.

T m[s*s];
};

template<class T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please can you fix the indentation of this?

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

@blackball
Copy link
Author

@sgolodetz I fixed the indentions. It was caused by mixing space and tabs. The codebase is using TAB instead of spaces.

Also I remove auto.

For the modification in GPU side, please refer the CPU implementation.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants