Added trigger to BoundingBoxCamera #322

vvasco · 2023-02-16T18:10:33Z

Signed-off-by: Valentina Vasco [email protected]

🎉 New feature

Closes #321

Summary

This adds the possibility to trigger a BoundingBoxCamera.

Test it

Checkout the branch https://github.com/vvasco/gz-sensors/tree/triggered_bounding_box_camera and run the INTEGRATION_triggered_boundingbox_camera

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: Valentina Vasco <[email protected]>

ahcorde · 2023-02-17T13:16:29Z

include/gz/sensors/CameraSensor.hh

@@ -22,6 +22,7 @@
 #include <string>

 #include <gz/msgs/image.pb.h>
+#include <gz/msgs/boolean.pb.h>


alphabetize

Addressed in d7bc027

ahcorde · 2023-02-17T13:17:00Z

src/BoundingBoxCameraSensor.cc

+  public: bool isTriggeredCamera = false;
+
+  /// \brief True if camera has been triggered by a topic
+  public: bool isTriggered = false;
+
+  /// \brief Topic for camera trigger
+  public: std::string triggerTopic = "";


Suggested change

public: bool isTriggeredCamera = false;

/// \brief True if camera has been triggered by a topic

public: bool isTriggered = false;

/// \brief Topic for camera trigger

public: std::string triggerTopic = "";

public: bool isTriggeredCamera{false};

/// \brief True if camera has been triggered by a topic

public: bool isTriggered{false};

/// \brief Topic for camera trigger

public: std::string triggerTopic{""};

Addressed in d7bc027

ahcorde · 2023-02-17T13:17:27Z

src/BoundingBoxCameraSensor.cc

+
+      if (this->dataPtr->triggerTopic.empty())
+      {
+        gzerr << "Invalid trigger topic name" << std::endl;


Suggested change

gzerr << "Invalid trigger topic name" << std::endl;

gzerr << "Invalid trigger topic name [" << this->dataPtr->triggerTopic << "]" << std::endl;

Addressed in d7bc027

ahcorde · 2023-02-17T13:17:39Z

src/BoundingBoxCameraSensor.cc

+    gzdbg << "Camera trigger messages for [" << this->Name() << "] subscribed"
+           << " on [" << this->dataPtr->triggerTopic << "]" << std::endl;


Suggested change

gzdbg << "Camera trigger messages for [" << this->Name() << "] subscribed"

<< " on [" << this->dataPtr->triggerTopic << "]" << std::endl;

gzdbg << "Camera trigger messages for [" << this->Name() << "] subscribed"

<< " on [" << this->dataPtr->triggerTopic << "]" << std::endl;

Addressed in d7bc027

ahcorde · 2023-02-17T13:18:00Z

test/integration/triggered_boundingbox_camera.cc

@@ -0,0 +1,228 @@
+/*
+ * Copyright (C) 2022 Open Source Robotics Foundation


Suggested change

* Copyright (C) 2022 Open Source Robotics Foundation

* Copyright (C) 2023 Open Source Robotics Foundation

Addressed in d7bc027

Signed-off-by: Valentina Vasco <[email protected]>

iche033

Thanks for the contribution!

There are some minor lint issues picked up here: https://github.com/gazebosim/gz-sensors/actions/runs/4204430572/jobs/7404649199

Can you address these issues.

I also made a comment about ABI compatibility.

iche033 · 2023-02-23T19:56:06Z

include/gz/sensors/CameraSensor.hh

@@ -175,6 +176,10 @@ namespace gz
      /// \param[in] _scene Pointer to the new scene.
      private: void OnSceneChange(gz::rendering::ScenePtr /*_scene*/);

+      /// \brief Callback for triggered subscription
+      /// \param[in] _msg Boolean message
+      private: virtual void OnTrigger(const gz::msgs::Boolean &/*_msg*/);


looks like this changed the v-table layout maybe ABI-incompatible. We'll need to make this function non-virtual in gz-sensors7. This can be made virtual later when we forward port to main where we can break ABI.

Done in b7ecaa5

Signed-off-by: Valentina Vasco <[email protected]>

iche033

looks good to me. Tested with the bounding box camera example world in gz-sim and works as expected

Signed-off-by: Ian Chen <[email protected]>

codecov · 2023-02-24T23:40:12Z

Codecov Report

Merging #322 (5af125c) into gz-sensors7 (9f7fb14) will increase coverage by 0.16%.
The diff coverage is 76.00%.

❗ Current head 5af125c differs from pull request most recent head 544534c. Consider uploading reports for the commit 544534c to get more accurate results

@@               Coverage Diff               @@
##           gz-sensors7     #322      +/-   ##
===============================================
+ Coverage        69.80%   69.97%   +0.16%     
===============================================
  Files               36       36              
  Lines             3898     3920      +22     
===============================================
+ Hits              2721     2743      +22     
  Misses            1177     1177

Impacted Files	Coverage Δ
src/BoundingBoxCameraSensor.cc	`60.90% <72.72%> (+2.79%)`	⬆️
src/CameraSensor.cc	`80.42% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

comments addressed

Crola1702 · 2023-02-28T15:24:39Z

Hey @iche033 @vvasco.

triggered_boundingbox_camera.cc test is failing. I'm not sure why 😕.

Check #328

vvasco added 3 commits February 16, 2023 18:03

added trigger to BoundingBoxCamera

e7cebc1

Signed-off-by: Valentina Vasco <[email protected]>

added test for triggered camera bounding box

bc34a77

Signed-off-by: Valentina Vasco <[email protected]>

ovveriding onTrigger callback

da8c7fe

Signed-off-by: Valentina Vasco <[email protected]>

vvasco requested a review from iche033 as a code owner February 16, 2023 18:10

github-actions bot added the 🌱 garden Ignition Garden label Feb 16, 2023

mjcarroll self-requested a review February 16, 2023 18:11

ahcorde previously requested changes Feb 17, 2023

View reviewed changes

addressed reviews comments

d7bc027

Signed-off-by: Valentina Vasco <[email protected]>

iche033 reviewed Feb 23, 2023

View reviewed changes

fixed cpplint and removed virtual

b7ecaa5

Signed-off-by: Valentina Vasco <[email protected]>

iche033 approved these changes Feb 24, 2023

View reviewed changes

lint

544534c

Signed-off-by: Ian Chen <[email protected]>

iche033 enabled auto-merge (squash) February 24, 2023 23:45

iche033 merged commit 0bab350 into gazebosim:gz-sensors7 Feb 25, 2023

Crola1702 mentioned this pull request Feb 28, 2023

🧑‍🌾 triggered_boundingbox_camera integration test failing in gz-sensors7 #328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added trigger to BoundingBoxCamera #322

Added trigger to BoundingBoxCamera #322

vvasco commented Feb 16, 2023 •

edited

Loading

ahcorde Feb 17, 2023

vvasco Feb 17, 2023

ahcorde Feb 17, 2023

vvasco Feb 17, 2023

ahcorde Feb 17, 2023

vvasco Feb 17, 2023

ahcorde Feb 17, 2023

vvasco Feb 17, 2023

ahcorde Feb 17, 2023

vvasco Feb 17, 2023

iche033 left a comment

iche033 Feb 23, 2023

vvasco Feb 24, 2023

iche033 left a comment

codecov bot commented Feb 24, 2023

Crola1702 commented Feb 28, 2023

	gzerr << "Invalid trigger topic name" << std::endl;
	gzerr << "Invalid trigger topic name [" << this->dataPtr->triggerTopic << "]" << std::endl;

		gzdbg << "Camera trigger messages for [" << this->Name() << "] subscribed"
		<< " on [" << this->dataPtr->triggerTopic << "]" << std::endl;

		@@ -0,0 +1,228 @@
		/*
		* Copyright (C) 2022 Open Source Robotics Foundation

	* Copyright (C) 2022 Open Source Robotics Foundation
	* Copyright (C) 2023 Open Source Robotics Foundation

Added trigger to BoundingBoxCamera #322

Added trigger to BoundingBoxCamera #322

Conversation

vvasco commented Feb 16, 2023 • edited Loading

🎉 New feature

Summary

Test it

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2023

Codecov Report

Crola1702 commented Feb 28, 2023

vvasco commented Feb 16, 2023 •

edited

Loading