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.

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

Adds a message that allows loading environments via a topic #320

Merged
merged 14 commits into from
Aug 4, 2023
Merged

Adds a message that allows loading environments via a topic #320

merged 14 commits into from
Aug 4, 2023

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Dec 20, 2022 •
edited
Loading

🦟 Bug fix

Fixes #

Summary

Required by gazebosim/gz-sim#1842

Signed-off-by: Arjo Chakravarty arjo@openrobotics.org

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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.

@arjo129 arjo129 requested a review from caguero as a code owner December 20, 2022 14:12
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Dec 20, 2022
@arjo129 arjo129 mentioned this pull request Dec 20, 2022
Merged
9 tasks
ahcorde
ahcorde previously requested changes Dec 20, 2022
View reviewed changes
include/gz/msgs/Utility.hh Outdated Show resolved Hide resolved
proto/gz/msgs/data_load_options.proto Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 20, 2022 •
edited
Loading

Codecov Report

Merging #320 (5d3797f) into gz-msgs9 (c7e0323) will increase coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 5d3797f differs from pull request most recent head 97531a0. Consider uploading reports for the commit 97531a0 to get more accurate results

@@             Coverage Diff              @@
##           gz-msgs9     #320      +/-   ##
============================================
+ Coverage     95.41%   95.57%   +0.15%     
============================================
  Files            10       10              
  Lines          1026     1062      +36     
============================================
+ Hits            979     1015      +36     
  Misses           47       47
Files Changed Coverage Δ
src/Utility.cc 99.02% <100.00%> (+0.05%) ⬆️

caguero
caguero reviewed Dec 20, 2022
View reviewed changes
include/gz/msgs/Utility.hh Outdated
@@ -212,6 +220,14 @@ namespace gz
msgs::SphericalCoordinates Convert(
const math::SphericalCoordinates &_coord);

/// \brief Convert a msgs::SphericalCoordinatesType to an
/// gz::math::SphericalCoordinates::CoordinateTpye
/// \param[in] _coord The spherical coordinates to convert
Copy link
Collaborator

Choose a reason for hiding this comment

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

The spherical coordinate type ...

include/gz/msgs/Utility.hh Outdated Show resolved Hide resolved
proto/gz/msgs/data_load_options.proto Outdated Show resolved Hide resolved
src/Utility.cc Show resolved Hide resolved
src/Utility.cc Outdated
case math::SphericalCoordinates::CoordinateType::LOCAL:
return msgs::SphericalCoordinatesType::LOCAL;
case math::SphericalCoordinates::CoordinateType::LOCAL2:
return msgs::SphericalCoordinatesType::LOCAL2;
Copy link
Collaborator

@caguero caguero Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This block returns a warning as we don't return anything if none of the previous conditions are met.

caguero
caguero approved these changes Jan 20, 2023
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Tiny comment, otherwise looks good to me.

proto/gz/msgs/data_load_options.proto Outdated Show resolved Hide resolved
@arjo129
Adds a message that allows loading environments via a topic
f4b43a5 
Required by gazebosim/gz-sim#1842

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
style
56b157a 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Address PR feedback.
303df10 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Style
2475c08 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
coverage
9b39a6d 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
style
cc4ce08 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 force-pushed the arjo/feat/environment_data_messages branch from a89aee2 to cc4ce08 Compare January 25, 2023 03:17
@arjo129
Copy link
Contributor Author

arjo129 commented Jan 25, 2023

I suggest holding off on merging this till gazebosim/gz-sim#1842 is complete.

@arjo129 arjo129 requested a review from ahcorde February 28, 2023 00:27
@arjo129 arjo129 dismissed ahcorde’s stale review February 28, 2023 00:28

Review stale.

ahcorde
ahcorde requested changes Feb 28, 2023
View reviewed changes
proto/gz/msgs/data_load_options.proto Outdated Show resolved Hide resolved
arjo129 and others added 2 commits March 1, 2023 09:18
@arjo129 arjo129 requested a review from ahcorde April 17, 2023 02:34
@arjo129 arjo129 enabled auto-merge April 18, 2023 06:59
mjcarroll
mjcarroll requested changes Apr 26, 2023
View reviewed changes
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.

There are some open comments from Carlos here, if you could address them.

arjo129 added 4 commits May 9, 2023 06:46
@arjo129
Address PR feedback
8c0e400 
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129
Coverage
c1766cf 
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129
Get rid of compiler
101a723 
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129
Style
bc90bd5 
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129 arjo129 requested a review from mjcarroll May 10, 2023 02:38
@arjo129
Copy link
Contributor Author

arjo129 commented May 17, 2023

@mjcarroll I've addressed the issues. It's waiting on a ✔️ from you.

@mabelzhang mabelzhang mentioned this pull request Jun 21, 2023
Open
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@arjo129 arjo129 merged commit 7fc5805 into gz-msgs9 Aug 4, 2023
@arjo129 arjo129 deleted the arjo/feat/environment_data_messages branch August 4, 2023 21:54
@azeey
Copy link
Contributor

azeey commented Aug 25, 2023

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.

This was merged with "Merge commit". Let's remember to use "Squash-Merge" next time.

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

@ahcorde ahcorde ahcorde approved these changes

@caguero caguero caguero approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees
No one assigned
Labels
beta Targeting beta release of upcoming collection 🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arjo129 @azeey @mjcarroll @caguero @ahcorde