Pcl: Request to replace exit(0) with more flexible error handling in Poisson surface reconstruction module

[daniel-packard]

Some operations use exit (0) to immediately stop the execution of you program if an invalid parameter is specified. e.g. :
https://github.com/PointCloudLibrary/pcl/blob/b3e73d4f421f84d74a720aac575983233d1b40d9/surface/include/pcl/surface/3rdparty/poisson4/bspline_data.hpp#L443-L448

I would prefer to have a chance to provide messaging to the user, and let them enter a valid parameter instead.

Your Environment

• Operating System and version: Windows 10
• Compiler: MSVC
• PCL Version: 1.9

Context

Trying to implement a flexible GUI application that allows the user to experiment with various combinations of parameters. If the application shuts down immediately whenever they attempt an invalid combination of parameters, that's.... very bad.

Code to Reproduce

The following code will reproduce this in at least these two cases (and maybe more cases that I haven't stumbled across yet):

• MESH_PoissonDegree > 2
• MESH_PoissonSolverDivide < MESH_PoissonMinDepth

  Poisson<PointXYZRGBNormal> p;
  p.setInputCloud ( input );
  p.setDepth ( ConfigSystem::Instance ()->MESH_PoissonDepth );
  p.setMinDepth ( ConfigSystem::Instance ()->MESH_PoissonMinDepth );
  p.setPointWeight ( ConfigSystem::Instance ()->MESH_PoissonPointWeight );
  p.setScale ( ConfigSystem::Instance ()->MESH_PoissonScale );
  p.setSolverDivide ( ConfigSystem::Instance ()->MESH_PoissonSolverDivide );
  p.setIsoDivide ( ConfigSystem::Instance ()->MESH_PoissonIsoDivide );
  p.setSamplesPerNode ( ConfigSystem::Instance ()->MESH_PoissonSamplesPerNode );
  p.setConfidence ( ConfigSystem::Instance ()->MESH_PoissonConfidence );
  p.setOutputPolygons ( ConfigSystem::Instance ()->MESH_PoissonOutputPolygons );
  p.setDegree ( ConfigSystem::Instance ()->MESH_PoissonDegree );
  p.setManifold ( ConfigSystem::Instance ()->MESH_PoissonManifold );
  p.reconstruct ( *mesh );

Possible Solution

I would be happy to submit a pull request to provide better error handling here.. But I would first like to ask if PCL implements any standard patterns for handling errors more gracefully. If so, can you please point me in the right direction?

[taketwo]

Poisson surface reconstruction code is a 3rd-party library that was imported into PCL source tree at some point. The original project has evolved since then, but the PCL copy was never updated, so it's stale. I believe we merged a few fixes over time, but generally try to avoid touching this code.

That being said, I agree that exit(0) in a library is a horrible idea. If you submit a pull request that instead throws an exception, we will merge it.

There is a design document regarding exceptions, however it don't think it's consistently followed within PCL codebase. Plus, the code in question is 3rd-party anyway, so I'd avoid making it dependent on PCL point types such as PCLException. I think the best approach would be to define a custom exception (or multiple, if you need) within poisson4/ and throw them.

[daniel-packard]

The original project has evolved since then, but the PCL copy was never updated

Is there any interest in updating the PCL copy, and maybe trying to make it easier to stay in sync with updates to the upstream project?

[jspricke]

I would prefer to push our changes upstream and make it a proper dependency (but I don't have time to work on it, sorry).

[taketwo]

Making it a proper dependency would be the right way to go, of course. However the project in question is not particularly dependee-friendly. It's not packaged in debian, not CMake-based, etc. So currently I don't really see how we cloud make it a dependency. Besides, as far as I can see, it's still full of exit(0)s, so your original problem won't be solved.

[SergioRAgostinho]

Well, from a dependee perspective, as long as it has installation rules we can have CMake find it from our side.

Nevertheless, all the potential changes discussed do require a certain involvement from the owner/maintainer. So it's probably relevant to ping him to see if this would be of interest.

[daniel-packard]

I don't think the project is structured as a library, ~and it only has a makefile... so I'm guessing cross platform was not a priority~

Edit: I take that back, the project does have a visual studio solution file

yeah, I think it's a good idea to ping the owner / maintainer

[taketwo]

Well, from a dependee perspective, as long as it has installation rules we can have CMake find it from our side

Yes, but since the library is not packaged for major distributions, it would require manual install from the user of PCL. Right now you check out PCL and get Poisson reconstruction at no extra effort. With the switch we are discussing, this will no longer be the case, so a downgrade in convenience. We'll also need some additional effort on CI. Additional effort on Windows installers.

I'm therefore extremely skeptical about this endeavor. (Personally I have no time neither to implement/assist in such a switch, nor to deal with the consequences of it.)

[daniel-packard]

For now, I'll get a PR up here to handle my immediate issues with exit( 0 );

I reached out, just to gauge interest from the project owner: https://github.com/mkazhdan/PoissonRecon/issues/97

If that goes anywhere, maybe we can re-up a discussion about pushing improvements from the work here in PCL to the upstream project, and about converting it to a proper dep

[daniel-packard]

Hey maintainers -- any thoughts on throwing exceptions inside an omp loop?

addressing the exit(0) on line 393:
https://github.com/PointCloudLibrary/pcl/blob/master/surface/include/pcl/surface/3rdparty/poisson4/multi_grid_octree_data.hpp#L384-L402

Found a comment here regarding the matter from jimdempseyatthecove.. but wanted to bounce here before making changes.

PS: https://software.intel.com/en-us/blogs/2009/11/05/openmp-and-exceptions

[SergioRAgostinho]

I'm too inexperienced with OMP to have a solid opinion, but from the post you linked, I would say to follow something akin to their strategy. If an error condition is detected set an error status inside the loop and continue. The moment you're out of the parallel loop section, throw the exception.