Skip to content
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

Added support for using ROS parameters to spawn entities in Gazebo using ros_gz_sim::create #465

Closed
wants to merge 7 commits into from

Conversation

Ayush1285
Copy link
Contributor

@Ayush1285 Ayush1285 commented Nov 17, 2023

🎉 New feature

Closes #459

Summary

Added ROS parameters support in ros_gz_sim::create node, while preserving the existing behavior. ROS parameters will be checked first, if not found then will revert back to Gflags arguments.

Test it

Launch the demo launch file from the ros_gz_sim_demos package.
ros2 launch ros_gz_sim_demos robot_description_publisher.launch.py

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

azeey and others added 3 commits November 17, 2023 16:49
* Add harmonic CI (gazebosim#447)

* Add harmonic CI

Signed-off-by: Michael Carroll <[email protected]>

* Include garden options

Signed-off-by: Michael Carroll <[email protected]>

* Add harmonic stanza

Signed-off-by: Michael Carroll <[email protected]>

* Additional message headers

Signed-off-by: Michael Carroll <[email protected]>

---------

Signed-off-by: Michael Carroll <[email protected]>

* Use stable osrf repo for Garden and stable+prerelease for Harmonic (gazebosim#448)

Harmonic is now in the prerelease repository

Signed-off-by: Jose Luis Rivero <[email protected]>

* Add additional cmake/package.xml changes
* Update headers
* Update namespaces
* Add ci for harmonic/humble
* Update readme

---------

Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Ayush1285 <[email protected]>
Ayush1285 and others added 2 commits November 17, 2023 17:06
Signed-off-by: Ayush1285 <[email protected]>
@Ayush1285
Copy link
Contributor Author

@ahcorde can you please review it for approval?

@Ayush1285
Copy link
Contributor Author

@ahcorde approval required for the workflow, can you please look into it?

@azeey
Copy link
Contributor

azeey commented Dec 5, 2023

Thanks for the contribution, @Ayush1285! Is this a duplicate of #463?

@Ayush1285
Copy link
Contributor Author

Ayush1285 commented Dec 5, 2023

Thanks for the contribution, @Ayush1285! Is this a duplicate of #463?

Just checked the other PR. I guess we both created PRs parallelly for the same issue.

@azeey
Copy link
Contributor

azeey commented Dec 21, 2023

@Ayush1285 , I've closed the other PR. Let's iterate on this one and get it merged.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

do you mind to target rolling ? then we can backport the changes to humble and iron.

const std::string & topic_name, const rclcpp::Node::SharedPtr ros2_node,
gz::msgs::EntityFactory & req)
{
const auto timeout = std::chrono::seconds(1);
Copy link
Collaborator

@ahcorde ahcorde Dec 22, 2023

Choose a reason for hiding this comment

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

include <chrono>, <future>, <functional>

Comment on lines +87 to 90
}


int main(int _argc, char ** _argv)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
int main(int _argc, char ** _argv)
}
int main(int _argc, char ** _argv)

@@ -59,8 +99,24 @@ int main(int _argc, char ** _argv)
[-R ROLL] [-P PITCH] [-Y YAW])");
gflags::ParseCommandLineFlags(&_argc, &_argv, true);

// Declare ROS Parameters to be passed from Launch file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Declare ROS Parameters to be passed from Launch file
// Declare ROS 2 parameters to be passed from launch file

Comment on lines +87 to +91
arguments=[
'-name', 'my_custom_model',
'-x', '1.2',
'-y', '3.3',
'-z', '2.4'
Copy link
Collaborator

Choose a reason for hiding this comment

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

these arguments are parameters now, aren't they ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these arguments are parameters now, aren't they ?

Yes, I'll move them to parameters.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Remove gflags on rolling too

@azeey
Copy link
Contributor

azeey commented Dec 22, 2023

Remove gflags on rolling too

Why are we removing gflags? Wouldn't this break a lot of launch files already out there?

@ahcorde
Copy link
Collaborator

ahcorde commented Dec 22, 2023

maybe add a warning

@Ayush1285
Copy link
Contributor Author

do you mind to target rolling ? then we can backport the changes to humble and iron.

Yes, I've created a new PR #475 for rolling.

@ahcorde
Copy link
Collaborator

ahcorde commented Dec 26, 2023

I will close this PR in favour of this PR on rolling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants