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

Added motion duration to the 'move to pose' service of the camera tracking plugin. #594

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

jrutgeer
Copy link
Contributor

@jrutgeer jrutgeer commented Nov 9, 2023

🎉 New feature

This PR adds the possibility to provide the duration time to the 'move to pose' service of the camera tracking plugin.

If no or negative duration is specified, the duration defaults to the previous value of 0.5 seconds.

@jrutgeer jrutgeer requested a review from jennuine as a code owner November 9, 2023 10:50
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Nov 9, 2023
@azeey
Copy link
Contributor

azeey commented Nov 10, 2023

This needs gazebosim/gz-msgs#408 and since that needs to target main, this should also target main.

@jrutgeer jrutgeer changed the base branch from gz-gui8 to main November 20, 2023 09:46
@jrutgeer
Copy link
Contributor Author

@azeey I retargetted to main. It seems no further steps (similar to these) are necessary as the 'Files changed' show only changes related to this PR?

@ahcorde
Copy link
Contributor

ahcorde commented Nov 20, 2023

This one looks fine

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.

can you fix the conflicts ?

@jrutgeer
Copy link
Contributor Author

There seems to be only one issue, which is due to gazebosim/gz-msgs#408 being required for this to compile?

/github/workspace/src/plugins/camera_tracking/CameraTracking.cc:296:12: error: 'const class gz::msgs::GUICamera' has no member named 'duration'
    296 |   if (_msg.duration() > 0)
        |            ^~~~~~~~

@ahcorde
Copy link
Contributor

ahcorde commented Nov 22, 2023

I was refering to this conflicts

aconflicts

@jrutgeer
Copy link
Contributor Author

I think it should be ok now.

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.

did you see this error ?

    369 |             std::bind(&CameraTrackingPrivate::OnMoveToPoseComplete, this));
        |                        ^~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Johan Rutgeerts <[email protected]>
@jrutgeer
Copy link
Contributor Author

No, I hadn't seen that.

To my defence:
For a regular contributor like me, for whom this isn't part of the day to day job, it is very hard to see the forest for the trees in this whole process:

  • Not clear which branch one should target,
  • No feedback upon change of base branch from gz-gui8 to main that main was changed in the mean time wrt the main in my fork,
  • Resolve requests in the web gui:
    • It wasn't clear to me where that came from (as locally the change compiled correctly),
    • Resolve through the web gui is obviously prone to errors, as you can't compile and check,

So for this last commit (hopefully!), I set up a local source tree targetting main:

  • There are no instructions for this (that I am aware of),
  • So I had taken collection-harmonic.yaml file and changed each version to those mentioned in the CMakeLists.txt files (e.g. gz-common5 to gz-common6),
  • Only to find out gz-common6 branch does not exits, it should just be main,

Obviously in hindsight this all makes sense, but... what a learning curve in order to supply such a simple patch.

Imo. the ROS procedure, where you just work with a local rolling install and provide PR's wrt that, and rolling is always up to date with all latest patches, is much more straight-forward.

I asked about the Gazebo Sim way of working here but I still have no true understanding the best-practice procedure to submit features and bug fixes for Gazebo Sim.

@jennuine
Copy link
Contributor

jennuine commented Dec 1, 2023

@jrutgeer we appreciate your feedback and your contribution!

Here has been my workflow in case it helps, I have a workspace for the distribution I'm working on. If there is a breaking ABI change in any of the libraries, then the feature needs to land in main. The next major release will be Ionic and its collection of libraries can be found here: https://github.com/gazebo-tooling/gazebodistro/blob/master/collection-ionic.yaml

When the next version is released, we also announce the name of the version. For example, when we released Harmonic we announced the release after that will be Ionic. The yaml collection is subject to change when we decide to bump a library version so it's good to be aware of changes and update the new version workspace when possible. In Ionic's case we are bumping most of the libraries (except for gz-tools), which is why they are almost all targeting main. In the future if the gazebo team sees there there needs to be breaking changes in gz-tools then it will be bumped to use main as well.

I hope this can provide a little more clarity. We apologize for the confusion but are doing our best with the time and resources we have allocated. Any contributions are appreciated and welcome 😄

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (352e726) 70.14% compared to head (b26b1e5) 70.08%.

Files Patch % Lines
src/plugins/camera_tracking/CameraTracking.cc 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
- Coverage   70.14%   70.08%   -0.07%     
==========================================
  Files          38       38              
  Lines        5343     5348       +5     
==========================================
  Hits         3748     3748              
- Misses       1595     1600       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jennuine jennuine merged commit 91b50b8 into gazebosim:main Dec 1, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants