Shape Healing - ShapeAnalysis::OuterWire() considers next iteration element always to be a wire causing skipping of primal one

The problem is reproducible on OCCT 7.4.0, but not on 7.5.0 - so it seems the bug has been fixed by some patch.

Boolean operation produces a broken Solid:

Draw[26]> bcut c s1 s2
Warning: Unable to orient the shape correctly

On OCCT 7.5.0 such Solid doesn't cause BRepMesh hanging, but still should be checked if Boolean operation works as expected.

Branch [archived branch] has been created by Commenter 1.

[revision removed]


Detailed log of new commits:

Author: Commenter 1
Date: Wed Aug 31 17:40:33 2022 +0300

    0031926: Shape Healing - ShapeAnalysis::OuterWire() considers next iteration element always to be a wire causing skipping of primal one
    
    ShapeAnalysis::OuterWire(): fixed missed logic when TopoDS_Iterator notifies about more objects to iterate, but there are only vertices and no additional wires at all.

Branch [archived branch] has been updated forcibly by Commenter 1.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 1.

[revision removed]

Patch is ready for review.

Test reports are available at:
http://jenkins-test-occt/view/master-CR31926-OAN/view/COMPARE/

To integrate:
OCCT: CR31926
PRODUCTS: None

Using explorer instead of iterator is unjustified here. Explorer is a heavy object involving stack of iterators and additional memory allocations. It is needed to provide correct logic with using iterator.

-  TopoDS_Iterator anIt (F, Standard_False);
+  TopExp_Explorer anIt (F, TopAbs_WIRE);

Declare aWire as a reference instead of a value:

    const TopoDS_Shape aWire = anIt.Value();

It is better to remove "REQUIRED" statement instead of commenting it out:

-puts "REQUIRED ALL: Meshing statuses: Failure"
+#puts "REQUIRED ALL: Meshing statuses: Failure"

Branch [archived branch] has been updated forcibly by Commenter 1.

[revision removed]

Despite of being lightweight, TopoDS_Iterator requires a lot of an additional code to prevent the described issue which partially duplicates functionality of TopExp_Explorer, whereas TopExp_Explorer itself provides required functionality by default without any additional charge, moreover, if you check, it is widely used along the remaining code of ShapeAnalysis. Why we need it at all then if there is just a better, and quicker, and simpler TopoDS_Iterator - we should either use it or remove it from OCCT, right?

So, indeed, I urge to use TopExp_Explorer to solve the problem here instead of inventing a wheel one more time and introduce new custom hidden problem like it was with #0031144 which was expected to fix exactly the same problem, but with different order of shapes (vertex-wire, instead of wire-vertex in here).

Overhead seems to be quite small, but code will be as clear and robust as possible.

>>> Declare aWire as a reference instead of a value:
The entire procedure will fail with suggested approach due to a detail with implementation of the iterator (both TopoDS_Iterator and TopExp_Explorer) which returns reference to its field instead of reference to the source shape which becomes invalid right at the next line when ShapeAnalysis::OuterWire() calls Next().

>>> It is better to remove "REQUIRED" statement instead of commenting it out:
Agree. Done.

Here we discuss only the method OuterWire. It is used as a separate method not dealing with other methods of the same class. So, it is not worth mentioning other methods here.
Explorer is very useful tool. We use it when we need to explore the shape more than one level in depth. But when we need to get access only to direct children it is preferable to use iterator.
In particular, here I do not see much complexity to solve the problem using iterator (wheel is not needed to invent here). I believe that it can be done without making the code unclear.

About declaring aWire as a reference, you are right, and it is my bad that I did not think about that side effect.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: Eugeny Maltchikov
Date: Tue Sep 13 10:43:37 2022 +0300

    # Avoid using ShapeAnalysis::OuterBound for computing area

Author: Commenter 1
Date: Wed Aug 31 17:40:33 2022 +0300

    0031926: Shape Healing - ShapeAnalysis::OuterWire() considers next iteration element always to be a wire causing skipping of primal one
    
    ShapeAnalysis::OuterWire(): fixed missed logic when TopoDS_Iterator notifies about more objects to iterate, but there are only vertices and no additional wires at all.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Oleg, could you please review the changes in CR31926_1?

http://jenkins-test-08.nnov.opencascade.com/view/CR31926_1-master-emv/view/COMPARE/

Commenter 5, I have no remarks.
Please have a look.

To integrate:
occt - CR31926_1
products - none

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]