-
Notifications
You must be signed in to change notification settings - Fork 1
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
Proposal for simulation interfaces #1
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Adam Dąbrowski <[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.
As stated in the comments, I think EntityState should not be a part of Entity. Instead, a message EntityWithState could be created to bind the two.
Co-authored-by: Martin Pecka <[email protected]> Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]> robot_namespace -> namespace Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[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.
I left a couple comments. Unfortunately, I haven't had much time to review this PR, so there are still parts I haven't reached. I'll be on vacation for a couple weeks, but I'll try to make this a priority when I return.
Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]>
Signed-off-by: Adam Dąbrowski <[email protected]>
All comments so far have been addressed, please take another look and approve / suggest changes. |
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.
Thank you for putting in this work.
For progress sake, you may want to consider breaking this up into several PRs. If you made one with just the package metadata, then merge that immediately, and then the other bits can go in chunks.
I'd also like to integrate another interface for collisions but that is blocked at the moment.
Co-authored-by: David V. Lu!! <[email protected]>
Co-authored-by: David V. Lu!! <[email protected]>
- wording change for stepping service - features message - comment documentation for USD and URDF formats - custom formats list field Signed-off-by: Adam Dąbrowski <[email protected]>
@DLu thank you for the review, I applied most of your comments and responded to remaining 2 with some argumentation. |
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.
Sorry for the long delay in responding. I like the scope of the PR and the direction it's been evolving so far.
I'd like to ask if @omichel would also chime in for the Webots perspective.
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.
@DLu asked me to take a look at this due to my work on https://github.com/sea-bass/pyrobosim -- as such, you now have even more feedback in this very ambitious (and unenviable) task of standardizing such a thing.
Hope this is helpful!
Co-authored-by: David V. Lu!! <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>
- Changed SpawnPose to NamedPose, added tags. - SpawnEntity interface substantially updated. - Some improved documentation, especially regarding spawning. - applied other code review comments Signed-off-by: Adam Dąbrowski <[email protected]>
Is there a clear guideline to determine which actions/services must be implemented for a simulator to qualify as part of this standard? If so, should we mark the optional interfaces as "optional" in the definitions? Or is the simulator free to decide which interfaces they will choose to implement? |
@adamdbrw @peci1 I understand your point of a generic custom script to be inferior. I am fine with keeping it specific to the simulator in this case. Some generic simulator configs that we could set across all simulators could be something like setting target simulation frame rate or setting sensor/actuator publish rates on the fly? |
@ayushgnv I am thinking about support for setting of parameters, be it through SetEntityAttribute-like interface or through the custom service. I have 4 approaches in mind:
|
I believe the role of the standard is to control how useful, common features are interacted with:
As for mandatory interfaces to implement, I believe at least GetSimulatorFeatures should be implemented alongside a set of useful features which makes sense as a subset of the standard. Look into SimulatorFeatures.msg and be consistent in implementing any feature that is covered by particular type. I would say that spawning and getting state information about entities are going to be the most important for users, and so is simulation control (play, pause, stop). An example of standard implementation would be consistent entity control with spawning, deleting, getting / setting state, but not necessarily any tags, bounds or filtering. |
I think running / quitting is outside of simulation interfaces standard, something due to launch files etc. I imagine simulation interfaces to be something that happens when the simulator app is up, otherwise what implements the interfaces? Quitting is of course possible, but running simulation would require a service server, which needs to run somewhere. |
- Changed from string to Entity type for entity interfaces. - Added SimulationState SetSimulationState and GetSimulationState interfaces. - Clarified twist reference frame as suggested. Signed-off-by: Adam Dąbrowski <[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.
Is there a clear guideline to determine which actions/services must be implemented for a simulator to qualify as part of this standard? If so, should we mark the optional interfaces as "optional" in the definitions?
Or is the simulator free to decide which interfaces they will choose to implement?I believe the role of the standard is to control how useful, common features are interacted with:
- If feature X is regulated by this standard, and simulator has an X feature, it should be implemented to conform with standard.
- If feature Y is not regulated by this standard and specific to a certain simulator, Y can be implemented freely, but it should be implemented consistently with the standard.
- If feature Z is not regulated by this standard, but it is (becoming) common among simulators, feature Z should be suggested for inclusion into the standard.
As for mandatory interfaces to implement, I believe at least GetSimulatorFeatures should be implemented alongside a set of useful features which makes sense as a subset of the standard. Look into SimulatorFeatures.msg and be consistent in implementing any feature that is covered by particular type.
I would say that spawning and getting state information about entities are going to be the most important for users, and so is simulation control (play, pause, stop).
An example of standard implementation would be consistent entity control with spawning, deleting, getting / setting state, but not necessarily any tags, bounds or filtering.
Thanks for the clarification. I agree with you. Should we designate these service types as strongly suggested or mandatory either in documentation or in the srv definitions themselves? I am also happy to propose or add the changes.
"srv/DeleteEntity.srv"
"srv/GetEntities.srv"
"srv/GetEntitiesStates.srv"
"srv/GetEntityInfo.srv"
"srv/GetEntityState.srv"
"srv/GetSimulationState.srv"
"srv/GetSimulatorFeatures.srv"
"srv/ResetSimulation.srv"
"srv/SetEntityState.srv"
"srv/SetSimulationState.srv"
"srv/SpawnEntity.srv"
# Simulation Interfaces | ||
|
||
Standard ROS 2 interfaces for interacting with simulators. | ||
Messages, services, and actions are documented in their respective files. |
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 there a clear guideline to determine which actions/services must be implemented for a simulator to qualify as part of this standard? If so, should we mark the optional interfaces as "optional" in the definitions?
Or is the simulator free to decide which interfaces they will choose to implement?I believe the role of the standard is to control how useful, common features are interacted with:
- If feature X is regulated by this standard, and simulator has an X feature, it should be implemented to conform with standard.
- If feature Y is not regulated by this standard and specific to a certain simulator, Y can be implemented freely, but it should be implemented consistently with the standard.
- If feature Z is not regulated by this standard, but it is (becoming) common among simulators, feature Z should be suggested for inclusion into the standard.
As for mandatory interfaces to implement, I believe at least GetSimulatorFeatures should be implemented alongside a set of useful features which makes sense as a subset of the standard. Look into SimulatorFeatures.msg and be consistent in implementing any feature that is covered by particular type.
I would say that spawning and getting state information about entities are going to be the most important for users, and so is simulation control (play, pause, stop).
An example of standard implementation would be consistent entity control with spawning, deleting, getting / setting state, but not necessarily any tags, bounds or filtering.
Thanks for the clarification @adamdbrw. I agree with you. Should we designate these service types as strongly suggested or mandatory either in documentation (in this README) or in the srv definitions themselves? I am also happy to propose or add the changes.
"srv/DeleteEntity.srv"
"srv/GetEntities.srv"
"srv/GetEntitiesStates.srv"
"srv/GetEntityInfo.srv"
"srv/GetEntityState.srv"
"srv/GetSimulationState.srv"
"srv/GetSimulatorFeatures.srv"
"srv/ResetSimulation.srv"
"srv/SetEntityState.srv"
"srv/SetSimulationState.srv"
"srv/SpawnEntity.srv"
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.
This one remains to be addressed, but I think documenting the suggestion in the README could be the solution, though I would love someone from Gazebo team to also throw in a perspective.
Please re-review if possible as there were some changes recently. I would appreciate approvals or clear conditions for approval - if you believe there is something key missing or some feature is not as it should be in your opinion, please comment. I would love to wrap the review up. I will be off next week. |
Co-authored-by: Michał Pełka <[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.
This has improved greatly. The improvements of the Bounds is great as well as the Results.
I have two items that I'd like to ask to clarify.
@@ -0,0 +1,6 @@ | |||
# Entity type and additional information | |||
|
|||
uint8 category # Major category for the entity. Extra entity type distinction can be made through tags. |
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.
It's a little bit awkward here, but I think that the categories should be defined in the same message as the
Either we should move the enums here from EntityCategories.msg or we should move the data storage into the EntityCategories and included it as a sub-message.
uint8 category # Major category for the entity. Extra entity type distinction can be made through tags. | |
EntityCategories category # Major category for the entity. Extra entity type distinction can be made through tags. |
And the corellary change would be to have uint8 category
in the EntityCategories.msg field.
That has the downside of having to call category.category
but it keeps the data usage close to the definition. Otherwise a reader of this message only will find the datatypes if they read this message. And depending on how things are included or installed, may not have access to the other message as there's only a documentation link.
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.
The reasoning here is that if you have the constants outside, you can freely add new constants without changing message hash of the messages that use the constants.
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 understand there are two different opinions here. about whether the EntityCategories message should contain the category field itself. Either solution is good with me, do you have further arguments or are there others willing to express their preference?
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.
The reasoning here is that if you have the constants outside, you can freely add new constants without changing message hash of the messages that use the constants.
I think something like that should go into the string[] tags
field or be added to EntityCategories.msg
whenever the need arises.
Maybe something like GetSupportedTags.srv
could be added to get the constants for the tags that the simulator supports.
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 you sure you wanted another review? 🥲
Happy to discuss my feedback more.
# is expected to stop the simulation first, and then exit. | ||
# Simulation interfaces will become unavailable after quitting. | ||
# Running simulation application is outside of the simulation interfaces as | ||
# there is no service to handle the call when the simulator is not up. |
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.
Maybe?
# there is no service to handle the call when the simulator is not up. | |
# there is no service to handle the call when the simulator is not up. | |
uint8 BROKEN = 255 |
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.
My main issue here is that this is not very informative or actionable, and may be hard to implement. Simulators have hard time being able to self-inspect this way, and setting this state reliably would be problematic (implementation cost). I imagine most of the cases of broken simulator would be the cases where the BROKEN state is, deceptively, not properly set.
I can see some limited utility as to handle situations where we know something is not alright, as an information to the runner node to close and re-run the simulation app or to restart the simulation. But it doesn't answer "Broken how?", "How do I fix it?", or any other useful questions.
This is my reasoning for being skeptical about this suggestion, but I am open to argumentation.
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'm not tied to it. 👍
# to provide a return code which is more specific. | ||
|
||
uint8 result # Result to be checked on return from service interface call | ||
string error_message # Additional error description when useful. |
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.
The existence of this message seems like a fundamental failing of ROS 2.
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.
Could you elaborate? I think I understand what you mean but don't want to jump to conclusions.
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 discussed this in the PMC meeting (a few weeks ago), I do think that this status
message should be a first-class citizen in the ecosystem. It's very similar to absl::Status
.
You will see that many services do this pattern, which indicates to me that it should at least be a common message, but it seems very likely that this could be built into the underlying service mechanism.
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.
Thanks for all the hard work! This looks great to me.
I will be disappearing for a while due to personal reasons, so I'm going to tag in other folks to help with merging and releasing this packages once all the comments have been resolved.
Co-authored-by: David V. Lu!! <[email protected]>
- Entity message is removed as it was just a string. - Suggestions applied to usage of bounds. - Renaming of MultiStepSimulation to SimulateSteps. - Adjustments of SimulationFeatures to changes. - Multiple documentation clarifications / additions. Additionally, introduced TYPE_EMPTY for bounds for consistency. Signed-off-by: Adam Dąbrowski <[email protected]>
# Entities matching any of the categories will be returned. | ||
# To get entity category, use GetEntityInfo. | ||
# Applies together with other filters (filter, tags). | ||
TagsFilter tags # Tags filter to apply. To get entity tags, use GetEntityInfo. |
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.
Tags for entities are part of spawnable. There is no way to apply tags to entity during /after spawning. The same is for categories. Please correct me if I am wrong @adamdbrw .
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 can spawn entities, but we cannot create named pose. I recommend adding a service definition to create a named pose. It can be a part of simulated world, or level, but also it can be a part of test scenario.
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 guess the benefit would be mostly in delegating bounds-related condition checks to the simulator? Named poses can be a scenario-only thing otherwise (e.g. you name a pose variable in the scenario itself).
srv/GetNamedPoseBounds.srv
Outdated
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 can spawn entities, but we cannot create named bound. I recommend adding a service definition to create a named bound. It can be a part of simulated world, or level, but also it can be a part of test scenario.
# The regular expression syntax is POSIX Extended, | ||
# see https://pubs.opengroup.org/onlinepubs/9799919799/ definitions. | ||
# Applies together with other filters (categories, tags). | ||
uint8[] categories # Optional, defaults to empty, which means no category filter. |
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.
Signed-off-by: Adam Dąbrowski <[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.
So I think we should mark Bounds.msg
and Result.msg
as things that should be migrated upstream in the long term. Bounds.msg
should probably be integrated with shape_msgs
and @mjcarroll probably has a better opinion of where Result should go.
# For entities, these limits are relative to entity's top link transform, following ROS rep-103 convention. | ||
|
||
# As bounds are optional in most interfaces, TYPE_EMPTY signifies empty bounds, to be understood as "unbounded". | ||
# Otherwise, the fields are expected to define a valid area containing the (0,0,0) point which is the position. |
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 am confused by this statement. Let's say we have {type: TYPE_SPHERE, points: [{x: 5}, {x: 1}]}
which I was intending to mean a sphere centered at (5, 0, 0) with a radius of 1. That does not contain the point (0, 0, 0).
At the very least, this should be tweaked to say "valid volume"
uint8 TYPE_BOX = 1 # Bounding box, points field should have two values, which are | ||
# upper right and lower left corners of the box. |
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.
uint8 TYPE_BOX = 1 # Bounding box, points field should have two values, which are | |
# upper right and lower left corners of the box. | |
uint8 TYPE_BOX = 1 # Axis-aligned bounding box, points field should have two values, | |
# which are upper right and lower left corners of the box. |
It is a busy time for me now but I will do my best to apply all the feedback this week. |
Following ros-infrastructure/rep#410, this PR contains a first version of simulation interfaces.