New Loop Boolean

This is a new Boolean that is possible following the new code recently added to vtkIntersectionPolyDataFilter. @cory-quammen @dgobbi

I have given the new boolean the name vtkLoopBooleanPolyDataFilter as it finds intersection loops and runs around these in an oriented fashion in order to determine the sub-surfaces of the Boolean. I felt this was better than vtkBooleanOperationPolyDataFilter2, especially since there was a test already named that way. I'm open to changing the name though.

Oh, and as a plus, I removed the vtkDataSetSurfaceFilter that I was previously using, so I no longer need to bring in the Filters/Geometry module. Makes the change a lot smoother and simpler.

Again, more details on this can be found here: http://www.sciencedirect.com/science/article/pii/S0965997816300230

Basic content checks passed!

Branch-at: a2f44f3a
Acked-by: @kwrobot

Adam - really cool. If I don't review it in the next week, please ping me again on this merge request.

Is there an open access version of this paper? http://www.sciencedirect.com/science/article/pii/S0965997816300230

Test coverage is low. .h - 18% .cxx - 70%

Try to get as close to 100% for new classes.

I can send you a copy of the paper.

Thanks, good to know. I will see what I can do.

@updega2 @cory-quammen Should this replace BooleanOPerationPolyDataFilter? API is similar and if it is more robust, why should we support two filters that do the same thing only with a different algorithm?

@bill: I used it for boolean operation of primitive surfaces (cube, cylinder, cone...) and it worked fine for me. Much better then the old BooleanOperationPolyDataFilter. I didn't try it with meshes of more complex surfaces. So at least you should have both methods for a while. BooleanOperationPolyDataFilter is not working at all for primitive surfaces.

I'd like to see a rebase of this so that the buildbots can properly test it (I don't know how well set-up for a 6-month old branch they are).

Do: check

Warnings:

• Eligible for the 7.1.x branch.

The warnings do not need to be fixed, but it is recommended to do so.

mentioned in issue #16954 (closed)

Warnings:

• Eligible for the 7.1.x branch.

The warnings do not need to be fixed, but it is recommended to do so.

@updega2 @cory-quammen @dgobbi @seanm @demarle @lassoan +2

I think this should be merged. It does need reindenting to conform to new style and it does need its doxygen fixed to the new style. But these can wait. This seems to perform much better that the vtkBooleanOperationPolyDataFilter.

I did have some trouble with some user data Gears.stl and sphere.stl. It seemed to eliminate a disconnected region that was outside the difference region, but we can address that later.

Looks very promising! By integrating it into VTK proper it becomes much easier for users to test it and report/fix problems.

If I recall correctly, some folks who had problems with the existing implementation tried this one and it didn't fix their particular problem. I'm sure we'll run into more.

I propose to merge this as a parallel implementation to the existing one until we have enough test cases and user reports of which one is the clear winner. If this works in all cases, I would not hesitate to replace the existing implementation with this one, but I don't think that is completely clear at this point.

@cory-quammen I agree. Also, I can fix the indentation and doxygen stuff after the merge. I don't want to hold this up.

Let me know if I can be of any help. I haven't had time to improve the test coverage and error checking of the code because I have been focused on other things. I'm hoping to get some time in the next few months to do this.

@updega2 If you don't mind after this is merged as is, I can do the reindenting and doxygen fixes. No need for you to waste time on that.

Do: test

This merge request has been queued for testing. Test results may be viewed on CDash or Buildbot (only visible from inside Kitware).

Branch-at: a2f44f3a

@lorensen Would you mind creating a new merge request with this branch and rebase it on master? It doesn't build with a number of the current buildbot configurations.

@cory-quammen I will create a new merge request and remove this one.

If all goes well with the new MR, I'll close this one.

@bill : did you create a different MR? Could you share the link here if so?

Mine has been merged. I forgot to close the old one.

closed

I found it, here's a link for the archives:

!3008 (merged)

void vtkLoopBooleanPolyDataFilter::Impl::DetermineIntersection(...) { .... newloop.endPt = nextPt; // NB: IF nextPt is last (== numInterPts-1) - cellIds->GetId(1); out of range (x64)? => exeption newloop.loopType = 2; nextCell = cellIds->GetId(1); .... } Sergey