victorprad / InfiniTAM

A Framework for the Volumetric Integration of Depth Images
http://www.infinitam.org
Other
918 stars 351 forks source link

Matrix multiplication with scalar not working #32

Closed yousefamar closed 8 years ago

yousefamar commented 8 years ago

I'm not sure if I'm missing something obvious, but while writing unit tests I came across strange results and tracked it down to something that can be recreated like this:

ORUtils::Matrix3<double> m;

std::cout << m << std::endl;
// Prints random unitialized values, as expected

m.setIdentity();
std::cout << m << std::endl;
// 1, 0, 0
// 0, 1, 0
// 0, 0, 1
// as expected

std::cout << (m * 5.0) << std::endl;
// Prints random unitialized values, not expected

m *= 5;
std::cout << m << std::endl;
// 5, 0, 0
// 0, 5, 0
// 0, 0, 5
// as expected

The place that the * is overloaded is here, but that looks fine to me. l'd really appreciate another set of eyes on that.

I ran the above on a completely clean build of the master branch, though I also compiled with -DWITH_CUDA=FALSE but I don't think that's relevant.

sgolodetz commented 8 years ago

@Paraknight: It's definitely not fine -- it returns a reference to a local variable :-)

_CPU_AND_GPU_CODE_ inline Matrix3& operator *(const T &r) const {
    Matrix3 res(this->m);
    return res *= r;
}

Once the local variable goes out of scope, you get a dangling reference, hence the observed behaviour.

sgolodetz commented 8 years ago

I've fixed it in the infinitam_v3 branch on Victor's private fork.

@olafkaehler: Should we cherry-pick it across to master in the public fork?

olafkaehler commented 8 years ago

That would be sensible. Go ahead!

yousefamar commented 8 years ago

Oh! I completely overlooked that. Thanks!

sgolodetz commented 8 years ago

Fixed and merged