Add new constructors to PCLVisualizer #2004

denix56 · 2017-09-22T18:32:50Z

New constructors allow to provide custom renderers and render windows. It gives the ability to use QVTKOpenGLWidget and lots of vtk render windows.

New constructors allow to provide custom renderers and render windows

denix56 · 2017-09-22T19:24:52Z

fixes #1989

Oh, I have found that there is the code repeat. I will fix it tomorrow.

taketwo · 2017-09-26T07:59:11Z

Could you please elaborate why you first used raw pointers and then switched to smart pointers? I'd like to understand the implications of both options.

denix56 · 2017-09-26T20:53:07Z

I moved to smart pointers, because the visualizer stores smart pointers (consistency) and it is safer (if we use this renderer somewhere else, and decide to delete visualizer, the renderer will not be deleted).

taketwo · 2017-09-27T09:21:57Z

because the visualizer stores smart pointers (consistency)

As far as I can tell, this is not entirely correct. The vtkRenderWindow object is indeed stored as a smart pointer member field (win_), however the vtkRenderer object is added to a collection called rens_, which actually stores raw pointers.

In general, the memory management in VTK is a bit of a mystery for me with all their "self-owning" pointers and reference counters inside objects. I'll just assume that you have tested your proposed changes (including destroying widget and visualizer in different order) and it does not crash.

taketwo

Generally looks good. Since you are already doing a bit of refactoring, maybe try to minimize code repeat between these two different versions of construct() function?

Also, please fix the code to match PCL style guide. Specifically, we need spaces between function names and brackets.

taketwo · 2017-09-27T09:23:00Z

visualization/src/pcl_visualizer.cpp

+  if (create_interactor)
+    createInteractor();
+
+  win_->SetWindowName(name.c_str());


Already done in line 339?

denix56 · 2017-09-29T18:03:38Z

I have dug into vtk library and have found that it is save to convert smart pointers to raw ones. When the object is deleted, the special function handles if the reference count is not bigger than 1 nad deletes the object only if it is true.

denix56 · 2017-09-29T18:18:49Z

Also, I would like to continue refactoring pcl visualizer, but it will be other pull requests.

taketwo · 2017-09-30T06:15:15Z

visualization/src/pcl_visualizer.cpp

+  // Set the window size as 1/2 of the screen size or the user given parameter
+  if (!camera_set_ && !camera_file_loaded_)
+  {
+    win_->SetSize (scr_size_x / 2, scr_size_y / 2);


Maybe I miss something, but why do we need this conditional resizing/repositioning? Resizing already happened in the other construct() function.

denix56 · 2017-09-30T17:00:52Z

I decided to add one more argument to 'small' construct function - resize_window.
This argument prevents from double window resizing. Because if we have cam file and load it, the small constructor resizes window first, then after loading camera parameters window will be resized again.

Another solution without additional argument would be to move window resizing to constructor (without camera loading), but I think that it would be inconsistent (we call only construct functions in all constructors) and harder to understand.

taketwo

I don't think that adding resize_window argument to one of the construct() overloads is a good idea. Note that the code that actually resizes the window in duplicated in both versions of the function nevertheless. I take this as a strong indication that the granularity that we chose for splitting construction work into functions is wrong.

What do you think about the following: split construct() into smaller functions, an call (some of) them as needed in each class constructor?

taketwo · 2017-09-30T17:20:24Z

visualization/src/pcl_visualizer.cpp

+  if (resize_window)
+  {
+    int scr_size_x = win_->GetScreenSize ()[0],
+      scr_size_y = win_->GetScreenSize ()[1];


Please only declare one name per line.

taketwo · 2017-09-30T20:48:30Z

Seems like the changes to the header file haven't been committed.

denix56 · 2017-10-01T11:34:59Z

Yeah, I missed to commit that header file.

It would be nice, if we could achieve merging of this pull request until tomorrow)

denix56 · 2017-10-02T06:39:35Z

The build failed due to too long build of stage, other parts are ok

taketwo

Reminder: squash on merge.

denix56 · 2017-10-02T17:32:16Z

Some more questions - will it be merged to master or it will be waiting for pcl 1.9.0? And do I have to do anything? I am a bit newbie :)

taketwo · 2017-10-03T09:07:29Z

From my side it's ready to be merged in master. But for nontrivial PRs we typically wait for a "yes" from a second maintainer. I think @SergioRAgostinho will be back from his vacations very soon, will give it a quick look, and merge.

SergioRAgostinho · 2017-10-08T15:58:19Z

visualization/include/pcl/visualization/pcl_visualizer.h

+         * \param[in] argc
+         * \param[in] argv
+         */
+        void setupCamera (int &argc, char **argv);


Small comment: I know this PR is only refactoring, but this signature is completely cryptic for me. Do you have any idea which format one passes to these camera parameters? Is there any way we can add some documentation to improve it this an the other signatures which pass these? From our current docs there's nothing on it.

As far as I understand, we pass the program arguments, which may contain -cam ".cam". It is good idea to add the documentation for it. I will do it, but in the another merge request. We are using my change in our project at the moment, and it will be nice to use pcl master repo instead of my fork.

nikolaseu · 2017-10-12T20:07:41Z

hey @denix56 great work! I was just trying to make pcl visualizer work with QVTKOpenGLWidget and found this.

I compiled from master today (not in debug mode :( ) and was just trying to use this new constructor but it crashes when doing the new.

Is this how it's supposed to be used? Do you see anything wrong? QVTKOpenGLWidget alone works ok.

   // before initializing QApplication, set the default surface format.
    QSurfaceFormat::setDefaultFormat(QVTKOpenGLWidget::defaultFormat());

    QApplication app(argc, argv);

    QVTKOpenGLWidget *widget = new QVTKOpenGLWidget();

    vtkNew<vtkRenderer> renderer;
    vtkNew<vtkGenericOpenGLRenderWindow> renderWindow;
    renderWindow->AddRenderer(renderer.Get());

    pcl::visualization::PCLVisualizer::Ptr pclViewer(new pcl::visualization::PCLVisualizer(renderer.Get(), renderWindow.Get(), "viewer", false));
    widget->SetRenderWindow(pclViewer->getRenderWindow());

    widget->show();

denix56 · 2017-10-13T11:23:30Z

Could you please provide the system info the you use? I have tested the code that you gave on Ubuntu 16.04 and it worked fine for me.

nikolaseu · 2017-10-13T11:43:03Z

pcl built from master yesterday and VTK 8.0.1 built with OpenGL2 and Module_vtkRenderingExternal, Qt 5.9.2, in macOS 10.13
Maybe I missed something when building pcl. I was using VTK7 and QVTKWidget previously and now I'm trying to update to VTK 8

denix56 · 2017-10-14T05:37:20Z

I will try to run the same code on Windows today. You could also try to update to latest vtk master (8.1) or replace vtkNew with vtkSmartPointer. Maybe in debug mode it produces some kind of error? 13 окт. 2017 г. 13:43 пользователь "Nicolas Ulrich" < notifications@github.com> написал:

…

pcl built from master yesterday and VTK 8.0.1 built with OpenGL2 and Module_vtkRenderingExternal, Qt 5.9.2, in macOS 10.13 Maybe I missed something when building pcl. I was using VTK7 and QVTKWidget previously and now I'm trying to update to VTK 8 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2004 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOdatX5VUE_N_AMqRm5prwlaI0i26IPbks5sr0zQgaJpZM4PhFAt> .

denix56 · 2017-10-14T14:59:26Z

Tested the code on Windows. I worked on it too. The problem might be caused if you use in your build pcl or vtk in release and qt in debug. As for me, in such combination it crashes.

taketwo · 2017-10-16T10:55:13Z

@nikolaseu If this is still a problem, please create a new issue with all the infos so that we can track it.

nikolaseu · 2017-10-19T17:29:43Z

I compiled vtk from master (8.1?) and it's working! thanks

tudodetalhado · 2017-10-30T00:34:26Z

I'm looking for this resource. Can I get it from master or need to wait for pcl 1.9.0?

SergioRAgostinho · 2017-10-30T09:15:50Z

You can get it from master.

tudodetalhado · 2017-11-03T11:02:18Z

Thank you, Sergio!
I wrote a custom widget to Qt using the new constructor, and I'm sharing the code:

VideoQVTKOpenGLWidget.h


    #ifndef VIDEOQVKTOPENGLWIDGET_H
    #define VIDEOQVKTOPENGLWIDGET_H

    #include <QWidget >
    #include <QVTKOpenGLWidget.h >
    #include <pcl/visualization/pcl_visualizer.h >
    using namespace pcl::visualization;

    class VideoQVTKOpenGLWidget : public QVTKOpenGLWidget
    {
      public:
        explicit VideoQVTKOpenGLWidget(QWidget *parent = 0);
        void populateCloud(pcl::PointCloud<pcl::PointXYZRGBA >::Ptr cloud);

      private:
        boost::shared_ptr<PCLVisualizer > _viewer;
        vtkSmartPointer<vtkGenericOpenGLRenderWindow > _renderWindow;
    };

    #endif // VIDEOQVKTOPENGLWIDGET_H

VideoQVTKOpenGLWidget.cpp


    #include "VideoQVTKOpenGLWidget.h"

    #include <vtkPointPicker.h >
    #include <vtkGenericOpenGLRenderWindow.h >

    VideoQVTKOpenGLWidget::VideoQVTKOpenGLWidget(QWidget *parent)
      : QVTKOpenGLWidget(parent)
    {
      auto renderer = vtkSmartPointer<vtkRenderer >::New();
      _renderWindow = vtkSmartPointer<vtkGenericOpenGLRenderWindow >::New();
      _renderWindow- >AddRenderer(renderer);
      _viewer.reset(new PCLVisualizer(renderer, _renderWindow, "viewer", false));
      this- >SetRenderWindow(_viewer- >getRenderWindow());
      this- >update();
    }

    void VideoQVTKOpenGLWidget::populateCloud(pcl::PointCloud <pcl::PointXYZRGBA >::Ptr cloud)
    {
      if(!_viewer- >updatePointCloud(cloud, "cloud")) {
        _viewer- >addPointCloud(cloud, "cloud");
      }
      _renderWindow- >Render();
    }

SergioRAgostinho · 2017-11-03T13:08:11Z

Better post it to the mailing list. The users don't come that often here because it's just a bug tracker. I'm sure you're gonna make some them very happy because I think QVtk is a recurrent topic.

Add new constructors to PCLVisualizer New constructors allow to provide custom renderers and render windows.

apalomer · 2018-09-04T06:09:05Z

I have been trying to use this constructor alongside with VTK trunk to create a pcl widget for Qt and I get segmentation faults if I set the create_interactor to true. However it is very weird because I do not get the segmentation fault all the times I run the code, but all the times that I get the segmentation fault it is in the same place, when the pcl::visualization::PCLVisualizer executes createInteractor (). In detail when in this function the following code is called interactor_->Initialize (); in line 285 of file pcl_visualizer.cpp. This actually segfaults within vtk when it calls the function XtDisplayInitialize(vtkXRenderWindowInteractor::App,this->DisplayId, "VTK","vtk",nullptr,0,&argc,nullptr);

I am running Ubuntu 18.04 using this commit for VTK and this commit for PCL. The two used libraries (VTK and PCL) are not installed, i link against this version instead of the installed one by setting the falgs -DPCL_DIR and -DVTK_DIR when configuring cmake: cmake -DPCL_DIR=/path/to/pcl/custom/install/dir -DVTK_DIR=/path/to/vtk/compiled ..

You can find an example code here.

SergioRAgostinho · 2018-09-04T12:24:10Z

@apalomer Let's move this to a new issue.

apalomer · 2018-09-04T12:57:23Z

Done: #2413

Add new constructors to PCLVisualizer

9abebf0

New constructors allow to provide custom renderers and render windows

Replace raw pointers with smart pointers, code re-usage

21248c3

taketwo added the needs: code review Specify why not closed/merged yet label Sep 26, 2017

taketwo reviewed Sep 27, 2017

View reviewed changes

Fix indents, remove repeating code

19b308c

taketwo reviewed Sep 30, 2017

View reviewed changes

taketwo added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Sep 30, 2017

Denys Senkin added 2 commits September 30, 2017 19:46

Remove double window resizing

9e95ccd

Add missed comment

1fe27c9

Remove unused parameter

492d6a5

taketwo reviewed Sep 30, 2017

View reviewed changes

Denys Senkin added 2 commits September 30, 2017 22:36

Split construct into several small functions

c1a3d85

Fix one name per line issue

6f4fe43

Add missing header file

a0219bb

taketwo approved these changes Oct 2, 2017

View reviewed changes

taketwo removed the needs: author reply Specify why not closed/merged yet label Oct 2, 2017

taketwo added this to the pcl-1.9.0 milestone Oct 2, 2017

SergioRAgostinho reviewed Oct 8, 2017

View reviewed changes

taketwo merged commit 9887bad into PointCloudLibrary:master Oct 9, 2017

denix56 deleted the PCLVisualizerCustomRenderers branch October 9, 2017 14:36

UnaNancyOwen pushed a commit to UnaNancyOwen/pcl that referenced this pull request Nov 24, 2017

Add new constructors to PCLVisualizer (PointCloudLibrary#2004)

73fc141

Add new constructors to PCLVisualizer New constructors allow to provide custom renderers and render windows.

SergioRAgostinho mentioned this pull request Sep 2, 2018

Change Log generation tool. Automates change log generation. #2396

Merged

taketwo added the module: visualization label Sep 2, 2018

SergioRAgostinho added the changelog: enhancement Meta-information for changelog generation label Sep 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new constructors to PCLVisualizer #2004

Add new constructors to PCLVisualizer #2004

denix56 commented Sep 22, 2017

denix56 commented Sep 22, 2017 •

edited

Loading

taketwo commented Sep 26, 2017

denix56 commented Sep 26, 2017 •

edited

Loading

taketwo commented Sep 27, 2017

taketwo left a comment

taketwo Sep 27, 2017

denix56 commented Sep 29, 2017

denix56 commented Sep 29, 2017

taketwo Sep 30, 2017

denix56 commented Sep 30, 2017

taketwo left a comment

taketwo Sep 30, 2017

taketwo commented Sep 30, 2017

denix56 commented Oct 1, 2017 •

edited

Loading

denix56 commented Oct 2, 2017

taketwo left a comment

denix56 commented Oct 2, 2017 •

edited

Loading

taketwo commented Oct 3, 2017

SergioRAgostinho Oct 8, 2017

denix56 Oct 9, 2017

nikolaseu commented Oct 12, 2017

denix56 commented Oct 13, 2017

nikolaseu commented Oct 13, 2017

denix56 commented Oct 14, 2017 via email

denix56 commented Oct 14, 2017

taketwo commented Oct 16, 2017

nikolaseu commented Oct 19, 2017

tudodetalhado commented Oct 30, 2017

SergioRAgostinho commented Oct 30, 2017

tudodetalhado commented Nov 3, 2017

SergioRAgostinho commented Nov 3, 2017

apalomer commented Sep 4, 2018

SergioRAgostinho commented Sep 4, 2018

apalomer commented Sep 4, 2018

Add new constructors to PCLVisualizer #2004

Add new constructors to PCLVisualizer #2004

Conversation

denix56 commented Sep 22, 2017

denix56 commented Sep 22, 2017 • edited Loading

taketwo commented Sep 26, 2017

denix56 commented Sep 26, 2017 • edited Loading

taketwo commented Sep 27, 2017

taketwo left a comment

Choose a reason for hiding this comment

taketwo Sep 27, 2017

Choose a reason for hiding this comment

denix56 commented Sep 29, 2017

denix56 commented Sep 29, 2017

taketwo Sep 30, 2017

Choose a reason for hiding this comment

denix56 commented Sep 30, 2017

taketwo left a comment

Choose a reason for hiding this comment

taketwo Sep 30, 2017

Choose a reason for hiding this comment

taketwo commented Sep 30, 2017

denix56 commented Oct 1, 2017 • edited Loading

denix56 commented Oct 2, 2017

taketwo left a comment

Choose a reason for hiding this comment

denix56 commented Oct 2, 2017 • edited Loading

taketwo commented Oct 3, 2017

SergioRAgostinho Oct 8, 2017

Choose a reason for hiding this comment

denix56 Oct 9, 2017

Choose a reason for hiding this comment

nikolaseu commented Oct 12, 2017

denix56 commented Oct 13, 2017

nikolaseu commented Oct 13, 2017

denix56 commented Oct 14, 2017 via email

denix56 commented Oct 14, 2017

taketwo commented Oct 16, 2017

nikolaseu commented Oct 19, 2017

tudodetalhado commented Oct 30, 2017

SergioRAgostinho commented Oct 30, 2017

tudodetalhado commented Nov 3, 2017

SergioRAgostinho commented Nov 3, 2017

apalomer commented Sep 4, 2018

SergioRAgostinho commented Sep 4, 2018

apalomer commented Sep 4, 2018

denix56 commented Sep 22, 2017 •

edited

Loading

denix56 commented Sep 26, 2017 •

edited

Loading

denix56 commented Oct 1, 2017 •

edited

Loading

denix56 commented Oct 2, 2017 •

edited

Loading