Throw exceptions instead of `exit(0)` in Poisson surface reconstruction #2891

daniel-packard · 2019-03-05T21:58:21Z

surface/CMakeLists.txt

surface/include/pcl/surface/3rdparty/poisson4/multi_grid_octree_data.hpp

daniel-packard · 2019-03-05T22:02:01Z

surface/include/pcl/surface/3rdparty/poisson4/bspline_data.hpp

@@ -443,8 +444,8 @@ namespace pcl
    template< int Degree >
    void BSplineElements< Degree >::upSample( BSplineElements< Degree >& high ) const
    {
-      fprintf( stderr , "[ERROR] B-spline up-sampling not supported for degree %d\n" , Degree );
-      exit( 0 );
+      fprintf (stderr, "[ERROR] B-spline up-sampling not supported for degree %d\n", Degree);


Let me know if you want me to remove these fprintf stderr log messages

I'd 👍 removal of stderr messages. Now that there are exceptions, the user can decide what to do. We will miss a bit of debug information around line 230, but I don't think it's important for a user anyway.

removed in abbe953

We will miss a bit of debug information around line 230, but I don't think it's important for a user anyway.

Well, not necessarily. Exceptions do support messages.

Before, lines ~230 were logging detailed info about each "unindexed corner" (actually only the first.. because of the exit( 0 ); )

Now it's replaced with a single exception message that only reports a count e.g. Found 37 unindexed corner nodes during openMP loop execution. without any info about the specific corners involved.

Are you proposing we build up a string that includes details about every unindexed corner?

I spoke too soon. I agree with your call. It's too much info to put on an exception.

SergioRAgostinho

Some minor comments in situ. Please don't squash or rebase till we finish reviewing.

SergioRAgostinho · 2019-03-07T09:21:18Z

surface/include/pcl/surface/3rdparty/poisson4/multi_grid_octree_data.hpp

+        offsets[t + 1] = offsets[t] + count[t];
+      }
+
+      int paralellExceptionCount = 0;


This should be unsigned.

SergioRAgostinho · 2019-03-07T09:21:39Z

surface/include/pcl/surface/3rdparty/poisson4/multi_grid_octree_data.hpp

+        offsets[t + 1] = offsets[t] + count[t];
+      }
+
+      int paralellExceptionCount = 0;


same comment.

surface/include/pcl/surface/3rdparty/poisson4/octree_poisson.hpp

SergioRAgostinho · 2019-03-07T09:24:15Z

surface/include/pcl/surface/3rdparty/poisson4/octree_poisson.hpp

-      if( d<minDepth ) fprintf( stderr , "[ERROR] Node depth lower than min-depth: %d < %d\n" , d , minDepth ) , exit( 0 );
+      if (d < minDepth)
+      {
+        fprintf (stderr, "[ERROR] Node depth lower than min-depth: %d < %d\n", d, minDepth);


Same comment here.

SergioRAgostinho · 2019-03-07T09:29:43Z

surface/include/pcl/surface/3rdparty/poisson4/poisson_exceptions.h

+          {
+            sstream << "in " << file_name << " ";
+            if (line_number != 0)
+              sstream << "@ " << line_number << " ";


SergioRAgostinho · 2019-03-07T09:31:16Z

surface/include/pcl/surface/3rdparty/poisson4/poisson_exceptions.h

+    public:
+      PoissonBadArgumentException (const std::string& error_description,
+        const char* file_name = NULL,
+        const char* function_name = NULL,


Same comment on the nullptr use.

SergioRAgostinho · 2019-03-07T09:31:41Z

surface/include/pcl/surface/3rdparty/poisson4/poisson_exceptions.h

+    public:
+      PoissonOpenMPException (const std::string& error_description,
+        const char* file_name = NULL,
+        const char* function_name = NULL,


Same comment on the nullptr use.

SergioRAgostinho · 2019-03-07T09:31:48Z

surface/include/pcl/surface/3rdparty/poisson4/poisson_exceptions.h

+    public:
+      PoissonBadInitException (const std::string& error_description,
+        const char* file_name = NULL,
+        const char* function_name = NULL,


Same comment on the nullptr use.

SergioRAgostinho · 2019-03-07T09:32:30Z

surface/include/pcl/surface/3rdparty/poisson4/multi_grid_octree_data.hpp

-        for( int d=minDepth ; d<=maxDepth ; d++ )
+      for (int t = 0; t < threads; t++)
+      {
+


Awkward new line.

…ant err logging

daniel-packard · 2019-03-07T16:04:44Z

Addressed comments in cff47e1

SergioRAgostinho

This looks great to me. Thank you for your effort Daniel. Shall we discuss, squashing strategy. At first sight and looking at the individual commits I think this one should all go in a single commit.

We can do this from our side, so don't worry.

taketwo · 2019-03-07T17:13:35Z

👍 for single commit.

daniel-packard · 2019-03-07T17:13:55Z

[...] looking at the individual commits I think this one should all go in a single commit

We can do this from our side, so don't worry.

Sounds good to me -- thanks for the detailed review/feedback!

replace exit( 0 ) in poisson module with exceptions

362b419

daniel-packard commented Mar 5, 2019

View reviewed changes

surface/CMakeLists.txt Outdated Show resolved Hide resolved

daniel-packard commented Mar 5, 2019

View reviewed changes

surface/include/pcl/surface/3rdparty/poisson4/multi_grid_octree_data.hpp Show resolved Hide resolved

Updated name of exception for consistency

cdc195c

SergioRAgostinho added the module: surface label Mar 5, 2019

daniel-packard commented Mar 5, 2019

View reviewed changes

daniel-packard added 2 commits March 6, 2019 09:26

Fix copy/paste error

2ce2f1d

Remove fprintf log messages that are redundant with new exceptions

abbe953

taketwo approved these changes Mar 7, 2019

View reviewed changes

SergioRAgostinho reviewed Mar 7, 2019

View reviewed changes

Use nullptr in PoissonException, make counter unsigned, remove redund…

cff47e1

…ant err logging

SergioRAgostinho approved these changes Mar 7, 2019

View reviewed changes

taketwo requested review from taketwo and removed request for taketwo March 7, 2019 17:14

SergioRAgostinho merged commit d923607 into PointCloudLibrary:master Mar 8, 2019

taketwo changed the title ~~replace exit( 0 ) in poisson module with exceptions~~ Throw exceptions instead of exit(0) in Poisson surface reconstruction Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw exceptions instead of `exit(0)` in Poisson surface reconstruction #2891

Throw exceptions instead of `exit(0)` in Poisson surface reconstruction #2891

daniel-packard commented Mar 5, 2019 •

edited

Loading

daniel-packard Mar 5, 2019

taketwo Mar 6, 2019

daniel-packard Mar 6, 2019

SergioRAgostinho Mar 6, 2019

daniel-packard Mar 6, 2019

SergioRAgostinho Mar 6, 2019

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

SergioRAgostinho Mar 7, 2019

daniel-packard commented Mar 7, 2019

SergioRAgostinho left a comment

taketwo commented Mar 7, 2019

daniel-packard commented Mar 7, 2019

Throw exceptions instead of exit(0) in Poisson surface reconstruction #2891

Throw exceptions instead of exit(0) in Poisson surface reconstruction #2891

Conversation

daniel-packard commented Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-packard commented Mar 7, 2019

SergioRAgostinho left a comment

Choose a reason for hiding this comment

taketwo commented Mar 7, 2019

daniel-packard commented Mar 7, 2019

Throw exceptions instead of `exit(0)` in Poisson surface reconstruction #2891

Throw exceptions instead of `exit(0)` in Poisson surface reconstruction #2891

daniel-packard commented Mar 5, 2019 •

edited

Loading

SergioRAgostinho left a comment •

edited

Loading