Point Cloud Editor: fix select2D after switch to QOpenGLWidget

[mvieth]

In commit 38cbd62 the CloudEditorWidget was switched from QGLWidget to QOpenGLWidget to be compatible with Qt6. The select2D tool needs the viewport and the projection matrix, which are loaded with glGetIntegerv(GL_VIEWPORT,viewport) and glGetFloatv(GL_PROJECTION_MATRIX, project) (they are set in CloudEditorWidget::resizeGL). However, since the switch to QOpenGLWidget, the received projection matrix is just the identity matrix, and the viewport does not look correct either. It seems that QOpenGLWidget overwrites them. These changes introduce variables in CloudEditorWidget to safely store the viewport and projection matrix, and passes a function to the select2D tool to ask for them when needed.

[themightyoarfish]

I tried this branch without building from scratch and found no difference. Now with a clean build, I am unable to configure, for some reason. Possibly unrelated:

> cmake .. -DBUILD_apps=ON -DBUILD_apps_point_cloud_editor=ON -DQt5_DIR=/opt/homebrew/Cellar/qt@5/5.15.8/lib/cmake/Qt5

[…]

CMake Warning (dev) in apps/CMakeLists.txt:
  AUTOGEN: No valid Qt version found for target pcl_apps.  AUTOMOC, AUTOUIC
  and AUTORCC disabled.  Consider adding:

    find_package(Qt<QTVERSION> COMPONENTS Widgets)

  to your CMakeLists.txt file.
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) in apps/point_cloud_editor/CMakeLists.txt:
  AUTOGEN: No valid Qt version found for target pcl_point_cloud_editor.
  AUTOMOC, AUTOUIC and AUTORCC disabled.  Consider adding:

    find_package(Qt<QTVERSION> COMPONENTS Widgets)

  to your CMakeLists.txt file.
This warning is for project developers.  Use -Wno-dev to suppress it.

<deps>/lib/cmake/vtk-9.1/VTK-targets.cmake:610 (set_target_properties):
  The link interface of target "VTK::GUISupportQt" contains:

    Qt5::OpenGL

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  <deps>/lib/cmake/vtk-9.1/vtk-config.cmake:138 (include)
  cmake/pcl_find_vtk.cmake:31 (find_package)
  CMakeLists.txt:393 (include)


CMake Error at apps/point_cloud_editor/CMakeLists.txt:89 (target_link_libraries):
  Target "pcl_point_cloud_editor" links to:

    ::Widgets

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

[themightyoarfish]

indeed, same on master. i guess my system is messed up again. but at least the non-clean build did not fix the problem.

[mvieth]

@themightyoarfish Please try again with a clean build. In my tests these changes did fix the problem.

[themightyoarfish]

I managed a clean build, but unfortunately the problem persists.

[themightyoarfish]

Maybe this is a new macos m1 problem of some sort or dependency-related.

[mvieth]

@themightyoarfish Can you put the following in Select2DTool::end in select2DTool.cpp after the for-loop and see what it prints:
PCL_INFO("[Select2DTool::end] indices.size()=%zu, project=(%g, %g, %g, %g, %g, %g, %g, %g, %g, %g, %g, %g, %g, %g, %g, %g), viewport=(%i, %i, %i, %i)\n", indices.size(), project[0], project[1], project[2], project[3], project[4], project[5], project[6], project[7], project[8], project[9], project[10], project[11], project[12], project[13], project[14], project[15], viewport[0], viewport[1], viewport[2], viewport[3]);

[mvieth]

@themightyoarfish Did you have a chance for further tests? @larshg tested this PR on Windows and found that it works. We would like to solve this problem within the next few days if possible, so that the fix can be included in the upcoming 1.13.1 release.

[themightyoarfish]

It prints

[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)

[themightyoarfish]

If i try random selections a bunch of times

FALLBACK (log once): Fallback to SW vertex for line stipple
FALLBACK (log once): Fallback to SW vertex processing, m_disable_code: 2000
FALLBACK (log once): Fallback to SW vertex processing in drawCore, m_disable_code: 2000
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=70951, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)
[Select2DTool::end] indices.size()=0, project=(1.99371, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 748, 861)

strangely, there is one occurrence with indices.size() != 0 (the selection should have had points in it for most attempts).

[mvieth]

@themightyoarfish Thanks, the projection matrix and viewport look very plausible. Next, can you put this after the other output: PCL_INFO("[Select2DTool::end] origin=(%i, %i), final=(%i, %i)\n", origin_x_, origin_y_, final_x_, final_y_);
Also, please test with table_scene_mug_stereo_textured.pcd so we can rule out that this problem is caused by the used point cloud.

[themightyoarfish]

I added the code and used the provided point cloud.

[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1552, 1128), final=(1230, 770)
[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1724, 1090), final=(546, 506)
[Select2DTool::end] indices.size()=187075, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1492, 1214), final=(560, 254)
[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(2044, 1222), final=(750, 594)
[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1740, 1052), final=(766, 522)
[Select2DTool::end] indices.size()=39688, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1862, 1094), final=(548, 426)
[Select2DTool::end] indices.size()=209280, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1696, 1122), final=(220, 370)
[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1504, 1074), final=(532, 488)
[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1696, 1116), final=(686, 664)
[Select2DTool::end] indices.size()=0, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(2338, 1346), final=(734, 614)
[Select2DTool::end] indices.size()=209280, project=(1.24275, 0, 0, 0, 0, 1.73205, 0, 0, 0, 0, -1, -1, -0, -0, -0.0002, -0), viewport=(0, 0, 1200, 861)
[Select2DTool::end] origin=(1724, 1022), final=(302, 222)

When I repeatedly select, I sometimes get some point selection, but it does not really correspond to the selected area.

[mvieth]

@themightyoarfish Do you use a high-resolution/"retina" display? I just remembered this issue and this pull request

[themightyoarfish]

I was wondering that as well. The linked issue was on another machine, but my display is 3456 × 2234, so I suppose that qualifies as hidpi.

[mvieth]

@themightyoarfish I guess then the straightforward solution would be to use

const auto ratio = this->devicePixelRatio();
viewport_ = {0, 0, static_cast<GLint>(width*ratio), static_cast<GLint>(height*ratio)};

in CloudEditorWidget::resizeGL. Can you try that?

[themightyoarfish]

Unfortunately does not help. The symptoms are very different than the scaling issue, so I'm not surprised.

[themightyoarfish]

actually i switched on my brain for a second and this diff solves it

diff --git a/apps/point_cloud_editor/src/cloudEditorWidget.cpp b/apps/point_cloud_editor/src/cloudEditorWidget.cpp
index 2f71fa7d3..e75cd031b 100644
--- a/apps/point_cloud_editor/src/cloudEditorWidget.cpp
+++ b/apps/point_cloud_editor/src/cloudEditorWidget.cpp
@@ -475,6 +475,9 @@ CloudEditorWidget::paintGL ()
 void
 CloudEditorWidget::resizeGL (int width, int height)
 {
+  const auto ratio = this->devicePixelRatio();
+  width = static_cast<GLint>(width*ratio);
+  height = static_cast<GLint>(height*ratio);
   glViewport(0, 0, width, height);
   viewport_ = {0, 0, width, height};
   cam_aspect_ = double(width) / double(height);

[themightyoarfish]

I am wondering though why it was working in earlier versions even though the display scaling issue was not touched since.