Fix add/remove of 3D text without custom viewport #2110

frozar · 2017-12-01T17:28:08Z

This PR in relative to issue #2109.

The tiny changes I had enable to display text3D when you don't use any viewport.

I am sure the deletions break the internal logic of the visualizer and I need helps to fix/understand it.

SergioRAgostinho · 2017-12-03T19:43:35Z

This doesn't fix the problem properly so it don't think it is a good idea to merge it like this.

I overlooked your case when one doesn't invoke createViewport explicitly. I worked things with this use case in mind

pcl/visualization/test/test_shapes_multiport.cpp

Lines 22 to 29 in 3d5d915

    
           // Start the visualizer 
        
           pcl::visualization::PCLVisualizer p ("test_shapes"); 
        
           int leftPort(0); 
        
           int rightPort(0); 
        
           p.createViewPort(0, 0, 0.5, 1, leftPort); 
        
           p.createViewPort(0.5, 0, 1, 1, rightPort); 
        
           p.setBackgroundColor (1, 1, 1); 
        
           p.addCoordinateSystem (1.0, "first");

What your changes tell me is that a default viewport is always created and that in my test example, this original viewport was overlaid with two other. What this means is that when the specified viewport id is 0, that first rendering element cannot be ignored, it needs to be checked and the corresponding shape added/deleted.

The bug fix should address that. I can fix this after I'm done with the octree begin/end iterators unit tests. Alternatively, you can also change the current PR to implement the logic I just described.

frozar · 2017-12-03T21:42:02Z

I'll do my best sempai 😼

frozar · 2017-12-04T19:36:23Z

⚠️ This PR ihas been rebase on top of the PR #2115

The work is done, ready to get a review. The solution I bring here is not really elegant but it works. I during the add or remove of a 3D text, I check if there is any custom viewport thanks to
the value rens_->GetNumberOfItems(). If this value is equal to 1, there is no custom viewport. I let you check the code.

I also add 2 simple tests to check the right behavior of the visualiser, without and with custom viewports. To compile them, I use the following command line (enable only what is necessary):

cmake -DPCL_ONLY_CORE_POINT_TYPES=ON -DPCL_NO_PRECOMPILE=ON -DBUILD_tools=OFF -DBUILD_examples=OFF -DBUILD_apps=OFF -DBUILD_2d=OFF -DBUILD_features=OFF -DBUILD_filters=OFF -DBUILD_outofcore=OFF -DBUILD_people=OFF -DBUILD_recognition=OFF -DBUILD_registration=OFF -DBUILD_sample_consensus=OFF -DBUILD_ml=OFF -DBUILD_stereo=OFF -DBUILD_surface=OFF -DBUILD_geometry=ON -DBUILD_io=ON -DBUILD_kdtree=ON -DBUILD_octree=ON -DBUILD_search=ON -DBUILD_visualization=ON -DBUILD_global_tests=ON -DBUILD_tests_common=OFF -DBUILD_tests_geometry=OFF -DBUILD_tests_io=OFF -DBUILD_tests_kdtree=OFF -DBUILD_tests_octree=OFF -DBUILD_tests_search=OFF -DBUILD_tests_surface=OFF -DBUILD_tests_segmentation=OFF -DBUILD_tests_features=OFF -DBUILD_tests_filters=OFF -DBUILD_tests_keypoints=OFF -DBUILD_tests_people=OFF -DBUILD_tests_outofcore=OFF -DBUILD_tests_recognition=OFF -DBUILD_tests_registration=OFF -DBUILD_tests_segmentation=OFF -DBUILD_tests_sample_consensus=OFF -DBUILD_tests_visualization=ON -DBUILD_TESTS=ON -DGTEST_INCLUDE_DIR=/home/frozar/soft/googletest/googletest/include -DGTEST_SRC_DIR=/home/frozar/soft/googletest/googletest -DCMAKE_BUILD_TYPE=Debug ..

To run the test cases:

make -j8 demo_text_simple && ./bin/demo_text_simple
make -j8 demo_text_simple_multiport && ./bin/demo_text_simple_multiport

PS: I let some line of "visualization/test/CMakeLists.txt" uncomment because the compilation of these programs are protected by the CMake variable BUILD_TESTS.

SergioRAgostinho · 2017-12-04T20:57:11Z

What happens if I do something like

pcl::visualization::PCLVisualizer viz ("Visualizator");
int leftPort (0);

viz.createViewPort (0, 0, 0.5, 1, leftPort);

[code to add shapes etc...]

Does the original viewport appear behind the new viewport?

PS: There's some parenthesis spacing issues you might want to have a look in your added test files.

frozar · 2017-12-04T22:39:00Z

I give a try to the situation you describe, and I got something like this.

So you get left side (half on the screen) which correspond to the leftPort and on the left side, you will see a part of the first viewport (not custom)... On the picture you can notice that the coordinate system is displayed on every viewport but not the 3D text.

Question: what is the correct behavior? What is the specification of the different viewport: the default one vs the custom ones? In the situation you give, should we display the text everywhere, on every viewport?

It's not clear.

PS: I'll check the parenthesis soon.

SergioRAgostinho

Can you try just applying the following changes.

diff --git a/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp b/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp
index 41805a0..c504116 100644
--- a/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp
+++ b/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp
@@ -674,8 +674,7 @@ pcl::visualization::PCLVisualizer::addText3D (
 
   // check all or an individual viewport for a similar id
   rens_->InitTraversal ();
-  rens_->GetNextItem (); //discard first because it's not a renderer for the viewps
-  for (size_t i = std::max (viewport, 1); rens_->GetNextItem () != NULL; ++i)
+  for (size_t i = viewport; rens_->GetNextItem () != NULL; ++i)
   {
     const std::string uid = tid + std::string (i, '*');
     if (contains (uid))
@@ -700,8 +699,8 @@ pcl::visualization::PCLVisualizer::addText3D (
 
   // Since each follower may follow a different camera, we need different followers
   rens_->InitTraversal ();
-  vtkRenderer* renderer = rens_->GetNextItem (); //discard first because it's not a renderer for the viewps
-  int i = 1;
+  vtkRenderer* renderer;
+  int i = 0;
   while ((renderer = rens_->GetNextItem ()) != NULL)
   {
     // Should we add the actor to all renderers or just to i-nth renderer?
@@ -753,8 +752,7 @@ pcl::visualization::PCLVisualizer::addText3D (
 
   // check all or an individual viewport for a similar id
   rens_->InitTraversal ();
-  rens_->GetNextItem (); //discard first because it's not a renderer for the viewps
-  for (size_t i = std::max (viewport, 1); rens_->GetNextItem () != NULL; ++i)
+  for (size_t i = viewport; rens_->GetNextItem () != NULL; ++i)
   {
     const std::string uid = tid + std::string (i, '*');
     if (contains (uid))
@@ -786,8 +784,7 @@ pcl::visualization::PCLVisualizer::addText3D (
 
   // Save the pointer/ID pair to the global actor map. If we are saving multiple vtkFollowers
   rens_->InitTraversal ();
-  rens_->GetNextItem (); //discard first because it's not a renderer for the viewps
-  int i = 1;
+  int i = 0;
   for ( vtkRenderer* renderer = rens_->GetNextItem ();
         renderer != NULL;
         renderer = rens_->GetNextItem (), ++i)

and for the removeText3D

diff --git a/visualization/src/pcl_visualizer.cpp b/visualization/src/pcl_visualizer.cpp
index 5ba1fdb..e390ba6 100644
--- a/visualization/src/pcl_visualizer.cpp
+++ b/visualization/src/pcl_visualizer.cpp
@@ -901,8 +901,7 @@ pcl::visualization::PCLVisualizer::removeText3D (const std::string &id, int view
 
   // check all or an individual viewport for a similar id
   rens_->InitTraversal ();
-  rens_->GetNextItem (); //discard first because it's not a renderer for the viewps
-  for (size_t i = std::max (viewport, 1); rens_->GetNextItem () != NULL; ++i)
+  for (size_t i = viewport; rens_->GetNextItem () != NULL; ++i)
   {
     const std::string uid = id + std::string (i, '*');
     ShapeActorMap::iterator am_it = shape_actor_map_->find (uid);

SergioRAgostinho · 2017-12-05T10:15:01Z

visualization/include/pcl/visualization/impl/pcl_visualizer.hpp

@@ -672,23 +672,45 @@ pcl::visualization::PCLVisualizer::addText3D (
  if (viewport < 0)
    return false;

-  // check all or an individual viewport for a similar id
+  // If there is no custom viewport and the viewport number is not 0, exit
+  if ((rens_->GetNumberOfItems () == 1) && (1 <= viewport))


Wouldn't

if (viewport => rens_->GetNumberOfItems ())

be generic for all cases and actually accomplish the same ?

SergioRAgostinho · 2017-12-05T10:23:24Z

visualization/src/pcl_visualizer.cpp

@@ -899,36 +899,64 @@ pcl::visualization::PCLVisualizer::removeText3D (const std::string &id, int view

  bool success = true;

+  // If there is no custom viewport and the viewport number is not 0, exit
+  if ((rens_->GetNumberOfItems () == 1) && (1 <= viewport))


Same comment here

SergioRAgostinho · 2017-12-05T10:37:09Z

I updated the diff. It was looking weird.

frozar · 2017-12-05T12:50:16Z

Ok, here I removed what I'd done, I applied the patches from @SergioRAgostinho and cherry-pick my previous commits about adding test. I check the result of this branch with the two new tests and it seems to work well. A review should be done again I think.

SergioRAgostinho

👍 thanks

SergioRAgostinho · 2017-12-05T13:19:26Z

visualization/test/text_simple_multiport.cpp

+  viz.createViewPort(0, 0, 0.5, 1, leftPort);
+  viz.createViewPort(0.5, 0, 1, 1, rightPort);
+
+  viz.addCoordinateSystem(1.0);


Space before the parenthesis from line 9 to 15.

Ok, it's done.

frozar · 2017-12-05T13:43:29Z

If you would like any squash before merge, let me know.

SergioRAgostinho · 2017-12-05T13:46:59Z

👍 in theory I'll do it from github. If we need something more specific I'll let you know.

I'm just gonna wait for CI tests to complete and check if we didn't output any new warnings.

SergioRAgostinho

Sorry for bringing this up. I just realized that addText3D and removeText3D actually return boolean flags whether or not the calls were successful, so we can actually write proper unit tests, with gtest. We can address this in another PR, if you prefer.

SergioRAgostinho · 2017-12-05T13:53:14Z

visualization/test/text_simple.cpp

+
+  viz.addText3D ("Following text", pcl::PointXYZ(0.0, 0.0, 0.0),
+                 1.0, 1.0, 0.0, 0.0, "id_following");
+  viz.spin ();


Isn't this a blocking call which continuously runs the main loop? Shouldn't spinOnce be used instead? This comment is applicable for all the other calls to spin you do.

In fact I was inspired by other tests from this directory, and the behavior is good. Blocking spin() method is correct. If you run the test, it allow you to see the effect of the different add/remove of text step by step. To pass to the next step, you just have to press "Q".

If I change it with spinOnce(), I will not be able to even see the window: it will open and close instantly.

frozar · 2017-12-05T14:17:42Z

As it is functionnal right now (and it allows me to work on my "real" project), I would like to do it in another pull request 😉

SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Dec 3, 2017

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Dec 3, 2017

SergioRAgostinho added needs: more work Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Dec 3, 2017

frozar force-pushed the fix_text3d branch 3 times, most recently from 778e968 to f55d693 Compare December 4, 2017 19:21

frozar changed the title ~~[TEXT3D] Break things (provocation)~~ [VISU] Fix add/remove of 3D text without custom viewport Dec 4, 2017

taketwo added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Dec 4, 2017

SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Dec 4, 2017

frozar force-pushed the fix_text3d branch from 67f04f7 to 85dc2cc Compare December 5, 2017 09:56

SergioRAgostinho mentioned this pull request Dec 5, 2017

Viewport Creation in PCLVisualizer #2119

Open

SergioRAgostinho reviewed Dec 5, 2017

View reviewed changes

SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Dec 5, 2017

frozar added 4 commits December 5, 2017 13:45

[VISU] Review from SergioRAgostinho.

8c77408

[TEST] Uncomment some test for visualisation.

c3991a1

[TEST] Add the test demo_text_simple.

8c05204

[TEST] Add the test demo_text_simple_multiport.

6d14398

frozar force-pushed the fix_text3d branch from 85dc2cc to 6d14398 Compare December 5, 2017 12:46

SergioRAgostinho approved these changes Dec 5, 2017

View reviewed changes

[CODING_STYLE] Add spaces.

5a228f6

SergioRAgostinho added status: ready to merge and removed needs: author reply Specify why not closed/merged yet labels Dec 5, 2017

SergioRAgostinho removed the status: ready to merge label Dec 5, 2017

SergioRAgostinho reviewed Dec 5, 2017

View reviewed changes

SergioRAgostinho merged commit e5d5631 into PointCloudLibrary:master Dec 5, 2017

SergioRAgostinho mentioned this pull request Dec 5, 2017

Display text3D #2109

Closed

frozar deleted the fix_text3d branch December 5, 2017 22:04

SergioRAgostinho mentioned this pull request Sep 2, 2018

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

Merged

taketwo changed the title ~~[VISU] Fix add/remove of 3D text without custom viewport~~ Fix add/remove of 3D text without custom viewport Sep 2, 2018

taketwo added the module: visualization label Sep 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix add/remove of 3D text without custom viewport #2110

Fix add/remove of 3D text without custom viewport #2110

frozar commented Dec 1, 2017

SergioRAgostinho commented Dec 3, 2017

frozar commented Dec 3, 2017

frozar commented Dec 4, 2017

SergioRAgostinho commented Dec 4, 2017

frozar commented Dec 4, 2017

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho Dec 5, 2017

frozar Dec 5, 2017

SergioRAgostinho Dec 5, 2017

frozar Dec 5, 2017

SergioRAgostinho commented Dec 5, 2017

frozar commented Dec 5, 2017

SergioRAgostinho left a comment

SergioRAgostinho Dec 5, 2017

frozar Dec 5, 2017

frozar commented Dec 5, 2017

SergioRAgostinho commented Dec 5, 2017

SergioRAgostinho left a comment

SergioRAgostinho Dec 5, 2017 •

edited

Loading

frozar Dec 5, 2017

frozar commented Dec 5, 2017

Fix add/remove of 3D text without custom viewport #2110

Fix add/remove of 3D text without custom viewport #2110

Conversation

frozar commented Dec 1, 2017

SergioRAgostinho commented Dec 3, 2017

frozar commented Dec 3, 2017

frozar commented Dec 4, 2017

SergioRAgostinho commented Dec 4, 2017

frozar commented Dec 4, 2017

SergioRAgostinho left a comment • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho Dec 5, 2017

Choose a reason for hiding this comment

frozar Dec 5, 2017

Choose a reason for hiding this comment

SergioRAgostinho Dec 5, 2017

Choose a reason for hiding this comment

frozar Dec 5, 2017

Choose a reason for hiding this comment

SergioRAgostinho commented Dec 5, 2017

frozar commented Dec 5, 2017

SergioRAgostinho left a comment

Choose a reason for hiding this comment

SergioRAgostinho Dec 5, 2017

Choose a reason for hiding this comment

frozar Dec 5, 2017

Choose a reason for hiding this comment

frozar commented Dec 5, 2017

SergioRAgostinho commented Dec 5, 2017

SergioRAgostinho left a comment

Choose a reason for hiding this comment

SergioRAgostinho Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

frozar Dec 5, 2017

Choose a reason for hiding this comment

frozar commented Dec 5, 2017

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho Dec 5, 2017 •

edited

Loading