Skip to content

Proposal for simulation interfaces #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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 33 commits into from
Mar 21, 2025
Merged

Proposal for simulation interfaces #1

merged 33 commits into from
Mar 21, 2025

Conversation

adamdbrw
Copy link
Contributor

Following ros-infrastructure/rep#410, this PR contains a first version of simulation interfaces.

@adamdbrw
simulation interfaces
5267ea2 
Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw adamdbrw changed the title First (complete) draft of simulation interfaces Proposal for simulation interfaces Dec 17, 2024
peci1
peci1 reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@peci1 peci1 left a 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.

scpeters
scpeters reviewed Dec 18, 2024
View reviewed changes
adamdbrw and others added 6 commits December 19, 2024 15:32
@adamdbrw @peci1 @scpeters
Apply suggestions from code review
0cfd0fd 
Co-authored-by: Martin Pecka <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@adamdbrw
applied review suggestions
d874c23 
Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw
added missing cmake entries
10f59d2 
Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw
stepping service
ea23f59 
Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw
robot_namespace -> namespace
97b79f7 
Signed-off-by: Adam Dąbrowski <[email protected]>

robot_namespace -> namespace

Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw
changed filter semantics to regex
8bae938 
Signed-off-by: Adam Dąbrowski <[email protected]>
azeey
azeey reviewed Dec 21, 2024
View reviewed changes
Copy link
Collaborator

@azeey azeey left a 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.

@adamdbrw adamdbrw requested review from scpeters, peci1 and azeey January 7, 2025 14:13
@adamdbrw
Copy link
Contributor Author

adamdbrw commented Jan 7, 2025

All comments so far have been addressed, please take another look and approve / suggest changes.

DLu
DLu reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@DLu DLu left a 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.

adamdbrw and others added 3 commits January 13, 2025 13:39
@adamdbrw @DLu
Apply suggestions from code review
4d551f1 
Co-authored-by: David V. Lu!! <[email protected]>
@adamdbrw @DLu
Update srv/GetEntityState.srv
95f46d9 
Co-authored-by: David V. Lu!! <[email protected]>
@adamdbrw
Code review improvements:
0169e10 
- 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]>
@adamdbrw
Copy link
Contributor Author

@DLu thank you for the review, I applied most of your comments and responded to remaining 2 with some argumentation.
I am open to break the PR down if needed, and to include collisions at a later stage (once this one is merged)

DLu
DLu reviewed Jan 14, 2025
View reviewed changes
DLu
DLu reviewed Jan 14, 2025
View reviewed changes
azeey
azeey reviewed Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@azeey azeey left a 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.

sea-bass
sea-bass reviewed Jan 16, 2025
View reviewed changes
Copy link
Contributor

@sea-bass sea-bass left a 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!

adamdbrw and others added 2 commits January 20, 2025 10:48
@adamdbrw @DLu @azeey
Apply suggestions from code review
1e92784 
Co-authored-by: David V. Lu!! <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
@adamdbrw
Revision following the review:
9bf757f 
- 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]>
Amronos
Amronos suggested changes Mar 4, 2025
View reviewed changes
@adamdbrw
corrected entities to strings
5f0aefb 
Signed-off-by: Adam Dąbrowski <[email protected]>
DLu
DLu approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@DLu DLu left a 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.

@adamdbrw
Copy link
Contributor Author

adamdbrw commented Mar 10, 2025 •
edited
Loading

It is a busy time for me now but I will do my best to apply all the feedback this week.
Update: on it now

adamdbrw and others added 2 commits March 17, 2025 11:35
@adamdbrw @DLu
Update msg/Bounds.msg
5fe0d30 
Co-authored-by: David V. Lu!! <[email protected]>
@adamdbrw
Applied review
eb6f27e 
- Added SetEntityInfo to be enable setting of items such as tags.
- EntityCategory is now self-contained.
- Updated SimulationFeatures to reflect changes
- Adjusted documentation in a few places.
- Added 2 new sections to the README.

Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw
Copy link
Contributor Author

@Amronos @ayushgnv could you check if your requested changes have been applied as intended and remove the change request / approve if that's the case?

@michalpelka michalpelka mentioned this pull request Mar 17, 2025
Merged
peci1
peci1 approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

I went through the result again and it looks pretty good.

I had some minor comments, but they mostly affect the comments in files, so should not affect the interfaces themselves.

@adamdbrw
Applied review
e98721f 
- Documentation of relevant feature support flag per each interface.
- Enumeration naming changed to TYPE_, CATEGORY_ etc.
- Default to 1 for steps.
- Other minor suggested docs changes.

Signed-off-by: Adam Dąbrowski <[email protected]>
@adamdbrw
Copy link
Contributor Author

@peci1 all applied.
@ayushgnv if you could, please also check the current iteration for your requested changes and comment further or approve.

peci1
peci1 reviewed Mar 18, 2025
View reviewed changes
peci1
peci1 reviewed Mar 18, 2025
View reviewed changes
@adamdbrw
feature naming fix
65a653f 
Signed-off-by: Adam Dąbrowski <[email protected]>
PawelLiberadzki
PawelLiberadzki reviewed Mar 19, 2025
View reviewed changes
Copy link
Contributor

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

Great proposition! I have only one remark about handling a specific case.

@adamdbrw @PawelLiberadzki
Update srv/SetEntityState.srv
4e6867d 
Co-authored-by: Paweł Liberadzki <[email protected]>
@adamdbrw
Copy link
Contributor Author

@mjcarroll @tfoote please let me know if you would like to approve the current version of the PR or any other changes need to happen. Thank you!

PawelLiberadzki
PawelLiberadzki reviewed Mar 20, 2025
View reviewed changes
Copy link
Contributor

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

I am sorry, I introduced this mistake (in comment) with my last suggestion. Here is the correct one. Except that, everything is fine.

@adamdbrw @PawelLiberadzki
Update srv/SetEntityState.srv
2206c58 
Co-authored-by: Paweł Liberadzki <[email protected]>
tfoote
tfoote approved these changes Mar 21, 2025
View reviewed changes
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

I think that this is in good shape to get started with. We should be ready to evolve it as the implementations get filled in and we learn what we overlooked in the design, either too detailed or not fully specified.

@DLu
Copy link
Contributor

DLu commented Mar 21, 2025

ship it

@mjcarroll mjcarroll merged commit 86d4cce into ros-simulation:main Mar 21, 2025
@masterccioli masterccioli mentioned this pull request Apr 22, 2025
Open
This was referenced Apr 30, 2025
Open
Open
@DLu
Copy link
Contributor

DLu commented Apr 30, 2025

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 anyone who wants to debate ROS interfaces even more, I have created two upstream PRs for upstream migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peci1 peci1 peci1 approved these changes

@sea-bass sea-bass sea-bass approved these changes

@PawelLiberadzki PawelLiberadzki PawelLiberadzki left review comments

@Amronos Amronos Amronos approved these changes

@ayushgnv ayushgnv ayushgnv requested changes

@azeey azeey azeey approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@tfoote tfoote tfoote approved these changes

@DLu DLu DLu approved these changes

@michalpelka michalpelka michalpelka approved these changes

@pijaro pijaro pijaro approved these changes

@scpeters scpeters Awaiting requested review from scpeters

@artivis artivis Awaiting requested review from artivis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.