Skip to content
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

Proof of concept for headless SDF exporting #215

Merged
merged 92 commits into from
Jun 21, 2024
Merged

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Apr 5, 2024

New feature implementation

Implemented feature

Add support for SDF exporting as well as a headless export mode for integration with the legacy pipeline.
For more details refer to the tracking issue#210

#212 and #214 have been locally merged to get a fully working branch

Implementation description

The headless part has been pretty hacked together but hopefully it doesn't affect the rest of the codebase too much since it happens in a completely separate executor / execution path.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
…ort_poc

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
…a/headless_sdf_export_poc

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
…/headless_sdf_export_poc

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova changed the title Luca/headless sdf export poc Proof of concept for headless SDF exporting Apr 5, 2024
@mxgrey mxgrey self-requested a review April 9, 2024 08:10
@mxgrey mxgrey marked this pull request as ready for review May 23, 2024 14:46
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I've finished reviewing the whole PR. I think the vast bulk of it is good to go, but I left some comments about places where we can have more graceful failures (i.e. avoid unwrapping).

I also left some feedback on the new camera poses feature. I think most of my feedback can/should be addressed in a follow up PR so we don't needlessly block this PR for too long, but there are two pieces of feedback that I think are worth addressing before we merge this PR:

  1. I think CameraPoses should be renamed to something like UserCameraPoses to clearly distinguish it from the poses of simulated camera devices.
  2. I'd rather not automatically change the user's camera pose when switching between levels.

rmf_site_editor/src/lib.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/door.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/file_menu.rs Show resolved Hide resolved
rmf_site_editor/src/site/level.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/save.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/sdf_exporter.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/site/sdf_exporter.rs Outdated Show resolved Hide resolved
rmf_site_format/src/camera_poses.rs Outdated Show resolved Hide resolved
rmf_site_format/src/camera_poses.rs Outdated Show resolved Hide resolved
rmf_site_format/src/site.rs Outdated Show resolved Hide resolved
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
mxgrey and others added 6 commits June 5, 2024 16:39
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
* Use site ID to ensure unique file names

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Fix style

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Fix door names for toggle floors plugin

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

---------

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Co-authored-by: Luca Della Vedova <lucadv@intrinsic.ai>
Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

It's awesome to see this in action 🤩

This is a huge step forward towards the next generation. Thanks for getting this to work!

@arjo129
Copy link
Member

arjo129 commented Jun 20, 2024

I tried this it works great. For usability I ran into 2 small issues:

  1. It'd be great if we informed the user that the export was successful.
  2. Generated SDF does require that the right Gazebo environment variables are set. Perhaps we should also export a ros package with a launch file in it? (Since gazebo<=>ros integration is much better, this seems rather sensible)

I probably can open PRs early next week for (2).

@luca-della-vedova
Copy link
Member Author

I tried this it works great. For usability I ran into 2 small issues:

  1. It'd be great if we informed the user that the export was successful.
  2. Generated SDF does require that the right Gazebo environment variables are set. Perhaps we should also export a ros package with a launch file in it? (Since gazebo<=>ros integration is much better now this seems rather sensible)

I probably can open PRs early next week for (2).

  1. There should already be a printout when export is successful / fails, what I wanted to do next is make the app return a non zero exit code when it is run in headless export mode and exporting fails. This will make colcon build fail to build the package and flag to the user what went wrong.
    This is currently blocked on 0.14 being released and a migration to it. Then we will be able to set error codes for AppExit Make AppExit more specific about exit reason. bevyengine/bevy#13022.

  2. Agreed, I think there are two fundamentally different ways to do go at this SDF support, both have pros and cons:

  • Make the export only save the world file / models, depend on external launch files. This makes it pretty much a rmf_building_map_tools replacement and plugs in the current pipeline without any change. It's the current approach but admittedly it is not able to actually generate a package from scratch and has to be part of either a manual step or a "CMakeLists" approach as we have been doing.
  • Make the export generate a whole package, it will make the feature more standalone but then we might have to ask users to commit the whole generated package to version control, and the generated package will still have to be built through colcon build otherwise it won't be launchable.

@mxgrey
Copy link
Collaborator

mxgrey commented Jun 20, 2024

Since this capability is only meant to make the site editor a replacement for the building map tools, I think the current environment variable situation should remain as-is.

If we want to make the simulation pipeline with gazebo easier, that should be its own follow-up discussion.

@mxgrey mxgrey merged commit e764be6 into main Jun 21, 2024
5 checks passed
@mxgrey mxgrey deleted the luca/headless_sdf_export_poc branch June 21, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants