Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bridge as a gz sim system #501
Bridge as a gz sim system #501
Changes from all commits
5d3d406
2882c87
4921a07
9ddd652
758aa21
f1f5fa0
be153b6
ac76877
3bc36bd
6f05a12
ebb6cab
bdcc825
42aa826
8d767e9
49c8811
e2a5db0
a195207
fe132a1
8c5b845
0ff8633
161f8ee
ddf084c
62b212c
15cbccb
93a0836
f232b4b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think it would be better to use
package://
URIs for the file name since this is all happening in the ROS ecosystem. Then, instead of callinggz::common::findFile
, we can pass the file directly toRosGzBridge
(so no need to callfindFile
here) and updateRosGzBridge
so it can useresource_retriever
to find the file.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.
resource_retriever
is functional but the API is slightly different because as far as I know it can't resolve the path only. Instead, it resolves the path and read the content of the file, returning it as a byte array.However I figured that we still can use the
package://
approach. I guessfindFile()
is solving it for us?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.
Yes, the API is different. That's why I was suggesting to pass the filename directly to
RosGzBridge
and useresource_retriever
there where we are currently opening the file:ros_gz/ros_gz_bridge/src/bridge_config.cpp
Line 176 in ef9cd5c
I don't know how
common::findFile
is handlingpackage://
. My guess is it ignores thepackage://
part and tries to find the path based on Gazebo environment variables, which is not what we want.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.
In any case I can tackle it in a separate PR as it affects mainly
ros_gz_bridge
. I already played with it.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.
#515
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.
Do we use
GAZEBO_RESOURCE_PATH
? I thought that was for Gazebo-classic.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.
Sorry, that was a typo. Changed in #511.
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 believe we can use
gz.msgs
for thegz_type_name
.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.
Changed in #511.
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.
Use
gz-sim
instead ofignition-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.
Changed in #511
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 rename this to be
name=ros_gz_sim::ROSGzBridge
andfilename=ros-gz-bridge-system
to havebridge
in the name and to be more consistent with our other systems (eg. we haven't usedPlugin
in any of our existing systems). For thefilename
, the shared library would need to belibros-gz-bridge-system.so
. In the<plugin>
tag thelib
and.so
can be dropped.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.
Updated in #511.