-
Notifications
You must be signed in to change notification settings - Fork 777
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
[ros2] Port multicamera to ros2 #939
Conversation
Now that #932 has been merged, this needs rebasing |
768a760
to
c377a96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! The requests for changes are minimal, the only critical one is changing the plugin namespace.
#include "gazebo/rendering/Camera.hh" | ||
#include "gazebo/util/system.hh" | ||
|
||
namespace gazebo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this plugin in the gazebo_plugins
namespace? Just in case someone creates the same plugin for Gazebo in the future.
In fact, feel free to make a pull request adding this plugin to Gazebo 9 if you have time. It would be just a matter of copying this and the cc file over, and making a couple tweaks to follow Gazebo's style guide (which is different from ROS 2).
I can help you out with style changes for you since I'm used to it. Some things I can already see:
- rename
.hpp
to.hh
and.cpp
to.cc
- Use
camelCase
(i.e.cameraNum
) - Prefix all arguments with
_
(i.e._cameraNum
) - There should be one
public
/protected
/private
per member {
in a new line
The only problem is that even if we release this plugin for Gazebo 9.X, it won't be available to most ROS users, which use Gazebo 9.0.0 from Ubuntu. So we woudn't be able to use it in gazebo_ros_pkgs
's debian packages 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be making the PR soon
Signed-off-by: Louise Poubel <[email protected]>
LGTM. We could backport this to |
Port gazebo_ros_multicamera plugin to ROS2.
Example:
Instead of a new plugin, multicamera has been integrated into
gazebo_ros_camera
. To use it as multicamera, <sensor_type> has to be specified as 2. It also includesgazebo_ros_triggered_multicamera