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

Camera: ported GstCameraPlugin from PX4 gazebo classic #97

Merged
merged 6 commits into from
Jun 24, 2024

Conversation

trexgerratt
Copy link

I'm sure it could be improved, but here's a port of the gazebo classic GstCameraPlugin into Gazebo harmonic. This was heavily based on the plugin from the PX4 gazebo repo (https://github.com/PX4/PX4-SITL_gazebo-classic/blob/main/src/gazebo_gst_camera_plugin.cpp) and the existing camera zoom plugin.

I ran into problem after problem trying to use the new gazebo api for taking frames from the gazebo server and ended up subscribing to the gazebo image topic instead. It works really well, but I'd love feedback or improvements for this approach as I know it's not super efficient. Hoping this PR gets the ball rolling in the right direction.

@srmainwaring srmainwaring added the enhancement New feature or request label Jun 18, 2024
@srmainwaring
Copy link
Collaborator

@trexgerratt - this is a great addition. I've got a few change requests, mostly formatting and conventions for the sdf elements. I think we also need an example and a section in the README explaining the additional dependencies and use.

Here's the video from the iris streamed to QCG running on macOS - very nice!

gz-sim-gstreamer

Copy link
Collaborator

@srmainwaring srmainwaring left a comment

Choose a reason for hiding this comment

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

A number of code style changes and removal of environment variables.

The plugin needs a way to generate an image topic consistent with the enclosing sensor so that the plugin may be used more generally.

I've made most of the initial changes here: trexgerratt#2. You can either rebase merge or I can force push. It includes an update to the github workflow to ensure the CI build passes

src/GstCameraPlugin.cc Outdated Show resolved Hide resolved
src/GstCameraPlugin.cc Outdated Show resolved Hide resolved
src/GstCameraPlugin.cc Show resolved Hide resolved
src/GstCameraPlugin.cc Outdated Show resolved Hide resolved
models/gimbal_small_3d/model.sdf Outdated Show resolved Hide resolved
models/gimbal_small_3d/model.sdf Outdated Show resolved Hide resolved
@srmainwaring
Copy link
Collaborator

srmainwaring commented Jun 24, 2024

@trexgerratt - could you please rebase-merge trexgerratt#2 into your PR branch, I can then merge this PR.

For some reason I cannot push commits to your branch, although you have allowed maintainers to edit this PR.

Edit worked on another attempt - so will merge on CI pass.

- Update class and parameter documentation.
- Apply Gazebo code style (mainly removing cuddled braces).
- Use snake case for SDF elements.
- Remove the environment variable defaults.
- Require mandatory fields are set in SDF.
- Use sensor topic name as default for image topic.
- Use sensor topic name as prefix for enable streaming topic.
- Update class doc string.
- Update gz message strings.
- Ensure streaming stops / starts when requested.

Signed-off-by: Rhys Mainwaring <[email protected]>
@srmainwaring srmainwaring self-requested a review June 24, 2024 11:56
@srmainwaring srmainwaring merged commit d5cfe36 into ArduPilot:main Jun 24, 2024
3 checks passed
This was referenced Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants