Skip to content

Conversation

cKohl10
Copy link
Contributor

@cKohl10 cKohl10 commented Jul 9, 2025

Changelog

None

Docs

Tutorial blog post currently in progress.

Description

Added Carson Kohlbrenner's work trial project converting the EuRoC MAV dataset into a MCAP SLAM visualization.

BeforeAfter

Copy link
Contributor

@msadowski msadowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skim-read this and left some comments. All in all it looks good but since there is some ROS code, I would appreciate a proper ros package structure that you could build. This could be a ros_ws directory inside the EuRoC_MAV, but ideally it would come with a .gitignore rules that will ignore any build artificats.

The reason why I would push for the proper ROS package is that I really want people to be able to run these examples in 5 minutes without thinking of creating a package from scratch

@@ -0,0 +1,396 @@
# Visualizing SLAM using Foxglove
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a frontmatter section to this README.md? Then it will be nicely rendered in the main README file. You can find more details on the process here: https://github.com/foxglove/tutorials/blob/main/CONTRIBUTING. It would be good if an empty blog post is created for it. This will create a slug that you can turn into a blog post address. You can check other readmes on the process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this README differ from the target tutorial? We can discuss this with @admfrk, but I was hoping we would have a very worded tutorials on the website, and a minimal tutorial in the README.md that allows you to repro the work in 5 minutes or less (ideally provide the commands to run to create or visualize the dataset).

@@ -0,0 +1,178 @@
# This script is used to convert the Euroc dataset to MCAP format
# Author: Carson Kohlbrenner
# Date: 2025-06-27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance you could remove the comments here to align with other tutorials? The author data will be preserved in git and github if anyone needs this information.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file needed? What value does it add over what can README contain?

writer = foxglove.open_mcap(out_mcap_path, allow_overwrite=True) # Open the mcap file for writing

# Define the schemas for our topics
# get the .msg file that ROS installs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you plz remove the commented out code if it's not useful?

from rclpy.serialization import serialize_message
from pathlib import Path

def getImageMsg(img_path: str, timestamp: int, cam_num: int) -> Image:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use consistent naming scheme across the source code? Ideally following PEP8 and consistent approach to static typing

@cKohl10
Copy link
Contributor Author

cKohl10 commented Jul 17, 2025

I appreciate the in depth comments! I know what to look out for next time. I'll fix this up asap

@cKohl10
Copy link
Contributor Author

cKohl10 commented Jul 20, 2025

I believe I addressed all of your comments, as well as improved the package to be easier to run with a couple CLI commands. I tested this repo out on a fresh Ubuntu flash and I ran into no problems

@msadowski
Copy link
Contributor

Thanks! It looks great to me!

Will this one come with a blog post? If yes, it would be great to create the blog post with CMS and add the blog_post_url variable to the README frontmatter. That way we can publish the blog post and merge this at the same time, and don't need to make another PR once the blog post is out.

Are you able to create a blogpost or will we need @admfrk 's help?

@cKohl10
Copy link
Contributor Author

cKohl10 commented Jul 21, 2025

@admfrk is helping me out with the blog post this time I believe. I'm happy to keep this open until I get the link to put in the README

@cKohl10
Copy link
Contributor Author

cKohl10 commented Jul 21, 2025

@admfrk I am also happy to take on publishing this blog myself with a little bit of guidance!

---
title: "EuRoC MAV Dataset in Foxglove"
short_description: "Drone SLAM of the MH01 Environment from EuRoC MAV"
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
---
blog_post_url: "https://foxglove.dev/blog/converting-euroc-mav-dataset-to-mcap"
---

I checked with @admfrk, @cKohl10 can you please incorporate this suggestion, and then we are good to merge?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing: once this suggestion is merged, can you please regenerate the readme so that the blog post is linked in the main readme?

@cKohl10
Copy link
Contributor Author

cKohl10 commented Jul 21, 2025

Updated the README. I think it should be all set. Thanks for the help @msadowski

@admfrk admfrk merged commit afc1d91 into foxglove:main Jul 31, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants