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

Optional orientation when spawning entity using spherical coordinates #1263

Merged
merged 8 commits into from
Jan 14, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Dec 21, 2021

🎉 New feature

Summary

#1008 added the ability of spawning an entity at a given latitude / longitude. But often, we also want to give the entity a custom 3D orientation (not just heading). With this PR, users can pass a pose.orientation that will be used in combination with lat / lon.

Test it

See the added test, or try calling the service with a custom orientation.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina requested a review from mjcarroll January 10, 2022 19:41
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Tests seem to pass and the logic looks good to me.

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM with merge from main and green CI

@chapulina
Copy link
Contributor Author

There was a sneaky bug detected by the touch plugin test, which was fixed in 10de2d1

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #1263 (6d661ff) into ign-gazebo6 (ae36d83) will increase coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo6    #1263   +/-   ##
============================================
  Coverage        62.24%   62.24%           
============================================
  Files              278      278           
  Lines            23135    23143    +8     
============================================
+ Hits             14400    14406    +6     
- Misses            8735     8737    +2     
Impacted Files Coverage Δ
src/systems/user_commands/UserCommands.cc 54.27% <85.71%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae36d83...6d661ff. Read the comment docs.

@chapulina chapulina merged commit f40806e into ign-gazebo6 Jan 14, 2022
@chapulina chapulina deleted the chapulina/6/spherical_orientation branch January 14, 2022 19:10
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants