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

Add a convenience function for getting possibly non-existing components. #629

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Feb 12, 2021

New feature

Continues in the line of #436 and #378.

Summary

Adds a convenience function EntityComponentManager::ComponentDefault() which works as Component(), but if the component isn't found, it automatically creates it (using a user-supplied default value).

I have an example usage of the function in osrf/subt#551 (which uses a shim until this PR is merged).

I'm not really sure whether ComponentDefault is the best name. If you come up with something better, I'll be glad. I was also thinking about adding optional parameters to Component() function, but that would actually need two of them - one boolean saying whether the component should be auto-constructed, and then the supplied default value. I felt that wouldn't really be nice to use and it would be less self-descriptive than having a function whose name explicitly states that it can auto-create the component if needed.

Test it

A unit test was added.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example world and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@peci1 peci1 requested a review from chapulina as a code owner February 12, 2021 13:00
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Feb 12, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This is an interesting use case that didn't occur to me before, but thinking of it now it makes sense. It's interesting how similar it is to SetComponentData but has an important difference.

It initially reminded me of rclcpp::Node::get_parameter_or: "Get the parameter value, or the "alternative_value" if not set, and assign it to "parameter"." (even though that does not "create" (i.e. declare) the parameter if not existent).

And then I remembered sdf::Element::Get, which has pretty much the same behaviour as the function here. So my naming suggestion would be to make this an overload of Component that accepts a default value.

include/ignition/gazebo/EntityComponentManager.hh Outdated Show resolved Hide resolved
include/ignition/gazebo/detail/EntityComponentManager.hh Outdated Show resolved Hide resolved
src/EntityComponentManager_TEST.cc Outdated Show resolved Hide resolved
@peci1
Copy link
Contributor Author

peci1 commented Feb 12, 2021

Thanks for your comment, Louise. If I understood you correctly, adding an overload of Component() with the default parameter would mean that at call time, the user would always need to provide "something" for the default value. I first thought the user would need to type in either a value or Type() for default-constructed value, which would make the API a little more complicated to use (always needing to search which particular type does the component use for storage), but then I found out you can provide just {} as the default value and it works :)

I think it might be a bit unintuitive for others, because I myself came to know initializers only recently, but it would allow for calling in the fashion of:

ecm.Component<component::Double>(entity, {});

It might be a good compromise between ease of use and compactness.

If there are no other ideas, I'll have a look at it over the weekend...

@chapulina
Copy link
Contributor

the user would always need to provide "something" for the default value.

Good point, I can see how that could make the API a bit less intuitive. I'm not opposed to ComponentDefault though, if there are no other ideas and you think that's clearer, I'm ok with that.

Signed-off-by: Martin Pecka <[email protected]>
@peci1
Copy link
Contributor Author

peci1 commented Feb 23, 2021

Okay, this was actually two weekends, but as no new ideas came, I vouch for the more intuitive API with ComponentDefault.

I applied the style nits.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me 👍

@chapulina chapulina merged commit 159f620 into gazebosim:ign-gazebo4 Feb 23, 2021
peci1 added a commit to peci1/subt that referenced this pull request Mar 3, 2021
@diegoferigo
Copy link
Contributor

xref #325

mjcarroll pushed a commit to osrf/subt that referenced this pull request Apr 8, 2021
* Absolem: Add relative positional control.

* Absolem: Added a reference to the issue blocking implementation of the differential.

* Absolem: Read flipper velocity and effort limits from the joint limits, too.

* Absolem: Preparations for setting max torque in flippers.

* Absolem: Simplify interaction with ECM using the new APIs.

* Absolem: Added TODO referencing a PR in ign-gazebo that would allow deleting the ECM shim from this package.

* Simplify the code with gazebosim/gz-sim#629 .
chapulina pushed a commit that referenced this pull request Jul 23, 2021
chapulina pushed a commit that referenced this pull request Jul 23, 2021
chapulina pushed a commit that referenced this pull request Jul 23, 2021
blakermchale pushed a commit to blakermchale/ign-gazebo that referenced this pull request Aug 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants