Adding Roemer's Boolean Filter as a Remote Module

Development
1

@Ron84 I am working on a Merge Request to add Roland Roemer’s Boolean operator filter as a Remote Module. Roemer’s class is supposedly more robust than the existing filters and has several features listed in the README.md description.

So far I have:

  1. Forked his vtkbool repo.
  2. Modified his top-level CMakeLists.txt file to either add the remote module Boolean or behave as his original CMakeLists.txt.
  3. I have not needed to change any other of the original files.

I have tried to suppress many warning in his code with compile options. However, this has become unmanageable across multiple compilers. I would like to repair these warnings.

Also, some method names differ from the method names in the existing vtkBooleanOperationPolyDataFilter and vtktLoopBooleanPolyDataFilter.

Roemer’s class uses these methods:

    void SetOperModeToUnion ()
    void SetOperModeToIntersection ()
    void SetOperModeToDifference ()
    void SetOperModeToDifference2 ()

while the existing classes use:

  void SetOperationToUnion()
  void SetOperationToIntersection()
  void SetOperationToDifference()

I’m soliciting feedback as to the best approach:

  1. Forget adding it as a remote module. Use the filter as is described here.
  2. Change Reomer’s’ API to match VTK’s.
  3. Since we can’t tolerate warnings, repair the warnings by changing the original source and retain the changes only in the forked repo.
  4. Generate a PR to merge our changes into Roemer’s repo.

Thanks,

Bill

2

Bill,

We’ve been trying Ronald’s code and it does usually work better for us too. We’d been thinking to bring up a discussion about getting it into VTK, but you beat us to it. :slight_smile:

I’m not a git expert, but why a ‘remote module’, why not just bring it into VTK proper?

I guess the best path will especially depend on Ronald’s feedback…

Perhaps we should consider getting rid of the existing vtkBooleanOperationPolyDataFilter? Or rather, gut its implementation (replace it with Ronald’s) but maintain its public API.

Sean

3

Bringing it in as a remote module reduces VTK’s commitment to maintaining this code going forward. If it turns out to be essential after vetting as a remote module, then bringing it into VTK proper is a logical next step if the benefit outweighs the code maintenance costs. vtkBooleanOperationPolyDataFilter probably should have been a remote module when it was first integrated, but I don’t recall that the mechanism was available at the time. It did start life as a VTK Journal article.

Aside from differences in AP, one thing vtkBooleanOperationPolyDataFilter does that the code under consideration doesn’t do is interpolate point data, so it isn’t at present a drop in replacement. I don’t know how much work it would be to implement that, but I spent a good amount of time getting it working in the original vtkIntersectionPolyDataFilter.

4

Ronald’s filters would take a lot of work to bring into VTK proper. It lacks VTK style and uses several other of his files in three subdirectories. VTK classes are usually self-contained. For now, the short putt is to make it a Remote Module.

Bill

3 Likes
5

Currently a remote module is the best way. It should be a part of VTK when the remaining problems have been solved. The missing interpolation of PointData, the missing mesh optimizer … Especially the optimizer is really important for the filter.

I don’t want that API changes. Please leave it as it is!

One problem could be the static class member of Point. It has to be initialized before you can use the filter. I think int Point::_tag = 0; must be replaced with something other. Any ideas?

When I change my code, how could I merge them into @lorensen fork? I don’t know the right procedure for that. Should I create a PR when I have something new? And what’s with smaller patches?

@lorensen Could you please change “A new boolean operation filter by Ronald Romer” to “A new boolean operation filter by Ronald Römer”?

6

@Ron84 Can you place parens to correct this warning:
Tools.cxx: In function ‘bool TestPIP(PolyType&, Point&)’:
[CTest: warning matched] Remote/Boolean/Tools.cxx:204:28: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
&& (a.y < pt.y && b.y >= pt.y

7

OK, sounds reasonable. We can help with testing and fixing warnings and the like. We have various datasets that cause problems with the existing class, they could form the basis of some unit tests.

Sean

8

There are unit-tests. Just enable VTKBOOL_TESTING.

I will fix the parentheses-warnings this evening.

9

Very excited about this getting brought in!
Thank you all for this work effort!

1 Like
10

@lorensen I fixed most of the warnings in my repository. Except for sign-compare…

I’m comparing size_t with int. A classical mistake… How could I resolve it? Static cast from size_t to int?

11

I have lots of changes. You should wait until I’m done with mine. A
merge would be impossible if you make changes.

12

@lorensen do you have a branch in progress to peruse.

13

Ronald, static cast from size_t to int can truncate, perhaps better to upcast from int to size_t. Either way there is a signedness difference though. Best solution will depend on the exact code.

Regarding tests, yes, there are some currently, I mean we can contribute more tests, including some that lead to unexpected/undesired failure.

Sean

14

I’ll have a usable MR in a day or two.

1 Like
15

Who is “we”?

What kind of tests? Only some complex models?

Have you analysed vtkbool with valgrind? There are a few errors when running valgrind --tool=memcheck --leak-check=full ./testing 12.

It doesn’t make sense to me. Any of you?

16

I’m making progress. Still some OSX linking issues.

I converted your tests in testing (which I had to rename to tests because OSX is case insensitive for file names) to a standard VTK test. I also added a regression test that mimics the existing vtk boolean operation tests.

17

I just ran valgrind with my version and have no errors reported.

18

Here is the output of valgrind. I’m using gcc 9.1.0 and VTK 8.2.0 on Arch Linux.

19

We’re testing two different source trees. Let’s worry about valgrind when I get a workable version on all platforms,

20

@lorensen When will you be done with it?