search

introlab / rtabmap

RTAB-Map library and standalone application
https://introlab.github.io/rtabmap
Other
2.6k stars 761 forks source link

Graph Reduction: visual words not referenced by any nodes in WM or LTM are still saved to database #1270

Open matlabbe opened 2 months ago

matlabbe commented 2 months ago

Thanks for the database. I can reproduce the issue.

rtabmap-reprocess --Mem/ReduceGraph true --Rtabmap/MemoryThr 50  ~/Downloads/240415-164828.db output.db

I think I know what is going on. Here is a sequence that would reproduce the issue.

  1. At some point, Node 31 is created with a new word 9950
  2. Later, Node 65 is created with descriptor matching 9950 and loop with Node 4, so Node 65 will be removed when it reaches WM as Mem/ReduceGraph=true in Mem/STMSize (assuming 10 in this example) updates
  3. At the update for node 71, the memory management is triggered (Nodes in memory > Rtabmap/MemoryThr), and Node 31 is one of the nodes transferred from WM to LTM. As its word 9950 is still referenced by node 65, word 9950 is not saved now in the database.
  4. At update for Node 75, Node 65 is reduced (STM->WM) to Node 4 of the previous loop closure. When a node is reduced, if the words of that node are not referenced by any other Node [in WM], the words are deleted directly (not saved to database). In this case word 9950 is deleted directly.
  5. At update for a later node, Retrieval is triggered on nodes around Node 30, so Node 31 is selected to be brought back from LTM to WM. When reactivating the words from Node 31, the word 9950 is neither in the database, neither in current dictionary state, thus errors like above happen!

The flaw is in step 4, we wrongly assume that "if a word is not referenced anymore by a node in current WM, we don't need to save it to database". This assumption works only if the node we remove is the latest one we just added to memory (e.g., when the robot is not moving, we delete the latest node by RGBD/LinearUpdate and RGBD/AngularUpdate parameters). I doublechecked when we delete nodes for Rehearsal and it still work as the old words are kept even if we update to new ID.

The easy fix would be to save all words to database of a signature deleted from graph reduction approach. This will go against the idea of reducing the graph to avoid increasing the database size over time. For this paper though, I didn't have this issue with both memory management and graph reduction enabled. I'll need to check deeper and see if I can reproduce with the data from that paper, if so, maybe compare the code between then and now.

From a quick look at the commits in the graph reduction section,

I can see that change from the last commit above could cause the current issue and why it didn't appear in the original version:

- this->moveToTrash(s, _notLinkedNodesKeptInDb);
+ this->moveToTrash(s, false);

I guess I would have the same error with old code version if the parameter Mem/NotLinkedNodesKept was set false. When true (default), all visual words are kept in database, following the "easy fix" solution above. I will set it back to true for now to avoid the error, but keep in mind that visual words size could grow less in the database if we do something more sophisticated to delete words only if they are not referenced in current WM AND if they are not referenced in LTM by some older transferred node!

EDIT: To add to my other comment above, the bug didn't appear in the 2022 paper because the memory management was not used at the same time, only graph reduction was used in the comparison (in contrast to previous papers, the goal of that paper wasn't about computation time issue). In that case, if a word was not referenced by another node in WM, it means it can be safely deleted (as all nodes not in WM would never be brought back anyway).

Originally posted by @matlabbe in https://github.com/introlab/rtabmap/issues/979#issuecomment-2058179851

borongyuan commented 3 weeks ago

Is it possible to add an option to keep the new node and remove the old node on graph reduction? If an environment continues changing, new nodes will capture new environment information, while old nodes will gradually become outdated. Therefore, if the new nodes are kept and the old ones are reduced, the entire system can better meet the needs of lifelong SLAM. I'm not sure if this could cause other problems.

matlabbe commented 2 weeks ago

One problem I can see is the map could drift over time. However, if we don't care too much about global position, that could be interesting to try. Currently, removing a node from working memory (WM) can be slightly more complicated than the one just added in short-term memory (STM), as we may have to change links in database (for nodes in long-term memory (LTM) linking the node in WM). But it could be doable, with maybe a new parameter to trigger this approach on graph reduction.

Another thing is that we may want to keep the old version to localize on later, in particular in cyclic environmental changes (day->night, seasons...).