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

Markers and visualization #622

Merged
merged 5 commits into from
Dec 15, 2024

Conversation

JLiesenborgs
Copy link
Contributor

We have been experimenting with markers in stella_vslam. I cleaned up the modifications we made that seem to improve their use. I've tried to be quite elaborate in the commit messages, to explain what they do.

@alvodo
Copy link

alvodo commented Nov 5, 2024

This can fail because the keyframe_inserter sets mkr->initialized_before_ = true but this does not add the marker to the marker_vertex_container. However, since the marker claims to be initialized, the local_bundle_adjuster_g2o wants to load from the container which is not necessarily filled yet. There is a similar problem in the global bundle adjuster and this is probably the same with the other backend as well.
I think this is a race condition as it doesnt always happen, but I have no idea about threads in C++. I do not know what the best way to fix it is.

@JLiesenborgs
Copy link
Contributor Author

Well spotted! I'll think about how best to deal with this issue

@alvodo
Copy link

alvodo commented Nov 12, 2024

And the markers need to be cleared in map_database::clear.

@tvt15
Copy link

tvt15 commented Nov 12, 2024

@JLiesenborgs Are you currently working on how to deal with the issue still or were you able to solve it?

@JLiesenborgs
Copy link
Contributor Author

I haven't had much time yet, but I do intend to look into this. Should I hurry?

@tvt15
Copy link

tvt15 commented Nov 12, 2024

Im currently working on application of stella_Vslam marker integration as well. Just wanted to know more about your approach

@tvt15
Copy link

tvt15 commented Nov 12, 2024

If we could connect over linkedin or email, I wanted to bounce some ideas off of you.

@JLiesenborgs
Copy link
Contributor Author

@alvodo I apologize for taking this long, but I've finally added two commits to this pull request. The first should take care of the race condition: it records in which cases the vtx info was actually created. The second one is the small change to the 'clear' routine in map_database, as requested.

If this looks acceptable, and you'd like to approve the pull request, it's probably best to squash the commit with the race condition fix into an earlier commit. I left it as a separate one for now for clarity, better suited for review.

@tvt15 if you check out my fork of the repository, you can find an email address you can use in the commit log

@tvt15
Copy link

tvt15 commented Nov 19, 2024

@JLiesenborgs I have sent you an email

@alvodo
Copy link

alvodo commented Nov 25, 2024

@JLiesenborgs No problem, thanks for the fix! I couldnt reproduce the problem with your new version, I would therefore conclude that it is fixed. I am not a maintainer or anything of this repo, so my approval wouldnt do anything. Maybe this is a good point in time for @ymd-stella to look at it?

j0r1 added 4 commits November 26, 2024 09:49
Loading/saving marker info, 2D and 3D, to sqlite3

keep_fixed_ flag in marker: previously, the marker positions were not
optimized, the position was estimated once based on a single observation
and kept fixed. The default is now to include the position in the
optimization, which could still be turned off by setting the flag to
true (would currently need to be done in the code)

initialized_before_ flag in marker and
"required_keyframes_for_marker_initialization" setting: if a single
observation of the marker is used to estimate its 3D position, a bad
estimate can still throw off the optimization procedure, causing the
markers to mess up the map building instead of helping. The setting
allows the initialization to wait until a number of keyframes have seen
the marker. Only when this happens, the flag is set, and the marker will
be included in the optimization. The default is to wait for 3 frames,
but can be controlled using a config file setting.
This patch add support for using the aruco nano header file
(https://sourceforge.net/projects/aruco/files/ArucoNano-v5/) to
detect markers. The user can enable the support using a CMake
config setting, and then use "aruconano" as marker model.
This draws the 2D detected markers, together with their IDs,
on the observed image. Because this needs the original coordinates
(not the undistorted ones), they are stored in the marker2d
class now as well (not saved/loaded to database though). To
allow a publisher (eg the socket_publisher) to share the 3D marker
info, a method has been added to retrieve them.
@JLiesenborgs JLiesenborgs force-pushed the markers_and_visualization branch from c5234f6 to 12e5d36 Compare November 26, 2024 16:24
@JLiesenborgs
Copy link
Contributor Author

I've fixed these formatting issues (I ran the tool locally, so I hope I caught the same issues), and also squashed that fix for the race condition into the relevant commit.

@JLiesenborgs
Copy link
Contributor Author

@alvodo @ymd-stella Just a small message to notify you - I forgot to mention you a few days ago, I don't know if you get to see the update without it (apologies if you were already informed)

@tvt15
Copy link

tvt15 commented Nov 29, 2024

@JLiesenborgs Hey Jori, I sent you an email regarding your work. Please do check it out when you get the time.

@ymd-stella ymd-stella self-requested a review December 14, 2024 23:53
@ymd-stella ymd-stella force-pushed the markers_and_visualization branch 3 times, most recently from 5fc2425 to f4ad896 Compare December 15, 2024 05:20
- Delete references from markers when erasing keyframes
- required_keyframes_for_marker_initialization can now be set individusually in KeyframeInseter and Initialiser
@ymd-stella ymd-stella force-pushed the markers_and_visualization branch from f4ad896 to d91beef Compare December 15, 2024 05:51
@ymd-stella ymd-stella merged commit 27c5915 into stella-cv:main Dec 15, 2024
5 checks passed
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.

5 participants