Add Part BOA multiCut, multiCommon and multiSection methods

[triplus]

Pull request:

https://github.com/FreeCAD/FreeCAD/pull/332

Reference:

viewtopic.php?f=10&t=18179&start=30#p143338

[ickby]

Nice work! I just have a general question about the API: does it make sense to have always two functions, cut and multiCut etc? Seems to be a bad API design, and honestly I'm not sure why it was done this way initially with multiFuse. IMHO in c++ the multiFuse version with a std::vector should only be a overload of fuse with a vector, and tolerance parameter should be added to both.
Same for python, just see what the user provides and handle it appropriately. What do you guy think about that?

[wmayer]

Seems to be a bad API design, and honestly I'm not sure why it was done this way initially with multiFuse.

The reason is that we have two feature classes Part::Fuse and Part::MultiFuse. To be consistent with this naming the low-level Python function got these function names to reflect the feature class names.

and tolerance parameter should be added to both.

When looking at the source code you will realize that the "fuzzy" stuff has been added to OCC >= 6.9. When adding the tolerance to fuse() then this function must be completely re-implemented for OCC >= 6.9 because the overloaded constructors where tool and argument shapes are passed are deprecated and won't consider the fuzzy value.

Then we also have generalFuse() and oldFuse() which work completely different to that.

[ickby]

When looking at the source code you will realize that the "fuzzy" stuff has been added to OCC >= 6.9. When adding the tolerance to fuse() then this function must be completely re-implemented for OCC >= 6.9 because the overloaded constructors where tool and argument shapes are passed are deprecated and won't consider the fuzzy value.
Then we also have generalFuse() and oldFuse() which work completely different to that.

Yes, but the new "multi*" methods already implement all that, and actually they work exactly like the current Fuse/Cut etc methods if supplied with one tool only. So the needed changes would be minimal, one could think about changing the normal methods simply to create a vector of the single shape and pass it to multi* method.

The reason is that we have two feature classes Part::Fuse and Part::MultiFuse. To be consistent with this naming the low-level Python function got these function names to reflect the feature class names.

Ah, I was not aware of Part::MultiFuse. Bringing the new functionality from the API to the user now would mean all new document objects for multiCut etc. IMHO simply changing Part::Cut etc. to have a LinkList instead of link would make it easier.

But I don't wanrt to make too much fuzz, that are mere implementation / API details and of minor importance. I appreciate that the code was written and look forward to use it.

[wmayer]

The classes BRepAlgoAPI_Section and BRepAlgoAPI_Common do not behave as expected when using multiple tool shapes.
When having the shapes s1, s2 and s3 then one would assume that common does: s1 AND s2 AND s3 but instead it does s1 AND (s2 OR s3), i.e. it unites the tool shapes and then computes the intersection of the result and s1.

Firstly, this is confusing behaviour especially because the implementation for OCC 6.8 or older of multiCommon() where one iterates the shapes step by step produces the expected result and thus is inconsistent to the behaviour to OCC 6.9 or newer. Secondly, this makes the function obsolete because one can fuse the tool shapes with multiFuse() and then do the difference with cut() which has no or a negligible performance penalty.

The same problems apply to multiSection.

For me only multiCut() makes sense because this behaves as expected and shows the same behaviour for all OCC versions. But it must be verified that for complex shapes it's really faster than cut().

Test code:

Code: Select all

import Part
s1=Part.makeBox(10,10,10)
s2=Part.makeSphere(5)
s3=Part.makeCone(3,10,10)
c=s1.multiCommon((s2,s3))
Part.show(c)
Part.show(s1)
Part.show(s2)
Part.show(s3)

Code: Select all

import Part
s1=Part.makeBox(10,10,10)
s2=Part.makeSphere(5)
s3=Part.makeCone(3,10,10)
c=s1.multiSection((s2,s3))
Part.show(c)
Part.show(s1)
Part.show(s2)
Part.show(s3)

Code: Select all

import Part
s1=Part.makeBox(10,10,10)
s2=Part.makeSphere(5)
s3=Part.makeCone(3,10,10)
c=s1.multiCut((s2,s3))
Part.show(c)
Part.show(s1)
Part.show(s2)
Part.show(s3)

[triplus]

Nice work!

Thanks.

The classes BRepAlgoAPI_Section and BRepAlgoAPI_Common do not behave as expected when using multiple tool shapes.

Indeed. But i don't feel the same about the conclusion (that we don't need this two methods because of that) as there is more to it than that.

• Fuzzy Boolean operations
• Support of multiple arguments for a single Boolean operation
• Parallelization of Boolean Operations algorithm

You don't get any of that (and future additions in the form of SetRunParallel()) with the current methods. You have a valid point about different behaviour when multiple arguments are used compared to what Part Common does. I investigated this a bit and came to some conclusions:

• If shapes (arguments) don't intersect the result is what one would expect. Check out the Boolean Benchmark macro i made. Final shape is consistent with the result you get from corresponding current method.
• If shapes (arguments) intersect results are different.

That i guess can go both ways:

Code: Select all

import Part
from FreeCAD import Base

s1 = Part.makeBox(10, 10, 10, Base.Vector(5, 0, 0))
s2 = Part.makeSphere(5, Base.Vector(2, -2, -2))
s3 = Part.makeCone(3, 10, 10, Base.Vector(0, 10, 0))
c = s1.multiCommon((s2, s3))
Part.show(c)
Part.show(s1)
Part.show(s2)
Part.show(s3)

Try to achieve the same result with Part Common. And finally there is nothing preventing the usage of Part multiCommon method in a way (by not using option 2 from the list above) to i guess always get consistent results with Part Common:

Code: Select all

import time
import Part
s1 = Part.makeBox(10, 10, 10)
s2 = Part.makeSphere(5)
s3 = Part.makeCone(3, 10, 10)

n = 100

t = time.time()

while n:
    c = s1.multiCommon([s2])
    d = c.multiCommon([s3])
    n -= 1

timeMultiCommon = time.time() - t

Part.show(d)

n = 100

t = time.time()

while n:
    c = s1.common(s2)
    d = c.common(s3)
    n -= 1

timeCommon = time.time() - t

Part.show(d)
Part.show(s1)
Part.show(s2)
Part.show(s3)

print timeCommon
print timeMultiCommon
print str(int(timeCommon / timeMultiCommon * 100)) + "%"

[sgrogan]

Secondly, this makes the function obsolete because one can fuse the tool shapes with multiFuse() and then do the difference with cut() which has no or a negligible performance penalty.

For me only multiCut() makes sense because this behaves as expected and shows the same behavior for all OCC versions. But it must be verified that for complex shapes it's really faster than cut().

I think this is the most interesting test case. The software I use, that does allow multi-cut, offers a gui dialog to move objects from tool to base. I think OCC would require a fuse() or multifuse() of the base object in this situation, before a cut() or multicut(). Maybe this could be merged to allow broader testing?

I think that wmayer is correct that the inconsistent behavior with older versions of OCC, for section and common, is undesirable. Even at the API level it should probably be consistent. Then if the overhead overcomes the advantage?

There have been requests in the past for multi-cut functionality from the gui. From my understanding the required selection logic has always outweighed the convenience.

[wmayer]

Indeed. But i don't feel the same about the conclusion (that we don't need this two methods because of that) as there is more to it than that.

It's not a feeling, it's a fact.

1.Run the test code for multiSection & multiCommon.
2. Edit TopoShape::multiSection & TopoShape::multiCommon: replace the line

Code: Select all

#if OCC_VERSION_HEX <= 0x060800

with

Code: Select all

#if OCC_VERSION_HEX > 0x060800

to make sure to test the behaviour with older OCC versions
3. Rerun the tests from point 1.
4. Compare the results
=> This different behaviour is not acceptable!

[triplus]

I hear you but there are other facts involved. Like the Boolean Benchmark and the last code snippet i provided in this thread. Both demonstrate multiCommon can produce expected results compared to common and can do that faster.

As it took years of discussions to get to here i feel that giving it a few more days does makes sense. Over the weekend i will read everything again and see if there are any more tests to do that make sense. And it is not like multiCommon must replace current common everywhere. Its about using the best tool for the job in the given situation. And multiCommon method available at least form the Python console therefore can still make much sense.

P.S. If we feel strongly about it and believe support of multiple arguments for a single Boolean operation doesn't work as expected for common and section possibility of reporting a bug on OCC issue tracker can be explored.

[wmayer]

P.S. If we feel strongly about it and believe support of multiple arguments for a single Boolean operation doesn't work as expected for common and section possibility of reporting a bug on OCC issue tracker can be explored.

Have you ever checked my last posting? What I telling you is that the method multiCommon (and multSection) returns a different result for OCC 6.9 compared to OCC6.8.
For 6.9 it does this: s1 AND (s2 OR s2)
For 6.8 it does this: s1 AND s2 AND s2