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

InterpMotion: initialize reference_p to be zero. #510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xinyazhang
Copy link
Contributor

@xinyazhang xinyazhang commented Nov 19, 2020

The coefficients of Eigen::Vector3d, like other data types in Eigen3,
are uninitialized by default.

This can be verified with the following code

#include <Eigen/Core>
#include <iostream>

int main() {
    {
        Eigen::Vector3d v;
        v << 1, 2, 3;
        std::cout << v(0) << " " << v(1) << " " << v(2) << std::endl;
    }
    {
        Eigen::Vector3d v;
        std::cout << v(0) << " " << v(1) << " " << v(2) << std::endl;
    }
    return 0;
}

This change is Reviewable

The coefficients of Eigen::Vector3d, like other data types in Eigen3,
are uninitialized by default.
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Thanks for this fix. A few requests:

  • Please describe the problem this fixes. What happens if reference_p is uninitialized?
  • The problem exists in FCL because of the lack of unit tests in the original implementation. We have been following a policy of adding missing tests as we improve the code. Please add a unit test that would have failed with reference_p uninitialized and now succeeds because of your fix. That will prevent future programmers from breaking this code ever again.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @xinyazhang)

@xinyazhang
Copy link
Contributor Author

Thanks for this fix. A few requests:

* Please describe the problem this fixes. What happens if reference_p is uninitialized?

* The problem exists in FCL because of the lack of unit tests in the original implementation. We have been following a policy of adding missing tests as we improve the code. Please add a unit test that would have failed with reference_p uninitialized and now succeeds because of your fix. That will prevent future programmers from breaking this code ever again.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @xinyazhang)

Got it, will add unit tests later.

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