gazebosim / gz-gui

Builds on top of Qt to provide widgets which are useful when developing robotics applications, such as a 3D view, plots, dashboard, etc, and can be used together in a convenient unified interface.
https://gazebosim.org
Apache License 2.0
74 stars 43 forks source link

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

Closed jrutgeer closed 10 months ago

jrutgeer commented 10 months ago

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

azeey commented 10 months ago

This needs https://github.com/gazebosim/gz-msgs/pull/408 and since that needs to target main, this should also target main.

jrutgeer commented 10 months ago

@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 commented 10 months ago

This one looks fine

jrutgeer commented 10 months ago

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 commented 10 months ago

I was refering to this conflicts

aconflicts

jrutgeer commented 10 months ago

I think it should be ok now.

jrutgeer commented 10 months ago

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:

So for this last commit (hopefully!), I set up a local source tree targetting 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 commented 10 months ago

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

codecov[bot] commented 10 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.