implementation of ball pivoting for surface reconstruction

[t-lou]

algorithm from

@article{bernardini1999ball,
   title={The ball-pivoting algorithm for surface reconstruction},
   author={Bernardini, Fausto and Mittleman, Joshua and Rushmeier, Holly and Silva, Cl{\'a}udio and Taubin, Gabriel},
   journal={IEEE transactions on visualization and computer graphics},
   volume={5},
   number={4},
   pages={349--359},
   year={1999},
   publisher={IEEE}
 }

general structure from https://github.com/rodschulz/BPA fitted from https://github.com/t-lou/bpa_remake

example input point cloud output surface

[SergioRAgostinho]

Thanks for contributing this. It looks pretty neat 👍

Some preliminary comments:

• You haven't submitted any unit tests. How do you know your implementation is doing exactly what it's supposed to? The reconstructed image you posted is definitely a good sign but the question is still valid. I'm bringing this up because we need a mechanism to ensure us that if we have to modify your implementation we're not breaking it's intended functionality, constraints, assumptions, etc...
• Can provide you submit also an example? Something like the images you posted, if possible with pointclouds we already have in repo's test folder.
• Can you also submit a tutorial explaining a little bit what's the primer behind this method? What are its strong points? What made you use it/implement it/need it instead of the other surface reconstruction methods we provide for instance?

[t-lou]

I'll do unit test soon.

But where should I write the example and tutorial?

[taketwo]

All our tutorials are contained in this same repository under the "doc" directory. You can take any of the existing tutorials as an example. But I would say let's first concentrate on adding unit tests, reviewing and merging this. The tutorial can be added in a separate PR later on.

Unfortunately I don't have any time today/tomorrow, but I will go through the code and leave comments later this week.

[t-lou]

The unit test I wrote failed. But it is successful locally. Can you help me?

[SergioRAgostinho]

Unfortunately there's not much I can do on my side other than run it on a Darwin platform.

You need to think about a synthetic small test case, in which you can theoretically predict the outcome, without having to run the algorithm. You should be able to fully understand what are the algorithm's capabilities and limitations, and come up with test scenarios which exploit success and failure conditions.

Loading the bun.pcd and then performing reconstruction, gives me the impression you're doing the exactly reverse process which is better than not having a unit test, but it's not a good unit test.

My advice is to rewrite the unit test following the guidelines I just gave you.

[t-lou]

Do you mean composing a hand-made simple cloud and doing unit test with it?

But aren't many other unit tests done with the reverse way?

[SergioRAgostinho]

Do you mean composing a hand-made simple cloud and doing unit test with it?

Exactly

But aren't many other unit tests done with the reverse way?

In our repo? Definitely. But it doesn't necessarily make them good unit tests. :)

[taketwo]

Speaking of the Edge structure, since it's just a data container, I think there is no need to have getter functions (e.g. getCenter, isBackBall, etc.). The member fields can simply be public. Also, there is no need to define an empty destructor.

[t-lou]

Buenas mis amigos. Is the code and unit test better now?

[taketwo]

Thanks! I'll have a final look early next week and then if @SergioRAgostinho has no more comments we will be done.

[SergioRAgostinho]

👍 I should look into it tomorrow.

[taketwo]

I think the last function in your list is very specific to this class. The rest can be reused, I also thought about it, but since the functions are private, we can also move them at any later point in time, as needed.

[SergioRAgostinho]

The rest can be reused, I also thought about it, but since the functions are private, we can also move them at any later point in time, as needed.

The only thing I don't like about that is that it relies on our memory of it. I would prefer to pull them out now already, then later having to change the code to make them public-static.

[t-lou]

Do you mean to "common" module or a "common" module in surface module? I dislike to have such common function in a templated class, so I wrote them at first as local functions in hpp file. But is it good to add them to common parts, I mean, common module should be for many modules of pcl, right?

[taketwo]

I would prefer to pull them out now already, then later having to change the code to make them public-static.

Fine with me.

[SergioRAgostinho]

Do you mean to "common" module or a "common" module in surface module?

The common module. The headers we mentioned are all from there.

[t-lou]

Hi, I'm glad you answered. Which headers did you refer? So you want me to move all these type independent functions to "common module of pcl"?

[SergioRAgostinho]

Sorry for the extremely delayed reply.

Which headers did you refer?

From my original comment.

• getPlaneBetweenPoints (common.h/distances.h or even better, in a new header plane.h)
• getCircleCenter (common.h)
• getRotationAngle (common.h/angles.h)

So you want me to move all these type independent functions to "common module of pcl"?

Yes.

I plan to get back on track with this PR this weekend

[t-lou]

Hi, thank you for your reply, I'm still on it so no worries. Are you still available afterwards? I'm about to handle in my thesis, so I may have little time in the following days, maybe after one week?

[SergioRAgostinho]

Off course, take as much time as you need. Sooner or later I always come here to check on things.

[stale[bot]]

This pull request has been automatically marked as stale because it hasn't had any activity in the past 60 days. Commenting or adding a new commit to the pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

[t-lou]

@SergioRAgostinho Ciao, sorry for the delay, how are you going?

So, could you give me a bit more hints about how to do it? Like should I create a specific directory and namespace for the common functions of BPA? Or should I add beside the current ones? :)

[SergioRAgostinho]

Ciao, sorry for the delay, how are you going?

Hey :) Busy times and work is starting to stack/slack around here :sweat_smile:.

The idea is to add those functions beside the current ones there. Follow the patterns you encounter on each file i.e.: I don't think they define an internal common namespace so follow the same rule and try to use and sort your function parameters just like the remaining functions on each file do (input parameter first and output second or otherwise, etc...)

[t-lou]

Hi, sorry for the miss-operations. After some reading, I think geometry.h is better as a container for the first two function: plane in between and circle centre, how do you think? I can also correct this odd thing, that the functions are defined in .h file.

Maybe also the case that angle.hpp should be a cpp file, as there is no template. How do you like?

[t-lou]

Hello, the CI seems to fail because time went out, what could I do about it? Thx :)

[SergioRAgostinho]

Rebase with master. The tests were split in two jobs IIRC and you should no longer have timeouts.

[t-lou]

Hello, thank you for your suggestion, it looks like the VTK cannot be found.. What should I do?

[stale[bot]]

This pull request has been automatically marked as stale because it hasn't had any activity in the past 60 days. Commenting or adding a new commit to the pull request will revert this.

Come back whenever you have time. We look forward to your contribution.