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 helper API to encapsulate the check/create step for components #84

Closed
gerkey opened this issue Apr 21, 2020 · 8 comments
Closed

Add helper API to encapsulate the check/create step for components #84

gerkey opened this issue Apr 21, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@gerkey
Copy link
Contributor

gerkey commented Apr 21, 2020

In the course of migrating a plugin from Gazebo to Ignition (#80), I ended up following a pattern like this when accessing components:

if(!_ecm.EntityHasComponentType(this->dataPtr->modelLink, components::WorldPose::typeId))
{
  _ecm.CreateComponent(this->dataPtr->modelLink, ignition::gazebo::components::WorldPose());
}

That is, do some sort of lookup and if it fails, then create the component, and subsequently read from or write to it.

This pattern seems common enough that it would be worthwhile providing helper API to encapsulate it into a single PleaseGiveMeThisComponentButCreateItFirstIfYouNeedTo() call.

On a related topic, and also related to defining each system's component API (#83), it would be useful to know which components, if any, are guaranteed to exist. I found that sometimes I needed to do the check-create pattern and sometimes I could just use a component blindly without checking its validity. If that's just bad practice, and the right thing to do is to always check for existence of a component before accessing it, then we should say so in the docs (and also clean up internal component handling, e.g., the IMU system accesses the Name component without checking that it's valid)

@gerkey gerkey added the enhancement New feature or request label Apr 21, 2020
@peci1
Copy link
Contributor

peci1 commented Oct 2, 2020

The example Brian showed gets even more obscure when you need to set some value to the component:

https://github.com/ignitionrobotics/ign-gazebo/blob/9fb057712d5eb1c4965b2755bfb17e7ce6234fe3/src/systems/joint_position_controller/JointPositionController.cc#L231-L241

From the developer's point of view, this is nightmare...

Compare with Gazebo:

this->dataPtr->joints[iter->first]->SetForce(0, iter->second);

I think this design pattern (or rather antipattern?) really needs some simplification. The earlier it gets implemented, the less work will be spent on rewriting other parts of the codebase :) Let's make it a challenge for Edifice? :)

One good solution would be to provide wrappers like https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo4/src/Link.cc for most of the relevant entity types. I always wondered why there is Link.cc and not Joint.cc ...

@chapulina
Copy link
Contributor

One good solution would be to provide wrappers

That's in the works, see #375 #378 #325

Let's make it a challenge for Edifice?

The goal is to add these from Citadel onwards 😉

@chapulina chapulina self-assigned this Oct 2, 2020
@peci1
Copy link
Contributor

peci1 commented Oct 2, 2020

Great to hear that!

@chapulina
Copy link
Contributor

chapulina commented Oct 31, 2020

See #436 for a helper function that simplifies the process of creating and setting component values.

@peci1
Copy link
Contributor

peci1 commented Oct 31, 2020

Hooray!

@gerkey
Copy link
Contributor Author

gerkey commented Nov 3, 2020

Looking good! Can we also have a GetComponentData() helper?

@chapulina
Copy link
Contributor

Can we also have a GetComponentData() helper?

That was added in #378 😉

@gerkey
Copy link
Contributor Author

gerkey commented Nov 4, 2020

Got it! Unless something else is meant to be done, then I think that this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants