-
Notifications
You must be signed in to change notification settings - Fork 269
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 more link APIs, with tutorial #375
Conversation
Signed-off-by: Louise Poubel <[email protected]>
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.
LGTM with minor comments. Probably need to rerun CI though
{ | ||
auto comp = _ecm.Component<components::WindMode>(this->dataPtr->id); | ||
if (comp) | ||
return comp->Data(); |
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.
Is comp->Data()
always going to be true
if set, or can it be set as true
or false
? The logic here might be confusing since we don't immediately know the type of Data()
is compatible with the return bool
value.
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.
we don't immediately know the type of Data() is compatible with the return bool value
After casting the component to components::WindMode
, yes, the data will always be bool. If WindMode
's data type ever changes, this will fail compilation. Note that this is different from CanonicalLink
above, which doesn't hold any data.
I guess it may be hard for a developer to know that the type of WindMode
is bool
without looking at that class' definition, but I'm not sure how to make this clearer here.
Is comp->Data() always going to be true if set, or can it be set as true or false?
Any system could change the set the value to true or false.
# Migration from Gazebo-classic: Link API | ||
|
||
When migrating plugins from Gazebo-classic to Ignition Gazebo, developers will | ||
notice that the C++ APIs for both simulators are quite different. Be sure to |
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.
Are we doing one sentence per line?
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 usually try to keep each line to less than 80 chars so it's easier to see diffs in the future. Would you prefer one sentence per line? Some sentences are quite long...
Signed-off-by: Louise Poubel <[email protected]> Signed-off-by: Guillaume Doisy <[email protected]>
Model::Parent
because I just realized we haveecm.ParentEntity
. We need to be careful to merge this before the other one is released.TODO
s to the empty items on the table, because leaving it empty doesn't render well, see https://ignitionrobotics.org/api/gazebo/3.3/migrationmodelapi.htmlPart of solving #325