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

Removing ignition #2055

Merged
merged 11 commits into from
Aug 31, 2023
Merged

Removing ignition #2055

merged 11 commits into from
Aug 31, 2023

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Jul 27, 2023

🎉 New feature

Removing "ignition"

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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.

Signed-off-by: Nate Koenig <natekoenig@gmail.com>
@nkoenig nkoenig marked this pull request as draft July 27, 2023 12:22
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jul 27, 2023
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

typo

include/gz/sim/components/Model.hh Outdated Show resolved Hide resolved
Signed-off-by: Nate Koenig <natekoenig@gmail.com>
Signed-off-by: Nate Koenig <natekoenig@gmail.com>
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
Signed-off-by: Nate Koenig <natekoenig@gmail.com>
@nkoenig nkoenig marked this pull request as ready for review August 2, 2023 12:47
@nkoenig nkoenig requested a review from ahcorde August 2, 2023 12:49
Copy link
Contributor

@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.

Looks good. My main concern is if we should remove support for using "ignition" in SDF files. Unlike code, those files are meant to work with multiple versions of Gazebo.

src/SystemLoader.cc Show resolved Hide resolved
src/systems/air_speed/AirSpeed.cc Outdated Show resolved Hide resolved
src/systems/log/LogPlayback.cc Outdated Show resolved Hide resolved
Signed-off-by: Nate Koenig <natekoenig@gmail.com>
Signed-off-by: Nate Koenig <natekoenig@gmail.com>
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #2055 (f8a6f50) into main (5846393) will decrease coverage by 0.01%.
Report is 4 commits behind head on main.
The diff coverage is 33.33%.

❗ Current head f8a6f50 differs from pull request most recent head 654c624. Consider uploading reports for the commit 654c624 to get more accurate results

@@            Coverage Diff             @@
##             main    #2055      +/-   ##
==========================================
- Coverage   65.32%   65.31%   -0.01%     
==========================================
  Files         321      321              
  Lines       30303    30272      -31     
==========================================
- Hits        19795    19772      -23     
+ Misses      10508    10500       -8     
Files Changed Coverage Δ
include/gz/sim/System.hh 100.00% <ø> (ø)
include/gz/sim/Util.hh 100.00% <ø> (ø)
include/gz/sim/components/Model.hh 93.75% <ø> (ø)
src/ServerConfig.cc 90.86% <0.00%> (+2.81%) ⬆️
src/ServerPrivate.cc 82.23% <ø> (ø)
src/SystemLoader.cc 63.86% <ø> (-7.57%) ⬇️
.../plugins/component_inspector/ComponentInspector.cc 5.57% <0.00%> (ø)
src/systems/log/LogPlayback.cc 59.91% <ø> (ø)
src/systems/physics/Physics.cc 72.38% <ø> (+0.12%) ⬆️
.../systems/triggered_publisher/TriggeredPublisher.hh 100.00% <ø> (ø)
... and 2 more

... and 1 file with indirect coverage changes

mjcarroll and others added 4 commits August 28, 2023 13:51
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
@mjcarroll
Copy link
Contributor

This now needs #2087 to build

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 30, 2023

Looks good. My main concern is if we should remove support for using "ignition" in SDF files. Unlike code, those files are meant to work with multiple versions of Gazebo.

I believe we have followed the tick-tock approach where deprecation warnings have been given to users to modify their SDF. Standard practice is to follow the tick with the tock.

@nkoenig nkoenig requested a review from azeey August 30, 2023 12:09
@azeey
Copy link
Contributor

azeey commented Aug 30, 2023

I believe we have followed the tick-tock approach where deprecation warnings have been given to users to modify their SDF. Standard practice is to follow the tick with the tock.

Fair enough.

@iche033
Copy link
Contributor

iche033 commented Aug 31, 2023

changes look goo to me. Just need to re-run CI when it's ready.

@mjcarroll
Copy link
Contributor

Now blocked on gazebosim/gz-msgs#380 and #2103

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 31, 2023

Now blocked on gazebosim/gz-msgs#380 and #2103

What are you doing to me?

@mjcarroll
Copy link
Contributor

What are you doing to me?

Jumping on the Windows grenade for you 🙃

@mjcarroll
Copy link
Contributor

It's not actually blocked at this point, Windows CI isn't going to pass until those two PRs are in. One for gz-msgs generation stuff, one for disabling pybind on windows which hadn't been forward ported yet.

I'm good with merging with those caveats.

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 31, 2023

A windows grenade seems like the most ineffectual grenade ever. Let me know when the grenade is done updating.

I'll wait...

@mjcarroll
Copy link
Contributor

CI Except Windows looks okay

@mjcarroll mjcarroll merged commit 6c9c899 into main Aug 31, 2023
4 of 8 checks passed
@mjcarroll mjcarroll deleted the nkoenig/remove-ign branch August 31, 2023 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants