Add waypoint plugin for taking photos at arrivals about navigation2_tutorials HOT 9 CLOSED

This also overlaps the primary ticket: ros-navigation/docs.nav2.org#89

from navigation2_tutorials.

Rough review:

• The name... we should work on that. Also this plugin might be better suited to be in the waypoint follower directly, I think this is sufficiently useful to have as a plugin in the pkg.
• You have some cmake issues but we can deal with that in the real PR
• tabs... remove all tabs.... you should change your settings on your editor to never use tabs. I don't think any open source project in the ROS community will accept tabs
• The common.h stuff, isn't that stuff taken care of for your by image transport or image_common?
• Default paths / topic names should be more standardized, but we can clean that up quickly in the PR
• C++ contains proper filepath tools, you should use them
• Use image transport subscriber to allow people to use compression streams and it handles alot of the boiler plate for you

from navigation2_tutorials.

The name...

• Sure no problem, also welcome to suggest a name if there are any on your mind

You have some cmake issues

• Will be dealt with in PR

tabs... remove all tabs....

• Noted, I will use the auto formatting provided in ament_code_style.cfg

The common.h stuff

• I did saw these functions being embedded somewhere in image_pipeline. But in my case the simulated realsense camera encoding type is rgb8, a type which they don't cover in original implementation, so I had to add this modified function, I believe most of gazebo simulated cameras will have same encoding in default setting, so IMO it is better to keep functions here and have control over them.

Default paths / topic names

• Yes my username isn't the best default path, they will be modified

C++ contains proper filepath tools

• I need more info on this, for example at which lines we need to replace the existing code with native C++ path tools ?

Use image transport subscriber to allow

• This was my original ideal, but seems like we cannot do that at the moment, the parent node(waypoint_follower) is a managed LifeCycleNode, a type of Node image_transport does not support yet , relevant issue; ros-perception/image_common#108

So I will do modifications and submit a PR to navigation2 , does that mean the plugin wont be added to this repo ?

from navigation2_tutorials.

OK - on image_transport, it wouldn't take that long to make that all work. We have many examples of supporting different node types in Navigation2.

from navigation2_tutorials.

I suggest that we stick with the current state on that, to be able to make image_transport work with LifecycleNode will require modification image_transport API itself. I believe some time in future they will add that support. Also I haven't come across any package that use image_transport with a LifecycleNode at the moment

from navigation2_tutorials.

Images should always use image_transport, its really important. The efficiency gains of having access to compression is substantial. We can continue to a PR without it, but before merging, I'll want to have the image_transport stuff patched in.

from navigation2_tutorials.

I see , just an idea ,how about if we make this plugin to have double inheritance as such;

class WaypointPaparazzi : public nav2_core::WaypointTaskExecutor, public rclcpp::Node 
{
};

then I believe we will be able to use image_transport, but the parent LifecycleNode will be ghosted in that case ..

from navigation2_tutorials.

This will involve making image_transport lifecycle node compatible

from navigation2_tutorials.

We actually did this already :-)

from navigation2_tutorials.