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

Update to Ignition Edifice #8

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Conversation

luca-della-vedova
Copy link
Member

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

New feature implementation

Implemented feature

Update dependencies to Ignition Edifice. Requires rmf_demos #22 and rmf_traffic_editor #330. Fixes #7 . Will also require updating the docs under rmf_demos once they are merged

Implementation description

Version bump of all the libraries that have been updated in Ignition Edifice.
Sdformat is now a lot less tolerant on deviations from the specification (hence the required PRs). Did some quick testing on scenarios with various features (robots, doors, delivery requests, crowdsim) and there seem to be no regressions.
To test install Ignition Edifice
sudo apt install ignition-edifice
and do a clean build of the workspace.

Signed-off-by: Luca Della Vedova <luca@openrobotics.org>
@youliangtan
Copy link
Member

I briefly tested this change with the different demo worlds. Looks fine to me. 👍

One concern might be regarding the rosdep installation for ignition-edifice. Will need to test the full installation pipeline in a clean workspace and env, to make sure all relevant ign dependencies are installed. Also, as stated, will need to update the CI in rmf_demos.

@luca-della-vedova
Copy link
Member Author

I briefly tested this change with the different demo worlds. Looks fine to me. +1

One concern might be regarding the rosdep installation for ignition-edifice. Will need to test the full installation pipeline in a clean workspace and env, to make sure all relevant ign dependencies are installed. Also, as stated, will need to update the CI in rmf_demos.

Yea sadly ignition-edifice is not in rosdep (same as ignition-dome) so it has to be installed manually. This will probably be fixed in Galactic / Fortress but it's still at least a few months away and will have to be installed manually for the time being.

@youliangtan
Copy link
Member

Yikes, In this case we will need indicate sudo apt install ignition-edifice` in CI and installation.

@luca-della-vedova
Copy link
Member Author

Yikes, In this case we will need indicate sudo apt install ignition-edifice` in CI and installation.

Yea I was waiting for the docs PR to be merged, we will have to update dome to edifice in both CI and docs under rmf_demos.
I expect the rmf_demos PR to break CI and become green once this is merged

Copy link
Member

@youliangtan youliangtan left a comment

Choose a reason for hiding this comment

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

LGTM

@luca-della-vedova luca-della-vedova merged commit 3ff9796 into main Apr 6, 2021
@luca-della-vedova luca-della-vedova deleted the feature/ignition_edifice branch April 6, 2021 06:08
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.

Ignition Edifice migration
2 participants