Bug: OD message "There might be a bug here!" #2
Conversation
src/octoclass.cpp
Outdated
| // not being deleted. If we have only the root node available (which we can | ||
| // capure by checking the depth of the iterator), then we can safely clear | ||
| // the tree, as it is not a use case we need | ||
| const uint depth = it.getDepth(); |
There was a problem hiding this comment.
seems like this could be a function on its own, called every time we do something with the tree. It appears twice in the code.
There was a problem hiding this comment.
are you talking about getting the depth?
It appears twice because it is in two different for loops. I think that a function to retrieve the depth from a leaf iterator would be overkill, as it is a short one-liner that is clear about its goal.
There was a problem hiding this comment.
If you are talking about the whole chunk of code that is repeated, note that one of them clears tree_, and the other clears tree_inflated_.
There was a problem hiding this comment.
I think you could write a function clearIfEmpty or something that took a reference to a tree, checked its depth, and emptied it. It's a small optimization but it is more DRY
src/octoclass.cpp
Outdated
| @@ -520,7 +542,7 @@ void OctoClass::FadeMemory(const double &rate) { // rate at which this function | |||
|
|
|||
| // tree nodes that are unknown | |||
| if (is_occ != tree_inflated_.isNodeOccupied(n)) { // if it was occupied then disoccupied, delete node | |||
| tree_inflated_.deleteNode(key, it.getDepth()); | |||
| tree_inflated_.deleteNode(key, depth); | |||
There was a problem hiding this comment.
I think is better not to change it.getDepth() in this line, since you define depth as a const unint. on the other hand you could define, depth as a pointer to it.getDepth() , instead of making a copy of this iterator.
There was a problem hiding this comment.
Sorry, I don't follow your comment... what is the problem of having depth as a const uint?
If you are worried about complexity, uint should take less RAM than a pointer, as uint has 4 bytes, while a pointer has 8 bytes (in a 64bit machine).
There was a problem hiding this comment.
it's a trivial matter, you declare depth variable in line 527 and it's not changing anymore. but technically it.getDepth() iterator can check the most recent root nodes.
There was a problem hiding this comment.
not really... the iterator can only point to a different node after if moves on in the for loop through the command ++it...
There was a problem hiding this comment.
is this thread-safe? If not then potentially it could change?
There was a problem hiding this comment.
This is the only place where FadeMemory is called:
mutexes_.octomap.lock();
if (globals_.octomap.memory_time_ > 0) {
globals_.octomap.FadeMemory(fading_memory_update_rate_);
}
mutexes_.octomap.unlock();
So yeah, it is thread-safe
src/octoclass.cpp
Outdated
| @@ -520,7 +542,7 @@ void OctoClass::FadeMemory(const double &rate) { // rate at which this function | |||
|
|
|||
| // tree nodes that are unknown | |||
| if (is_occ != tree_inflated_.isNodeOccupied(n)) { // if it was occupied then disoccupied, delete node | |||
| tree_inflated_.deleteNode(key, it.getDepth()); | |||
| tree_inflated_.deleteNode(key, depth); | |||
There was a problem hiding this comment.
is this thread-safe? If not then potentially it could change?
src/octoclass.cpp
Outdated
| // not being deleted. If we have only the root node available (which we can | ||
| // capure by checking the depth of the iterator), then we can safely clear | ||
| // the tree, as it is not a use case we need | ||
| const uint depth = it.getDepth(); |
There was a problem hiding this comment.
I think you could write a function clearIfEmpty or something that took a reference to a tree, checked its depth, and emptied it. It's a small optimization but it is more DRY
|
I fully rewrote the solution to this issue. |
closes Pensasystems/pensa-ros#740
This is a workaround for what seems to be a bug in octomap. I have filed an issue in the octomap repository: OctoMap/octomap#307
I tested the current solution and it works in simulation.
We can reproduce the error by killing the lidar node, which I plan to test on our drone.
Test card: https://docs.google.com/spreadsheets/d/11nEIxDr3Xem8T7m9ldJpqJAyNbu205QsmVvI4Ka0qGE/edit#gid=486985852