Visualze Frustum #2707

BA-Utkarsh · 2024-12-26T11:17:28Z

🎉 New feature

Summary

This PR mainly adds the visualization of Frustum.
We could see it was present in gazebo classic and from gazebo garden onwards the plugin/feature is not available.

Test it

$ Build gazebo from source.
$ . install/setup.sh
$ gz sim examples/worlds/visualize_frustum.sdf

Test Ref images,

Play the simulation.
Select the topic from scroll down.
Refresh it to get the "logical_camera/frustum" topic.
Subcriibed to "logical_camera/frustum".
Final output

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
All tests passed
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Supporting PRs,

ahcorde · 2024-12-26T17:01:41Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

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


Suggested change

* Copyright (C) 2020 Open Source Robotics Foundation

* Copyright (C) 2024 Open Source Robotics Foundation

@ahcorde thank you for reviewing.
Before proceeding, I have a small query.
Do you want me to,

use sign off and commit suggestion for each review comment?

Or, do all the changes in my local branch and push it?

Additionally, for sign off, do I need to do to all the commits as I missed it in my first version, or commits from now onwards it's ok.
I'm bit confused.

ahcorde · 2024-12-26T17:03:12Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+/////////////////////////////////////////////////
+void VisualizeFrustum::OnRefresh()
+{
+  gzmsg << "Refreshing topic list for Logical Camera Sensor messages." << std::endl;


remove or maybe debug

ahcorde · 2024-12-26T17:03:33Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.hh

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


Suggested change

* Copyright (C) 2020 Open Source Robotics Foundation

* Copyright (C) 2024 Open Source Robotics Foundation

ahcorde · 2024-12-26T17:03:51Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.hh

+                        EntityComponentManager &_ecm) override;
+
+    /// \brief Callback function to get data from the message
+    /// \param[in]_msg FrustumSensor message


Suggested change

/// \param[in]_msg FrustumSensor message

/// \param[in] _msg FrustumSensor message

ahcorde · 2024-12-26T17:04:06Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.qml

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


Suggested change

* Copyright (C) 2020 Open Source Robotics Foundation

* Copyright (C) 2024 Open Source Robotics Foundation

Signed-off-by: Utkarsh <[email protected]>

BA-Utkarsh · 2024-12-27T09:21:09Z

@ahcorde , thank you for review. I have addressed all the review comments.

arjo129 · 2025-01-09T05:20:39Z

Thanks for the contribution. There are still stray spaces in the code. Do you mind fixing them: https://github.com/gazebosim/gz-sim/actions/runs/12514253900/job/35270143157?pr=2707

Also is there a chance that we can visualize the frustum without the need for pressing play?

Signed-off-by: Utkarsh <[email protected]>

ahcorde · 2025-01-10T08:55:01Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+    /// \brief Mutex for variable mutated by the checkbox and spinboxes
+    /// callbacks.
+    /// The variables are: msg
+    public: std::mutex serviceMutex;


include <mutex>

ahcorde · 2025-01-10T08:55:39Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+  if (!engine)
+  {
+    gzerr << "Internal error: failed to load engine [" << engineName
+          << "]. VisualizeFrustum plugin won't work." << std::endl;


include <ostream>

ahcorde · 2025-01-10T08:56:34Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+      {
+        auto parent = baseEntity;
+        bool success = false;
+        for (size_t i = 0u; i < frustumURIVec.size()-1; i++)


include <cstddef>

ahcorde · 2025-01-10T08:58:49Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+  std::lock_guard<std::mutex> lock(this->dataPtr->serviceMutex);
+  if (this->dataPtr->initialized)
+  {
+    this->dataPtr->msg = std::move(_msg);


include <utility>

It looks like this->dataPtr->msg is redundant member variable.
It's used in this method only, to initialize some other fields and invoking Set(Near|Far|FOV|AspectRatio) methods
Maybe it could be removed?

ahcorde · 2025-01-10T08:59:08Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.hh

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


Suggested change

* Copyright (C) 2024 Open Source Robotics Foundation

* Copyright (C) 2025 Open Source Robotics Foundation

ahcorde · 2025-01-10T08:59:15Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

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


Suggested change

* Copyright (C) 2024 Open Source Robotics Foundation

* Copyright (C) 2025 Open Source Robotics Foundation

ahcorde · 2025-01-10T08:59:37Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.qml

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


Suggested change

* Copyright (C) 2024 Open Source Robotics Foundation

* Copyright (C) 2025 Open Source Robotics Foundation

Signed-off-by: Utkarsh <[email protected]>

ntfshard · 2025-01-12T16:34:14Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+{
+  std::lock_guard<std::mutex> lock(this->dataPtr->serviceMutex);
+  this->dataPtr->frustum->SetVisible(_value);
+  gzerr << "Frustum Visual Display " << ((_value) ? "ON." : "OFF.")


is it really error level of logging?

ntfshard · 2025-01-12T16:37:21Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+            }
+            std::string childname = std::string(
+                            comp->Data());
+            if (nextstring.compare(childname) == 0)


nextstring and childname are std::strings, why not use == for it?
Like: if (nextstring == childname)

ntfshard · 2025-01-12T16:40:06Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+  {
+    std::vector<transport::MessagePublisher> publishers;
+    this->dataPtr->node.TopicInfo(topic, publishers);
+    for (auto pub : publishers)


do you really need a pub by copy? Just curious

ntfshard · 2025-01-12T16:41:14Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+  // Get updated list
+  std::vector<std::string> allTopics;
+  this->dataPtr->node.TopicList(allTopics);
+  for (auto topic : allTopics)


maybe const reference could be enough for topic?

ntfshard · 2025-01-12T16:43:38Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+  {
+    auto frustumURIVec = common::split(common::trimmed(
+                  this->dataPtr->frustumString), "::");
+    if (frustumURIVec.size() > 0)


size() is already unsigned, maybe !frustumURIVec.empty() is more c++-like
(and for some containers size() operation can be quite costly)

ntfshard · 2025-01-12T16:45:13Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+          bool foundChild = false;
+          for (auto child : children)
+          {
+            std::string nextstring = frustumURIVec[i+1];


it can be const reference on a left side of expression, you don't need a full string copy here AFAIS
const std::string &nextstring = frustumURIVec[i+1]; or use auto instead of std::string

ntfshard · 2025-01-12T16:54:25Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+
+    this->dataPtr->visualDirty = true;
+
+    for (auto data_values : this->dataPtr->msg.header().data())


const reference should be fine here I suppose

ntfshard · 2025-01-12T16:55:23Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+        if (this->dataPtr->frustumString.compare(
+                common::trimmed(data_values.value(0))) != 0)


seems it's comparing of 2 strings, seems better to use != here

ntfshard · 2025-01-12T17:01:06Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+      }
+    }
+  }
+  if (this->dataPtr->topicList.size() > 0)


same comment as above about size() and empty

ntfshard · 2025-01-12T17:15:19Z

src/gui/plugins/visualize_frustum/VisualizeFrustum.cc

+  std::lock_guard<std::mutex> lock(this->dataPtr->serviceMutex);
+  if (this->dataPtr->initialized)
+  {
+    this->dataPtr->msg = std::move(_msg);


It looks like this->dataPtr->msg is redundant member variable.
It's used in this method only, to initialize some other fields and invoking Set(Near|Far|FOV|AspectRatio) methods
Maybe it could be removed?

BA-Utkarsh · 2025-01-13T14:50:11Z

@ntfshard , \cc: @ahcorde
All the changes are done from the reference of https://github.com/gazebosim/gz-sim/blob/gz-sim9/src/gui/plugins/visualize_lidar/
I have not modified the code (apart from frustum part). visualize_lidar are already in the upstream repo. Thus taking the same reference.

In my opinion, the changes are not needed for now. @ahcorde , please let me know your views on the same..

BA-Utkarsh requested a review from mjcarroll as a code owner December 26, 2024 11:17

github-actions bot added 🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty labels Dec 26, 2024

This was referenced Dec 26, 2024

Visualze Frustum gazebosim/gz-sensors#491

Merged

Visualize Frustum gazebosim/gz-rendering#1095

Merged

Visualization of frustum gazebosim/gz-sensors#492

Closed

Visualization of frustum gazebosim/gz-rendering#1096

Closed

Visualization of frustum #2688

Open

ahcorde requested changes Dec 26, 2024

View reviewed changes

Visualze Frustum

cbabb37

Signed-off-by: Utkarsh <[email protected]>

BA-Utkarsh force-pushed the gz-sim9-v1 branch from 32d5e9b to cbabb37 Compare December 27, 2024 08:05

Update visualize_frustum plugins

f87b563

Signed-off-by: Utkarsh <[email protected]>

strip trailing whitespace

2393539

Signed-off-by: Utkarsh <[email protected]>

BA-Utkarsh requested a review from ahcorde January 10, 2025 07:44

ahcorde requested changes Jan 10, 2025

View reviewed changes

Code formatting and Copyright updates

342e5ef

Signed-off-by: Utkarsh <[email protected]>

BA-Utkarsh requested a review from ahcorde January 11, 2025 10:23

ntfshard reviewed Jan 12, 2025

View reviewed changes

iche033 added the needs upstream release Blocked by a release of an upstream library label Jan 17, 2025

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

	* Copyright (C) 2020 Open Source Robotics Foundation
	* Copyright (C) 2024 Open Source Robotics Foundation

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

	/// \param[in]_msg FrustumSensor message
	/// \param[in] _msg FrustumSensor message

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

	* Copyright (C) 2024 Open Source Robotics Foundation
	* Copyright (C) 2025 Open Source Robotics Foundation

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


		this->dataPtr->visualDirty = true;

		for (auto data_values : this->dataPtr->msg.header().data())

		if (this->dataPtr->frustumString.compare(
		common::trimmed(data_values.value(0))) != 0)

Visualze Frustum #2707

Are you sure you want to change the base?

Visualze Frustum #2707

Conversation

BA-Utkarsh commented Dec 26, 2024 • edited Loading

🎉 New feature

Summary

Test it

Test Ref images,

Checklist

Supporting PRs,

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

Choose a reason for hiding this comment

BA-Utkarsh commented Dec 27, 2024

arjo129 commented Jan 9, 2025 • edited Loading

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

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

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

BA-Utkarsh commented Jan 13, 2025 • edited Loading

BA-Utkarsh commented Dec 26, 2024 •

edited

Loading

arjo129 commented Jan 9, 2025 •

edited

Loading

BA-Utkarsh commented Jan 13, 2025 •

edited

Loading