Use shared_ptr/weak_ptr for keyframe and landmark

[ymd-stella]

Thank you for your work!

I implemented a version that uses shared_ptr/weak_ptr for keyframe and landmark.

There seems to be almost no performance degradation that was mentioned below.
(ref: #9 (comment))

[shinsumicco]

@ymd-stella
Thanks! Please give me some time to review it.

At a glance, the following modifications are needed:

• rebasing your PR branch to the latest develop branch in order to enable CI for clang-format at wercker.
• using auto where it can be applicable because type names wrapped by smart pointers are longer than before. (Please don't use auto for Eigen types.)

[shinsumicco]

@ymd-stella
Thank you for fixing.
I think the review will need much time because there is a lot of fixes.
Please give me some time to check it.

[shinsumicco]

@ymd-stella
Thanks for updating.
We're sorry that we cannot start the review quickly due to other tasks.
We'll review this PR after it. Thank you for your patience.

[stolpa4]

@ymd-stella Great job! Thank you. It seems to me that you have not fully resolved circular references issue. The landmark still has "observations" map with pointers to key_frames. It seems to me that most of keyframes will leak that way

[ymd-stella]

@stolpa4 You're right. Thanks. I'll fix it.

[ymd-stella]

openvslam/src/openvslam/data/landmark.cc

Lines 277 to 292 in 6de5fec

	void landmark::prepare_for_erasing() {
	std::map<keyframe*, unsigned int> observations;
	{
	std::lock_guard<std::mutex> lock1(mtx_observations_);
	std::lock_guard<std::mutex> lock2(mtx_position_);
	observations = observations_;
	observations_.clear();
	will_be_erased_ = true;
	}
	for (const auto& keyfrm_and_idx : observations) {
	keyfrm_and_idx.first->erase_landmark_with_index(keyfrm_and_idx.second);
	}
	map_db_->erase_landmark(this);
	}

The observations_ is cleared before the landmark is deleted, so it seems to work fine.

[stolpa4]

@ymd-stella
Consider the use case: you want to use the system in a long-term application (say, for your car / bike pose tracking). The pure localization use case is not interesting for you as your trajectory presumably will not have any loops (or the potential loop is too far away and there is a little chance that the system will not lose track until you get there). So you decide to make your application reset the system after the track is lost (to switch from localization mode to SLAM mode again). Then, after the system is reset, will those keyframes still occupy the memory (despite the fact, that map database was cleared)? I would say yes, because there is circular reference between keyframe and mappoint. Anyway, can you verify this with a keyframe destructor, for example?

As for prepare_for_erasing() function, it is called while discarding bad landmarks. I don't think it is going to be called on the system reset.

In a similar system, I've broke this dependency by changing the vector of shared pointers to landmarks in the keyframe class to vector of weak pointers. I've leveraged some kind of converter from weak pointer to shared and vice versa, something similar to:

template<typename T>
std::vector<std::shared_ptr<T>> to_shared(const std::vector<std::weak_ptr<T>> &elems)
{
    std::vector<std::shared_ptr<T>> res(elems.size());
    std::transform(elems.begin(), elems.end(), res.begin(),
                   std::bind(&std::weak_ptr<T>::lock, std::placeholders::_1));
    return res;
}


template<typename T>
std::vector<std::weak_ptr<T>> to_weak(const std::vector<std::shared_ptr<T>> &elems)
{
    return {elems.begin(), elems.end()};
}

This can lead to a minor performance degradation, but in my tests it is not sufficient and I consider it as a good price for the safe memory usage.

[ymd-stella]

I didn't care for the reset.

[stolpa4]

@ymd-stella No problems. It is not a question to you, but rather it is a must-have for the project, as it can be used in such long-term application, but makes it really hard because of the memory management issue.