Add Part BOA multiCut, multiCommon and multiSection methods
Add Part BOA multiCut, multiCommon and multiSection methods
Pull request:
https://github.com/FreeCAD/FreeCAD/pull/332
Reference:
viewtopic.php?f=10&t=18179&start=30#p143338
https://github.com/FreeCAD/FreeCAD/pull/332
Reference:
viewtopic.php?f=10&t=18179&start=30#p143338
Re: Add Part BOA multiCut, multiCommon and multiSection methods
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?
Same for python, just see what the user provides and handle it appropriately. What do you guy think about that?
Re: Add Part BOA multiCut, multiCommon and multiSection methods
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.Seems to be a bad API design, and honestly I'm not sure why it was done this way initially with multiFuse.
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.and tolerance parameter should be added to both.
Then we also have generalFuse() and oldFuse() which work completely different to that.
Re: Add Part BOA multiCut, multiCommon and multiSection methods
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.wmayer wrote: 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.
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.wmayer wrote: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.
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.
Re: Add Part BOA multiCut, multiCommon and multiSection methods
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:
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)
Re: Add Part BOA multiCut, multiCommon and multiSection methods
Thanks.ickby wrote:Nice work!
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.wmayer wrote:The classes BRepAlgoAPI_Section and BRepAlgoAPI_Common do not behave as expected when using multiple tool shapes.
- Fuzzy Boolean operations
- Support of multiple arguments for a single Boolean operation
- Parallelization of Boolean Operations algorithm
- 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.
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)
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)) + "%"
Re: Add Part BOA multiCut, multiCommon and multiSection methods
wmayer wrote: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.
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?wmayer wrote: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 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.
"fight the good fight"
Re: Add Part BOA multiCut, multiCommon and multiSection methods
It's not a feeling, it's a fact.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.
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
Code: Select all
#if OCC_VERSION_HEX > 0x060800
3. Rerun the tests from point 1.
4. Compare the results
=> This different behaviour is not acceptable!
Re: Add Part BOA multiCut, multiCommon and multiSection methods
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.
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.
Re: Add Part BOA multiCut, multiCommon and multiSection methods
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.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.
For 6.9 it does this: s1 AND (s2 OR s2)
For 6.8 it does this: s1 AND s2 AND s2