-
Notifications
You must be signed in to change notification settings - Fork 14
Add array spawn service. #20
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Michał Pełka <[email protected]>
jhanca-robotecai
left a comment
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.
Please bump the version of the package.
peci1
left a comment
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.
Hmm, I don't like this approach of making each field an array. However, given the fact that a service request (IMO) cannot be reused elsewhere, it is probably the best possible solution without going awkward (like specifying a standalone message type for the request part).
Please, see the inline comments.
| @@ -0,0 +1,46 @@ | |||
| # Spawn entities (a robot, other object) by name or URI | |||
| # Support for this interface is indicated through the SPAWNING_ARRAY value in GetSimulationFeatures. | |||
|
|
|||
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.
Add a comment about the array lengths. Do all of them need to be filled? Or are empty arrays supported? And what about unfinished arrays? (i.e. names[] has 5 elems but allow_renaming[] only 3).
| uint8 INVALID_POSE = 109 # initial_pose is invalid, such as when the quaternion is invalid or position | ||
| # exceeds simulator world bounds. | ||
|
|
||
| Result results[] |
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.
There should also be Result result as the overall result to keep the interface consistent. This should come with good specification on what should be put in it when only partial success is achieved. Maybe only restrict the error codes to OK (all succeeded), SOME_FAILED, FAILED?
| --- | ||
|
|
||
| # Additional result.result_code values for this service. Check result.error_message for further details. | ||
| uint8 NAME_NOT_UNIQUE = 101 # Given name is already taken by entity and allow_renaming is false. |
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 it needed to repeat those here? Wouldn't it be better to link to SpawnEntity.srv?
I guess version bumps are done when releasing the package, so I wouldn't expect the bump to be a part of this PR. |
Simulation interfaces can be used to populate scene with assets or props, depending on the scenario.
I would like to propose the way to ask underlying simulator to spawn multiple objects.
Motivation: