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

Multiple Map Loading #146

Merged
merged 8 commits into from
Feb 14, 2025
Merged

Conversation

remco-r
Copy link
Contributor

@remco-r remco-r commented Feb 10, 2025

This feature enables merging multiple maps in the offline viewer.

After the first global map is loaded, an additional map can be loaded into the existing GlobalMapping.
The state (factor id's) of the existing global_mapping will be checked, and the new Graph and Values will be added after rekeying.

At this stage two maps are loaded into the ISAM optimizer. Since these two maps are not connected, an indeterminant system error can be thrown. This is solved by adding a temporary between factor between the two graphs. It is up to the user to connect the two graphs afterwards in the viewer. Upon the next optimzation step, the temporary factor will be removed.

glim_map_merge

issues:

  • merging larger maps can become unstable (graphs with 1000+ frames), sometimes cant optimize and damping factors will be added.

@koide3
Copy link
Owner

koide3 commented Feb 12, 2025

Hi @remco-r ,

Thanks a lot for your contribution! This is a huge improvement! I'll soon review the PR and give you feedback.

@koide3
Copy link
Owner

koide3 commented Feb 13, 2025

Hi @remco-r ,

I tested this new feature on my side, and I successfully merged three dump files without problems --- IT IS AMAZING! While I found that the implementation is mostly fine, I added a few modifications to simplify the rekey process and improve optimization stability. Can you merge https://github.com/koide3/glim/tree/mmm into this branch? Then, I think the PR will be ready to be merged!

The modifications I made are:

  • Deferring optimization when an additional dump is loaded instead of inserting a dummy relative pose factor
  • Simplifying the rekey process with the submap key naming rule (X(i) has corresponding E(i2) and E(i2+1))
  • Renaming method names according to the style of other methods

@remco-r
Copy link
Contributor Author

remco-r commented Feb 14, 2025

Hi @koide3,

Thanks for the review. I like your updates, they simplify things. I cherry picked your commit, and added one commit on top where i removed one more obsolete thing.

About the rekeying process. I had it like you've done it before, but i believed you cannot guarantee exactly two between factors per submap. Because i thought when you manually connect two frames in the offline_viewer, it also creates a between factor, so there may be an arbitrary amount of extra between factors in the graph that is loaded. However, i just did a small quick test, and it still worked, so maybe i understood it wrong.

rekey_mapping[E(i * 2)] = E((i + start_from_frame_id) * 2);
rekey_mapping[E(i * 2 + 1)] = E((i + start_from_frame_id) * 2 + 1);

src/glim/mapping/sub_map.cpp Outdated Show resolved Hide resolved
@koide3
Copy link
Owner

koide3 commented Feb 14, 2025

About the rekeying process. I had it like you've done it before, but i believed you cannot guarantee exactly two between factors per submap. Because i thought when you manually connect two frames in the offline_viewer, it also creates a between factor, so there may be an arbitrary amount of extra between factors in the graph that is loaded. However, i just did a small quick test, and it still worked, so maybe i understood it wrong.

I think it doesn't matter because graph.rekey() changes the keys of all factors. Even if there are several manually created loop factors, their keys should also be updated accordingly.

am i correct to assume that submap->id is either not correlated to the rekeyed frame_id, or not necessary to be correct?

Yes, submap->id is used for only submap management purposes, and it is fine to update it after loading.

Also, this line does nothing now.

Ah, I'll remove it later.

I'm merging this PR into the main branch.
This is a very big improvement, and it will be helpful for the community (I’ll announce it on X and credit you).
Thank you so much again, @remco-r!

@koide3 koide3 merged commit d8d79bb into koide3:master Feb 14, 2025
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