Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Proposal for simulation interfaces #1
Changes from 12 commits
5267ea2
0cfd0fd
d874c23
10f59d2
ea23f59
97b79f7
8bae938
885a3fa
53d6ee9
aaee904
df98b24
4d551f1
95f46d9
0169e10
1e92784
9bf757f
e94797f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think for ground truth twist, the body twist has to be expressed in the world frame. In the convention mentioned in ros2/common_interfaces#240, we'd have:
@tfoote can you confirm? When both
header.frame_id
andreference_frame_id
are "world", it looks like a spatial twist, but that's not what we need here. Maybe we need to clarify that theVelocityStamped
message always represents body twists, but expressed in the frame specified byreference_frame_id
.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.
@azeey @tfoote once you confirm, I would welcome a suggestion or an extra context to make sure I understand and apply this correctly
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 would be nice to include an optional
source
field here to indicate if the simulator should limit itself to spawnables available locally or should include remote sources as well.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 suggest semantics of such a field in more detail?
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, this field makes more sense as an input for the
GetSpawnables
service. I'd imagine a simulator can have multiple sources of spawnables:Both of these sources may have subcategorizations to limit the number spawnables returned as well the depth of searching required.
We could make the field an array of strings and let the simulator define what the values are, or we can make them enums if there is broad agreement on what the enums should be. I lean toward an array of strings.
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.
Looking at this, another cool idea could be to tag
Entity
s with some category label, e.g., "robot" vs. "object" vs. "furniture", etc.This way you can not only filter by name, but also by category -- of course, every simulator interface implementer would tag their own entities with relevant categories.
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 like this idea but it seems to be introducing a new concept of entity tags. Which needs to be reflected in the Entity.msg and other interfaces. Perhaps this could be an extension? This sounds much like a synthetic data feature.
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.
@sea-bass what do you think? Would you like to include labels in the PR still?
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 think it would be useful to label entities with categories (as you said in the
Entity.msg
), and then be able to retrieve them by categories.Should it be done with this initial PR? Not necessarily, but I'd appreciate at least a separate gitissue tracking this request. I certainly use these mechanisms all the time in my simulator.