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 docs to wheel_slip_parameters_cmd.proto #227

Merged
merged 3 commits into from
Mar 12, 2022

Conversation

ivanpauno
Copy link
Contributor

@ivanpauno ivanpauno commented Mar 10, 2022

🦟 Bug fix

Fixes #205 (comment).

Summary

Add docs to wheel_slip_parameters_cmd.proto.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno added the enhancement New feature or request label Mar 10, 2022
@ivanpauno ivanpauno requested a review from caguero as a code owner March 10, 2022 14:13
@ivanpauno ivanpauno self-assigned this Mar 10, 2022
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 10, 2022
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #227 (05178a7) into ign-msgs7 (eff9354) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-msgs7     #227   +/-   ##
==========================================
  Coverage      85.60%   85.60%           
==========================================
  Files              9        9           
  Lines            924      924           
==========================================
  Hits             791      791           
  Misses           133      133           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eff9354...05178a7. Read the comment docs.

@ivanpauno ivanpauno mentioned this pull request Mar 10, 2022
8 tasks
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 adding docs!

@@ -31,9 +31,29 @@ message WheelSlipParametersCmd
/// \brief Optional header data
Header header = 1;

/// \brief Name of the model we want to modify the wheel slip parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to clarify if it's the scoped name (i.e. parent_model::model) or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that I'm not using scoped names in the current implementation (gazebosim/gz-sim#1241), should I be?
If it's common to reuse model names but have different parents for each model, maybe I should be using scoped names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, it looks like you're treating them as unscoped names.

I think it would be good to support nested models in some way. I can imagine creating a single wheel model that's repeated multiple times inside the same vehicle, for example, and they all may end up with the same name. I can't think of a clean way to do it with just model_name and link_name. Maybe if model_name accepts scoped names, but the implementation will need to be updated. My suggestion would be to remove string model_name and string link_name in favor of Entity link_entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My suggestion would be to remove string model_name and string link_name in favor of Entity link_entity.

That sounds good to me.
Is name in that case scoped?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha I thought that message was better suited to work with nested models but looking at it now I think it isn't. Just saw this bit of code used to get "top-level" (i.e. children of the world) entities using the message.

Now I can't think of a clean way of using the Entity message for nested models unless the name is scoped. So it ends up not being too different from what you have here, except that the user can alternatively pass the ID, or make the type explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

except that the user can alternatively pass the ID

Yes, this might be useful in some cases.

or make the type explicit.

I think it's always going to be a link, so that's maybe not needed (?).


In the current approach, you can use the same link name in different models, and distinguish them using the model name.
Would that be possible if I start using msgs::Entity?
e.g. https://github.com/ignitionrobotics/ign-gazebo/blob/f5bb284884de9c5d4d7fa5614e53b528654b6611/examples/worlds/trisphere_cycle_wheel_slip.sdf#L283 and https://github.com/ignitionrobotics/ign-gazebo/blob/f5bb284884de9c5d4d7fa5614e53b528654b6611/examples/worlds/trisphere_cycle_wheel_slip.sdf#L685.

Ideally, I agree that supporting nested models would be the best.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's always going to be a link, so that's maybe not needed (?).

I think it's mostly useful if there's a generic way to handle the Entity message which multiple systems can use. This plugin could handle an error in case the user specifies a joint, for example.

It would be helpful to distinguish between links and other entities, in case there are name overlaps. For example, it's common to have links and joints with the same name (i.e. right_wheel).

Would that be possible if I start using msgs::Entity?

Only if the name is scoped, I believe. I think it would be reasonable to use the Entity message this way. By the way, the entitiesFromScopedName helper function should help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only if the name is scoped, I believe. I think it would be reasonable to use the Entity message this way. By the way, the entitiesFromScopedName helper function should help.

Sounds good, I can try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got that working, see 7c15ad8 and gazebosim/gz-sim@2d98ab9.

proto/ignition/msgs/wheel_slip_parameters_cmd.proto Outdated Show resolved Hide resolved
proto/ignition/msgs/wheel_slip_parameters_cmd.proto Outdated Show resolved Hide resolved
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
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 iterating, LGTM!

@chapulina
Copy link
Contributor

Merging with a failing ABI checker because the first version of the message hasn't been released yet.

@chapulina chapulina merged commit 6e5c9ef into ign-msgs7 Mar 12, 2022
@chapulina chapulina deleted the ivanpauno/wheel-slip-parameters-cmd-docs branch March 12, 2022 02:02
@chapulina chapulina mentioned this pull request Mar 12, 2022
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

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

Successfully merging this pull request may close these issues.

3 participants