TransportSceneManager: Prevent freeze when inserted from menu

src/plugins/transport_scene_manager/TransportSceneManager.cc

@@ -19,8 +19,11 @@
 #include <map>
 #include <sstream>
 #include <string>
+#include <thread>
 #include <vector>
 
+#include <QQmlProperty>
+
 #include <ignition/common/Console.hh>
 #include <ignition/common/MeshManager.hh>
 #include <ignition/math/Pose3.hh>
@@ -44,6 +47,7 @@
 #endif
 
 #include <ignition/transport/Node.hh>
+#include <ignition/transport/TopicUtils.hh>
 
 #include "ignition/gui/Application.hh"
 #include "ignition/gui/Conversions.hh"
@@ -124,16 +128,16 @@ class ignition::gui::plugins::TransportSceneManagerPrivate
   public: void DeleteEntity(const unsigned int _entity);
 
   //// \brief Ign-transport scene service name
-  public: std::string service;
+  public: std::string service{"scene"};
 
   //// \brief Ign-transport pose topic name
-  public: std::string poseTopic;
+  public: std::string poseTopic{"pose"};
 
   //// \brief Ign-transport deletion topic name
-  public: std::string deletionTopic;
+  public: std::string deletionTopic{"delete"};
 
   //// \brief Ign-transport scene topic name
-  public: std::string sceneTopic;
+  public: std::string sceneTopic{"scene"};
 
   //// \brief Pointer to the rendering scene
   public: rendering::ScenePtr scene{nullptr};
@@ -165,6 +169,9 @@ class ignition::gui::plugins::TransportSceneManagerPrivate
   /// \brief Transport node for making service request and subscribing to
   /// pose topic
   public: ignition::transport::Node node;
+
+  /// \brief Thread to wait for transport initialization
+  public: std::thread initializeTransport;
 };
 
 using namespace ignition;
@@ -180,6 +187,8 @@ TransportSceneManager::TransportSceneManager()
 /////////////////////////////////////////////////
 TransportSceneManager::~TransportSceneManager()
 {
+  if (this->dataPtr->initializeTransport.joinable())
+    this->dataPtr->initializeTransport.join();
 }
 
 /////////////////////////////////////////////////
@@ -194,29 +203,56 @@ void TransportSceneManager::LoadConfig(const tinyxml2::XMLElement *_pluginElem)
     auto elem = _pluginElem->FirstChildElement("service");
     if (nullptr != elem && nullptr != elem->GetText())
     {
-      this->dataPtr->service = elem->GetText();
+      this->dataPtr->service =
+          transport::TopicUtils::AsValidTopic(elem->GetText());
     }
 
     elem = _pluginElem->FirstChildElement("pose_topic");
     if (nullptr != elem && nullptr != elem->GetText())
     {
-      this->dataPtr->poseTopic = elem->GetText();
+      this->dataPtr->poseTopic =
+          transport::TopicUtils::AsValidTopic(elem->GetText());
     }
 
     elem = _pluginElem->FirstChildElement("deletion_topic");
     if (nullptr != elem && nullptr != elem->GetText())
     {
-      this->dataPtr->deletionTopic = elem->GetText();
+      this->dataPtr->deletionTopic =
+          transport::TopicUtils::AsValidTopic(elem->GetText());
     }
 
     elem = _pluginElem->FirstChildElement("scene_topic");
     if (nullptr != elem && nullptr != elem->GetText())
     {
-      this->dataPtr->sceneTopic = elem->GetText();
+      this->dataPtr->sceneTopic =
+          transport::TopicUtils::AsValidTopic(elem->GetText());
     }
   }
 
-  App()->findChild<MainWindow *>()->installEventFilter(this);
+  QQmlProperty::write(this->PluginItem(), "service",
+      QString::fromStdString(this->dataPtr->service));
+  QQmlProperty::write(this->PluginItem(), "poseTopic",
+      QString::fromStdString(this->dataPtr->poseTopic));
+  QQmlProperty::write(this->PluginItem(), "deletionTopic",
+      QString::fromStdString(this->dataPtr->deletionTopic));
+  QQmlProperty::write(this->PluginItem(), "sceneTopic",
+      QString::fromStdString(this->dataPtr->sceneTopic));
+
+  if (this->dataPtr->service.empty() ||
+      this->dataPtr->poseTopic.empty() ||
+      this->dataPtr->deletionTopic.empty() ||
+      this->dataPtr->sceneTopic.empty())
+  {
+    ignerr << "One or more transport parameters invalid:" << std::endl
+        << "  * <service>: " << this->dataPtr->service << std::endl
+        << "  * <pose_topic>: " << this->dataPtr->poseTopic << std::endl
+        << "  * <deletion_topic>: " << this->dataPtr->deletionTopic << std::endl
+        << "  * <scene_topic>: " << this->dataPtr->sceneTopic << std::endl;
+  }
+  else
+  {
+    App()->findChild<MainWindow *>()->installEventFilter(this);
+  }
 }
 
 /////////////////////////////////////////////////
@@ -288,13 +324,14 @@ void TransportSceneManagerPrivate::Request()
     if (publishers.size() > 0)
       break;
     std::this_thread::sleep_for(sleepDuration);
-    igndbg << "Waiting for service " << this->service << "\n";
+    igndbg << "Waiting for service [" << this->service << "]\n";
   }
 
   if (publishers.empty() || !this->node.Request(this->service,
       &TransportSceneManagerPrivate::OnSceneSrvMsg, this))
   {
-    ignerr << "Error making service request to " << this->service << std::endl;
+    ignerr << "Error making service request to [" << this->service << "]"
+           << std::endl;
   }
 }
 
@@ -334,7 +371,8 @@ void TransportSceneManagerPrivate::OnRender()
     if (nullptr == this->scene)
       return;
 
-    this->InitializeTransport();
+    this->initializeTransport = std::thread(
+        &TransportSceneManagerPrivate::InitializeTransport, this);
   }
 
   std::lock_guard<std::mutex> lock(this->msgMutex);

src/plugins/transport_scene_manager/TransportSceneManager.hh

@@ -36,10 +36,13 @@ namespace plugins
   /// ## Configuration
   ///
   /// * \<service\> : Name of service where this system will request a scene
-  ///                 message.
+  ///                 message. Optional, defaults to "/scene".
   /// * \<pose_topic\> : Name of topic to subscribe to receive pose updates.
+  ///                    Optional, defaults to "/pose".
   /// * \<deletion_topic\> : Name of topic to request entity deletions.
-  /// * \<scene_topic\> : Name of topic to receive scene updates.
+  ///                        Optional, defaults to "/delete".
+  /// * \<scene_topic\> : Name of topic to receive scene updates. Optional,
+  ///                     defaults to "/scene".
   class TransportSceneManager : public Plugin
   {
     Q_OBJECT

src/plugins/transport_scene_manager/TransportSceneManager.qml

@@ -15,14 +15,37 @@
  *
 */
 
-import QtQuick 2.0
-import QtQuick.Controls 2.0
+import QtQuick 2.9
+import QtQuick.Controls 2.1
 import QtQuick.Layouts 1.3
 
-// TODO: remove invisible rectangle, see
-// https://github.com/ignitionrobotics/ign-gui/issues/220
-Rectangle {
-  visible: false
-  Layout.minimumWidth: 100
-  Layout.minimumHeight: 100
+GridLayout {
+  columns: 1
+  columnSpacing: 10
+  Layout.minimumWidth: 350
+  Layout.minimumHeight: 200
+  anchors.fill: parent
+  anchors.margins: 10
+
+  property string service: 'N/A'
+  property string poseTopic: 'N/A'
+  property string deletionTopic: 'N/A'
+  property string sceneTopic: 'N/A'
+
+  Label {
+    Layout.columnSpan: 1
+    Layout.fillWidth: true
+    wrapMode: Text.WordWrap
+    text: "<b>Service</b>: /" + service +
+          "<br><b>Pose topic</b>: /" + poseTopic +
+          "<br><b>Deletion topic</b>: /" + deletionTopic +
+          "<br><b>Scene topic</b>: /" + sceneTopic
+  }
+
+
+  Item {
+    Layout.columnSpan: 1
+    width: 10
+    Layout.fillHeight: true
+  }
 }

test/integration/transport_scene_manager.cc

@@ -162,15 +162,24 @@ TEST(MinimalSceneTest, IGN_UTILS_TEST_ENABLED_ONLY_ON_LINUX(Config))
     QCoreApplication::processEvents();
     sleep++;
   }
+  EXPECT_TRUE(sceneRequested);
+  EXPECT_LT(sleep, maxSleep);
 
   auto scene = engine->SceneByName("banana");
   ASSERT_NE(nullptr, scene);
 
   auto root = scene->RootVisual();
   ASSERT_NE(nullptr, root);
 
+  for (sleep = 0; root->ChildCount() < 2 && sleep < maxSleep; ++sleep)
+  {
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));
+    QCoreApplication::processEvents();
+  }
+  EXPECT_LT(sleep, maxSleep);
+
   // Check scene is populated
-  EXPECT_EQ(2u, root->ChildCount());
+  ASSERT_EQ(2u, root->ChildCount());
 
   // First child is user camera
   auto camera = std::dynamic_pointer_cast<rendering::Camera>(