Merge pull request #1409: fix issues in Camera and Image display

src/rviz/default_plugin/camera_display.cpp

@@ -127,11 +127,7 @@ CameraDisplay::~CameraDisplay()
     unsubscribe();
     caminfo_tf_filter_->clear();
 
-
-    //workaround. delete results in a later crash
-    render_panel_->hide();
-    //delete render_panel_;
-
+    delete render_panel_;
     delete bg_screen_rect_;
     delete fg_screen_rect_;
 
@@ -276,15 +272,14 @@ void CameraDisplay::subscribe()
   std::string target_frame = fixed_frame_.toStdString();
   ImageDisplayBase::enableTFFilter(target_frame);
 
-  ImageDisplayBase::subscribe();
-
   std::string topic = topic_property_->getTopicStd();
   std::string caminfo_topic = image_transport::getCameraInfoTopic(topic_property_->getTopicStd());
 
+  ImageDisplayBase::subscribe();
+
   try
   {
     caminfo_sub_.subscribe( update_nh_, caminfo_topic, 1 );
-    setStatus( StatusProperty::Ok, "Camera Info", "OK" );
   }
   catch( ros::Exception& e )
   {
@@ -338,10 +333,9 @@ void CameraDisplay::clear()
 
   new_caminfo_ = false;
   current_caminfo_.reset();
-
   setStatus( StatusProperty::Warn, "Camera Info",
-             "No CameraInfo received on [" + QString::fromStdString( caminfo_sub_.getTopic() ) + "].  Topic may not exist.");
-  setStatus( StatusProperty::Warn, "Image", "No Image received");
+             "No CameraInfo received on [" + QString::fromStdString( caminfo_sub_.getTopic() ) + "].\n"
+             "Topic may not exist.");
 
   render_panel_->getCamera()->setPosition( Ogre::Vector3( 999999, 999999, 999999 ));
 }
@@ -494,8 +488,6 @@ bool CameraDisplay::updateCamera()
 
   render_panel_->getCamera()->setCustomProjectionMatrix( true, proj_matrix );
 
-  setStatus( StatusProperty::Ok, "Camera Info", "OK" );
-
 #if 0
   static Axes* debug_axes = new Axes(scene_manager_, 0, 0.2, 0.01);
   debug_axes->setPosition(position);
@@ -529,8 +521,8 @@ bool CameraDisplay::updateCamera()
   bg_screen_rect_->setBoundingBox( aabInf );
   fg_screen_rect_->setBoundingBox( aabInf );
 
-  setStatus( StatusProperty::Ok, "Time", "ok" );
-  setStatus( StatusProperty::Ok, "Camera Info", "ok" );
+  setStatus( StatusProperty::Ok, "Time", "OK" );
+  setStatus( StatusProperty::Ok, "Camera Info", "processed" );
 
   return true;
 }
@@ -545,6 +537,7 @@ void CameraDisplay::caminfoCallback( const sensor_msgs::CameraInfo::ConstPtr& ms
   boost::mutex::scoped_lock lock( caminfo_mutex_ );
   current_caminfo_ = msg;
   new_caminfo_ = true;
+  setStatus( StatusProperty::Ok, "Camera Info", "received" );
 }
 
 void CameraDisplay::fixedFrameChanged()

src/rviz/display.cpp

@@ -292,40 +292,55 @@ void Display::reset()
   clearStatuses();
 }
 
+static std::map<PanelDockWidget*, bool> associated_widgets_visibility;
+inline void setVisible(PanelDockWidget* widget, bool visible)
+{
+  associated_widgets_visibility[widget] = visible;
+}
+inline bool isVisible(PanelDockWidget* widget)
+{
+  auto it = associated_widgets_visibility.find(widget);
+  return it != associated_widgets_visibility.end() && it->second;
+}
+
 void Display::onEnableChanged()
 {
   QApplication::setOverrideCursor(QCursor(Qt::WaitCursor));
   queueRender();
+  /* We get here, by two different routes:
+   * - First, we might have disabled the display.
+   *   In this case we want to close/hide the associated widget.
+   *   But there is an exception: tabbed DockWidgets shouldn't be hidden, because then we would loose the tab.
+   * - Second, the corresponding widget changed visibility and we got here via associatedPanelVisibilityChange().
+   *   In this case, it's usually counterproductive to show/hide the widget here.
+   *   Typical cases are: main window was minimized/unminimized, tab was switched.
+   */
   if( isEnabled() )
   {
     scene_node_->setVisible( true );
 
     if( associated_widget_panel_ )
     {
-      associated_widget_panel_->show();
+      if (!isVisible(associated_widget_panel_))
+        associated_widget_panel_->show();
     }
     else if( associated_widget_ )
-    {
       associated_widget_->show();
-    }
 
-    onEnable();
+    if( isEnabled() )  // status might have changed, e.g. if show() failed
+      onEnable();
   }
   else
   {
     onDisable();
 
     if( associated_widget_panel_ )
     {
-      if( associated_widget_panel_->isVisible() )
-      {
+      if (isVisible(associated_widget_panel_))
         associated_widget_panel_->hide();
-      }
     }
-    else if( associated_widget_ && associated_widget_->isVisible() )
-    {
+    else if( associated_widget_ )
       associated_widget_->hide();
-    }
 
     scene_node_->setVisible( false );
   }
@@ -359,6 +374,7 @@ void Display::setAssociatedWidget( QWidget* widget )
     if( wm )
     {
       associated_widget_panel_ = wm->addPane( getName(), associated_widget_ );
+      setVisible(associated_widget_panel_, true);
       connect( associated_widget_panel_, SIGNAL( visibilityChanged( bool ) ), this, SLOT( associatedPanelVisibilityChange( bool ) ));
       connect( associated_widget_panel_, SIGNAL( closed( ) ), this, SLOT( disable( )));
       associated_widget_panel_->setIcon( getIcon() );
@@ -377,15 +393,11 @@ void Display::setAssociatedWidget( QWidget* widget )
 
 void Display::associatedPanelVisibilityChange( bool visible )
 {
-  // if something external makes the panel visible, make sure we're enabled
-  if ( visible )
-  {
-    setEnabled( true );
-  }
-  else
-  {
-    setEnabled( false );
-  }
+  setVisible(associated_widget_panel_, visible);
+  // If something external makes the panel visible/invisible, make sure to enable/disable the display
+  setEnabled(visible);
+  // Remark: vice versa, in Display::onEnableChanged(),
+  //         the panel is made visible/invisible when the display is enabled/disabled
 }
 
 void Display::setIcon( const QIcon& icon )

src/rviz/display_group.cpp

@@ -107,6 +107,8 @@ void DisplayGroup::load( const Config& config )
     Config display_config = it->second;
     Display* disp = it->first;
     disp->initialize( context_ );
+    // avoid premature enabling of the display from "Value" instead of from "Enabled"
+    display_config.mapSetValue("Value", disp->isEnabled());
     disp->load( display_config );
   }
 

src/rviz/image/image_display_base.cpp

@@ -130,8 +130,13 @@ void ImageDisplayBase::reset()
 {
   Display::reset();
   if (tf_filter_)
+  {
     tf_filter_->clear();
+    update_nh_.getCallbackQueue()->removeByID((uint64_t)tf_filter_.get());
+  }
+
   messages_received_ = 0;
+  setStatus(StatusProperty::Warn, "Image", "No Image received");
 }
 
 void ImageDisplayBase::updateQueueSize()
@@ -184,6 +189,7 @@ void ImageDisplayBase::subscribe()
           update_nh_
         ));
         tf_filter_->registerCallback(boost::bind(&ImageDisplayBase::incomingMessage, this, _1));
+        // TODO: also register failureCallback to report about frame-resolving issues (now: "no images received")
       }
     }
     setStatus(StatusProperty::Ok, "Topic", "OK");
@@ -196,9 +202,6 @@ void ImageDisplayBase::subscribe()
   {
     setStatus( StatusProperty::Error, "Topic", QString("Error subscribing: ") + e.what());
   }
-
-  messages_received_ = 0;
-  setStatus(StatusProperty::Warn, "Image", "No Image received");
 }
 
 void ImageDisplayBase::unsubscribe()

src/rviz/properties/property.cpp

@@ -427,7 +427,7 @@ void Property::load( const Config& config )
   else if( config.getType() == Config::Map )
   {
     // A special map entry named "Value" means the value of this property, not a child.
-    // (If child "Value"does not exist, loadValue() will do nothing.)
+    // (If child "Value" does not exist, loadValue() will do nothing.)
     loadValue( config.mapGetChild( "Value" ));
 
     // Loop over all child Properties.