Fix selection buffer crash after resizing window (#446)

* fix selection buffer crash due to resize and incorrect selections Signed-off-by: Ian Chen <[email protected]> * test updating full selection buffer texture Signed-off-by: Ian Chen <[email protected]> * reenable visual at test Signed-off-by: Ian Chen <[email protected]> * fix codecheck Signed-off-by: Ian Chen <[email protected]> * testing rgb no depth, full buffer Signed-off-by: Ian Chen <[email protected]> * use 1x1 buffer, still no depth data Signed-off-by: Ian Chen <[email protected]> * prnt scaling factor Signed-off-by: Ian Chen <[email protected]> * disable device ratio Signed-off-by: Ian Chen <[email protected]> * reenable depth and utils test Signed-off-by: Ian Chen <[email protected]> * disable utils test, add visual at test after resize Signed-off-by: Ian Chen <[email protected]> * test texelfetch Signed-off-by: Ian Chen <[email protected]> * back to full buffer Signed-off-by: Ian Chen <[email protected]> * print ogre log Signed-off-by: Ian Chen <[email protected]> * codecheck Signed-off-by: Ian Chen <[email protected]> * fixing selection buffer mat script Signed-off-by: Ian Chen <[email protected]> * try 1x1 buffer again Signed-off-by: Ian Chen <[email protected]> * revert some test changes Signed-off-by: Ian Chen <[email protected]> * uncomment tests Signed-off-by: Ian Chen <[email protected]> * update scaling factor Signed-off-by: Ian Chen <[email protected]> * fix removing selection mat Signed-off-by: Ian Chen <[email protected]> * update screenScalingFactor Signed-off-by: Ian Chen <[email protected]> * minor tweak Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Louise Poubel <[email protected]>
gazebosim · Dec 6, 2021 · d88e99e · d88e99e
1 parent 3d98714
commit d88e99e
Show file tree

Hide file tree

Showing 9 changed files with 90 additions and 57 deletions.
diff --git a/ogre2/src/Ogre2Camera.cc b/ogre2/src/Ogre2Camera.cc
@@ -278,7 +278,6 @@ VisualPtr Ogre2Camera::VisualAt(const ignition::math::Vector2i &_mousePos)
   ignition::math::Vector2i mousePos(
       static_cast<int>(std::rint(ratio * _mousePos.X())),
       static_cast<int>(std::rint(ratio * _mousePos.Y())));
-
   Ogre::Item *ogreItem = this->selectionBuffer->OnSelectionClick(
       mousePos.X(), mousePos.Y());
 

diff --git a/ogre2/src/Ogre2RayQuery.cc b/ogre2/src/Ogre2RayQuery.cc
@@ -144,6 +144,10 @@ RayQueryResult Ogre2RayQuery::ClosestPoint(bool _forceSceneUpdate)
 //////////////////////////////////////////////////
 RayQueryResult Ogre2RayQuery::ClosestPointBySelectionBuffer()
 {
+  // update selection buffer dimension in case window is resized
+  this->dataPtr->camera->SelectionBuffer()->SetDimensions(
+    this->dataPtr->camera->ImageWidth(), this->dataPtr->camera->ImageHeight());
+
   RayQueryResult result;
   Ogre::Item *ogreItem = nullptr;
   math::Vector3d point;

diff --git a/ogre2/src/Ogre2SelectionBuffer.cc b/ogre2/src/Ogre2SelectionBuffer.cc
@@ -88,6 +88,9 @@ class ignition::rendering::Ogre2SelectionBufferPrivate
   /// into a render target or render texture.
   public: Ogre::CompositorWorkspace *ogreCompositorWorkspace = nullptr;
 
+  /// \brief Name of the compositor workspace definition
+  public: std::string ogreCompWorkspaceDefName;
+
   /// \brief The selection buffer material
   public: Ogre::MaterialPtr selectionMaterial;
 };
@@ -142,6 +145,16 @@ Ogre2SelectionBuffer::~Ogre2SelectionBuffer()
 {
   this->DeleteRTTBuffer();
 
+  // remove selectionMaterial in destructor
+  // this does not need to be done in DeleteRTTBuffer as we do not need to
+  // reload the same material every time
+  if (!this->dataPtr->selectionMaterial.isNull())
+  {
+    Ogre::MaterialManager::getSingleton().remove(
+        this->dataPtr->selectionMaterial->getName());
+    this->dataPtr->selectionMaterial.setNull();
+  }
+
   // remove selection buffer camera
   this->dataPtr->sceneMgr->destroyCamera(this->dataPtr->selectionCamera);
 }
@@ -181,10 +194,17 @@ void Ogre2SelectionBuffer::Update()
 /////////////////////////////////////////////////
 void Ogre2SelectionBuffer::DeleteRTTBuffer()
 {
-  if (this->dataPtr->selectionMaterial)
+  if (this->dataPtr->ogreCompositorWorkspace)
   {
-    Ogre::MaterialManager::getSingleton().remove(
-        this->dataPtr->selectionMaterial->getName());
+    // TODO(ahcorde): Remove the workspace. Potential leak here
+    this->dataPtr->ogreCompMgr->removeWorkspace(
+        this->dataPtr->ogreCompositorWorkspace);
+
+    this->dataPtr->ogreCompMgr->removeWorkspaceDefinition(
+        this->dataPtr->ogreCompWorkspaceDefName);
+    this->dataPtr->ogreCompMgr->removeNodeDefinition(
+        this->dataPtr->ogreCompWorkspaceDefName + "/Node");
+    this->dataPtr->ogreCompositorWorkspace = nullptr;
   }
 
   auto engine = Ogre2RenderEngine::Instance();
@@ -222,11 +242,16 @@ void Ogre2SelectionBuffer::CreateRTTBuffer()
   // The SelectionBuffer material is defined in script
   // (selection_buffer.material).
   std::string matSelectionName = "SelectionBuffer";
-  Ogre::MaterialPtr matSelection =
-      Ogre::MaterialManager::getSingleton().getByName(matSelectionName);
-  this->dataPtr->selectionMaterial = matSelection->clone(
-      this->dataPtr->camera->getName() + "_" + matSelectionName);
-  this->dataPtr->selectionMaterial->load();
+  std::string matSelectionCloneName =
+      this->dataPtr->camera->getName() + "_" + matSelectionName;
+  if (this->dataPtr->selectionMaterial.isNull())
+  {
+    Ogre::MaterialPtr matSelection =
+        Ogre::MaterialManager::getSingleton().getByName(matSelectionName);
+    this->dataPtr->selectionMaterial = matSelection->clone(
+        matSelectionCloneName);
+    this->dataPtr->selectionMaterial->load();
+  }
   Ogre::Pass *p = this->dataPtr->selectionMaterial->getTechnique(0)->getPass(0);
   Ogre::GpuProgramParametersSharedPtr psParams =
       p->getFragmentProgramParameters();
@@ -252,13 +277,15 @@ void Ogre2SelectionBuffer::CreateRTTBuffer()
 
   // create compositor workspace for rendering
   // Setup the selection buffer compositor.
-  const Ogre::String workspaceName = "SelectionBufferWorkspace" +
+  this->dataPtr->ogreCompWorkspaceDefName = "SelectionBufferWorkspace" +
       this->dataPtr->camera->getName();
 
+  std::string nodeSpaceDefName =
+      this->dataPtr->ogreCompWorkspaceDefName + "/Node";
+
   Ogre::CompositorNodeDef *nodeDef =
       this->dataPtr->ogreCompMgr->addNodeDefinition(
-      "AutoGen " + Ogre::IdString(workspaceName +
-      "/Node").getReleaseText());
+      nodeSpaceDefName);
   Ogre::TextureDefinitionBase::TextureDefinition *depthTexDef =
       nodeDef->addTextureDefinition("depthTexture");
   depthTexDef->textureType = Ogre::TextureTypes::Type2D;
@@ -346,15 +373,16 @@ void Ogre2SelectionBuffer::CreateRTTBuffer()
   }
 
   Ogre::CompositorWorkspaceDef *workDef =
-      this->dataPtr->ogreCompMgr->addWorkspaceDefinition(workspaceName);
+      this->dataPtr->ogreCompMgr->addWorkspaceDefinition(
+      this->dataPtr->ogreCompWorkspaceDefName);
   workDef->connectExternal(0, nodeDef->getName(), 0);
 
   this->dataPtr->ogreCompositorWorkspace =
       this->dataPtr->ogreCompMgr->addWorkspace(
         this->dataPtr->scene->OgreSceneManager(),
         this->dataPtr->renderTexture,
         this->dataPtr->selectionCamera,
-        workspaceName,
+        this->dataPtr->ogreCompWorkspaceDefName,
         false);
 }
 
@@ -369,14 +397,6 @@ void Ogre2SelectionBuffer::SetDimensions(
   this->dataPtr->height = _height;
 
   this->DeleteRTTBuffer();
-
-  if (this->dataPtr->ogreCompositorWorkspace)
-  {
-    // TODO(ahcorde): Remove the workspace. Potential leak here
-    this->dataPtr->ogreCompMgr->removeWorkspace(
-        this->dataPtr->ogreCompositorWorkspace);
-  }
-
   this->CreateRTTBuffer();
 }
 /////////////////////////////////////////////////
@@ -398,15 +418,23 @@ bool Ogre2SelectionBuffer::ExecuteQuery(const int _x, const int _y,
   if (!this->dataPtr->camera)
     return false;
 
+  // check camera has valid projection matrix
+  // There could be nan values if camera was resized
+  Ogre::Matrix4 projectionMatrix =
+      this->dataPtr->camera->getProjectionMatrix();
+  if (projectionMatrix.getTrans().isNaN() ||
+      projectionMatrix.extractQuaternion().isNaN())
+    return false;
+
    const unsigned int targetWidth = this->dataPtr->width;
    const unsigned int targetHeight = this->dataPtr->height;
 
    if (_x < 0 || _y < 0 || _x >= static_cast<int>(targetWidth)
        || _y >= static_cast<int>(targetHeight))
      return false;
 
-  // // 1x1 selection buffer, adapted from rviz
-  // // http://docs.ros.org/indigo/api/rviz/html/c++/selection__manager_8cpp.html
+  // 1x1 selection buffer, adapted from rviz
+  // http://docs.ros.org/indigo/api/rviz/html/c++/selection__manager_8cpp.html
   unsigned int width = 1;
   unsigned int height = 1;
   float x1 = static_cast<float>(_x) /
@@ -425,10 +453,10 @@ bool Ogre2SelectionBuffer::ExecuteQuery(const int _x, const int _y,
   transMatrix[0][3] -= x1+x2;
   transMatrix[1][3] += y1+y2;
   Ogre::Matrix4 customProjectionMatrix =
-      scaleMatrix * transMatrix * this->dataPtr->camera->getProjectionMatrix();
-
-   this->dataPtr->selectionCamera->setCustomProjectionMatrix(true,
-        customProjectionMatrix);
+      scaleMatrix * transMatrix *
+      this->dataPtr->camera->getProjectionMatrix();
+  this->dataPtr->selectionCamera->setCustomProjectionMatrix(true,
+      customProjectionMatrix);
 
   this->dataPtr->selectionCamera->setPosition(
       this->dataPtr->camera->getDerivedPosition());
@@ -440,9 +468,8 @@ bool Ogre2SelectionBuffer::ExecuteQuery(const int _x, const int _y,
 
   Ogre::Image2 image;
   image.convertFromTexture(this->dataPtr->renderTexture, 0, 0);
-
   Ogre::ColourValue pixel = image.getColourAt(0, 0, 0, 0);
-  // Ogre::ColourValue pixel = image.getColourAt(_x, _y, 0, 0);
+
   float color = pixel[3];
   uint32_t *rgba = reinterpret_cast<uint32_t *>(&color);
   unsigned int r = *rgba >> 24 & 0xFF;

diff --git a/ogre2/src/media/materials/programs/GLSL/selection_buffer_fs.glsl b/ogre2/src/media/materials/programs/GLSL/selection_buffer_fs.glsl
@@ -25,6 +25,7 @@ in block
 
 uniform sampler2D colorTexture;
 uniform sampler2D depthTexture;
+uniform vec4 colorTexResolution;
 
 out vec4 fragColor;
 
@@ -58,7 +59,8 @@ void main()
     point = vec3(inf);
 
   // color
-  vec4 color = texture(colorTexture, inPs.uv0);
+  vec4 color = texelFetch(colorTexture,
+      ivec2(inPs.uv0 * colorTexResolution.xy), 0);
 
   float rgba = packFloat(color);
 

diff --git a/ogre2/src/media/materials/scripts/selection_buffer.material b/ogre2/src/media/materials/scripts/selection_buffer.material
@@ -33,6 +33,8 @@ fragment_program selection_buffer_fs_GLSL glsl
   {
     param_named colorTexture int 0
     param_named depthTexture int 1
+
+    param_named_auto colorTexResolution texture_size 0
   }
 }
 

diff --git a/src/Utils.cc b/src/Utils.cc
@@ -104,7 +104,10 @@ float screenScalingFactor()
 {
   // todo(anyone) set device pixel ratio for high dpi displays on Windows
   float ratio = 1.0;
-#ifdef __linux__
+
+  // the scaling factor seems to cause issues with mouse picking.
+  // see https://github.com/ignitionrobotics/ign-gazebo/issues/147
+#if 0
   auto closeDisplay = [](Display * display)
   {
     if (display)

diff --git a/src/Utils_TEST.cc b/src/Utils_TEST.cc
@@ -121,15 +121,6 @@ void UtilTest::ClickToScene(const std::string &_renderEngine)
   root->AddChild(camera);
   camera->Update();
 
-  if (_renderEngine == "ogre2")
-  {
-    // tests using selection buffer fail on CI, see issue #170
-    // https://github.com/ignitionrobotics/ign-rendering/issues/170
-    igndbg << "Selection buffer based screenToScene test is disabled in "
-           << _renderEngine << "." << std::endl;
-    return;
-  }
-
   // API without RayQueryResult and default max distance
   result = screenToScene(centerClick, camera, rayQuery, rayResult);
 

diff --git a/test/integration/camera.cc b/test/integration/camera.cc
@@ -187,14 +187,6 @@ void CameraTest::VisualAt(const std::string &_renderEngine)
             << _renderEngine << std::endl;
     return;
   }
-  else if (_renderEngine == "ogre2")
-  {
-    // VisualAt tests fail on CI, see issue #170
-    // https://github.com/ignitionrobotics/ign-rendering/issues/170
-    igndbg << "VisualAt test is disabled in " << _renderEngine << "."
-           << std::endl;
-    return;
-  }
 
   // create and populate scene
   RenderEngine *engine = rendering::engine(_renderEngine);
@@ -285,6 +277,27 @@ void CameraTest::VisualAt(const std::string &_renderEngine)
     }
   }
 
+  // change camera size
+  camera->SetImageWidth(1200);
+  camera->SetImageHeight(800);
+
+  // render a few frames
+  for (auto i = 0; i < 30; ++i)
+  {
+    camera->Update();
+  }
+
+  // test that VisualAt still works after resize
+  {
+    unsigned int x = 300u;
+    auto vis = camera->VisualAt(math::Vector2i(x, camera->ImageHeight() / 2));
+    EXPECT_NE(nullptr, vis) << "X: " << x;
+    if (vis)
+    {
+      EXPECT_EQ("sphere", vis->Name());
+    }
+  }
+
   // Clean up
   engine->DestroyScene(scene);
   rendering::unloadEngine(engine->Name());

diff --git a/test/integration/scene.cc b/test/integration/scene.cc
@@ -154,14 +154,6 @@ void SceneTest::VisualAt(const std::string &_renderEngine)
             << _renderEngine << std::endl;
     return;
   }
-  else if (_renderEngine == "ogre2")
-  {
-    // VisualAt tests fail on CI, see issue #170
-    // https://github.com/ignitionrobotics/ign-rendering/issues/170
-    igndbg << "VisualAt test is disabled in " << _renderEngine << "."
-           << std::endl;
-    return;
-  }
 
   // create and populate scene
   RenderEngine *engine = rendering::engine(_renderEngine);
-Original file line number
+Diff line change
@@ Expand Up / @@ -33,6 +33,8 @@ fragment_program selection_buffer_fs_GLSL glsl @@
       {
         param_named colorTexture int 0
         param_named depthTexture int 1
+        param_named_auto colorTexResolution texture_size 0
       }
     }
@@ Expand Down @@