search

ros-perception / image_pipeline

An image processing pipeline for ROS.
Other
801 stars 729 forks source link

Initial Merging: Setting up ImageSaverNode #944

Open CursedRock17 opened 9 months ago

CursedRock17 commented 9 months ago

This PR looks to solve issue #935 by Merging ExtractImagesNode into ImageSaver. Before we go ahead and do it, I assume we want ExtractImagesNode entirely removed since ImageSaver will now have all the functionailty and more of that previous node, opposed to just inheriting from that class.

mikeferguson commented 9 months ago

Yes - there really isn't any reason to have two separate nodes that have similar, but not identical features. If image_saver supports saving at a particular rate, then it replaces the functionality of extract images node

CursedRock17 commented 9 months ago

Adding [a] new "fps" parameter to ImageSaver - when the parameter is set, we should setup a timer to fire and save images.

Do we need to make a TimerBase with a new callback, or does that seem excessive? Furthermore, if we had that callback we would need to get access to that image, from inside the constructor of ImageSaverNode

mikeferguson commented 9 months ago

Do we need to make a TimerBase with a new callback, or does that seem excessive? Furthermore, if we had that callback we would need to get access to that image, from inside the constructor of ImageSaverNode

The end goal is to save at a fixed rate, regardless of the rate that the image shows up - I haven't come up with anything better than a ROS timer. The timer would require another callback (the function signature is different). When FPS parameter is set, we would want the image callback to save the latest image in a class variable so that the timer callback can access it (and that also means adding a mutex to protect that shared variable in case somebody loads the component with a multi-threaded executor).

CursedRock17 commented 3 months ago

@mikeferguson friendly ping, this is ready for review