ShapeAnalysis_FreeBounds::ConnectEdgesToWires does not return the wires by reference

[trelau]

I think the "wires" input in this method is updated by reference using "=" in OCC source and isn't captured in Python bindings. I had similar issues in pyOCCT in various places and I had to just write a lambda whenever I encountered things like this on a case-by-case basis. Not sure how SWIG handles it.

I think this unit test demonstrates the issue:

import unittest

from OCC.Core.BRepBuilderAPI import BRepBuilderAPI_MakeEdge
from OCC.Core.ShapeAnalysis import ShapeAnalysis_FreeBounds, ShapeAnalysis_FreeBounds_ConnectEdgesToWires
from OCC.Core.TopTools import TopTools_HSequenceOfShape, TopTools_SequenceOfShape
from OCC.Core.gp import gp_Pnt


class Test_ShapeAnalysis_FreeBounds(unittest.TestCase):
    """
    Test for ShapeAnalysis_FreeBounds class.
    """

    @classmethod
    def setUpClass(cls):
        """
        Set up for ShapeAnalysis_FreeBounds.
        """
        p1 = gp_Pnt()
        p2 = gp_Pnt(1, 0, 0)
        e1 = BRepBuilderAPI_MakeEdge(p1, p2).Edge()

        p3 = gp_Pnt(1, 1, 0)
        e2 = BRepBuilderAPI_MakeEdge(p2, p3).Edge()

        seq = TopTools_HSequenceOfShape()
        seq.Append(e1)
        seq.Append(e2)
        cls._edges = seq

    def test_ConnectEdgesToWires(self):
        """
        Test ShapeAnalysis_FreeBounds::ConnectEdgesToWires
        """        
        wires = TopTools_HSequenceOfShape()

        ShapeAnalysis_FreeBounds.ConnectEdgesToWires(self._edges, 1.0e-7,
                                                     False, wires)
        
        self.assertEqual(wires.Length(), 1)



if __name__ == '__main__':
    unittest.main()

This is how I handled it in the pyOCCT source but I have no real way to automatically detect it during binding generation or runtime, so just left to fix as I run into these types of issues.

Great work on OCCT 7+ update btw. I spun up pyOCCT in the past out of necessity because I needed the improved performance of OCCT 7+ compared to OCE, but now I'm wondering if it's worth maintaining. I'm close to getting the 7.4 bindings done and might finish those just for fun, but not sure how much value it adds.

Any plans for SMESH support? I've wanted to go back and update my own SMESH project to be more like a feedstock/builder of the upstream Salome SMESH repo with patches and such for the latest NETGEN, and generate corresponding Python bindings. Maybe that is a better place for my time if you don't have plans for supporting that yourself?

[rainman110]

@trelau I think it should be considered to combine both code bases of pyocct and pythonocc since both have different strengths (I like the abstractions in pyocct). Pyocct also inspired me to implement transparent handles.

@trelau BTW, are you by accident at the AIAA Scitech in Orlando? I'll be there :)

[trelau]

@rainman110 ya maybe listing out the best of both would be a good exercise, and then see how the best of each could be captured in a single wrapping tech, if that is what you are suggesting

unfortunately i won't be at SciTech (i wish i was), tho you should stop by the nTopology booth if you want to see what my day job is 😆

[tpaviot]

@trelau I confirm the issue. I faced the same problem with the TDF_Label::FindAttributes method that returns an Handle(TDF_Attribute) by reference. As you did, I had to create an additional method (see https://github.com/tpaviot/pythonocc-core/blob/master/src/SWIG_files/wrapper/TDF.i#L1802), but it's not handled in an automatic fashion, and I don't know which classes/methods need such an hack.

Sor far I dropped the smesh support from pythonocc because:

1. it was based upon my smesh fork (https://github.com/tpaviot/smesh) that has to be maintained/updated, which is quite a big work; It was hard to get both smesh/pythonocc wrappers ready at the same time for a release

2. recent SMESH releases (and your fork) rely on additional dependencies (vtk, netgen, boost etc.), leading to something bigger and more complex.

For these reasons, IMO coupling smesh and occt under the same big wrapper is not the best way to go. I prefer the ifcopenshell/pythonocc relationship, based on shapes serialization exchanges. Each project (pythonocc, ifcopenshell) remains independent, but can still benefit from each other features. I don't know if it could be done for smesh, using this approach I would name "weak coupling".

@trelau @rainman110 FYI, there's an other ongoing wrapping work at CadQuery, from @adam-urbanczyk, see https://github.com/CadQuery/pywrap It's a similar approach, based on pybind11, that focuses on the code generator rather than the generated code itself.

[tpaviot]

@trelau I checked the occt source code for ShapeAnalysis_FreeBounds::ConnectEdgesToWires, file ShapeAnalysis_FreeBounds.cxx. I had to go through 3 methods to finally find the line owires = new TopTools_HSequenceOfShape; where the object, returned by reference, is created. IMO there's no way to automatically recognize this behavior, the way the method is called and parameters passed should be documented in the hxx or cxx code itself, in a docstring. Otherwise, how could you guess how to use the method ?

When trying to learn something about an occt method (for instance ConnectEdgesToWires), the best tool I used to work with is not the official API documentation, but rather grep 'ConnectEdgesToWires' -r . * from the /src directory, to see how it is used by occt guys themselves !!

[rainman110]

@trelau + @tpaviot Returning handles by reference argument (not as return value) does not work with the current implementation of the transparent handles.

I assumed, that this is not required, as it is not very pythonic and should be returned rather as a return value.

If we want to support handles created by reference argument, then the %wrap_handle implementation must be changed. I have an idea, how this could be solved, but it everything else than elegant.

[tpaviot]

@rainman110 How many methods are targeted ? Is it really necessary to look for something general and inelegant if applied to 20 methods only ?

I agree that it's better to stay close to python idioms. For instance, rather than

wires = TopTools_HSequenceOfShape()
ShapeAnalysis_FreeBounds.ConnectEdgesToWires(self._edges,
1.0e-7, False, wires)

I would prefer use this method as:

wires = ShapeAnalysis_FreeBounds.ConnectEdgesToWires(self._edges,
1.0e-7, False)

Note that, in the current pythonocc-core implementation, it's the way I implemented methods that return by reference Standard_Boolean, Standard_Integer and Standard_Real (see https://github.com/tpaviot/pythonocc-core/blob/master/src/SWIG_files/common/FunctionTransformers.i#L29)

[rainman110]

@tpaviot I don't know, how many functions are affected. We could use the generator to count the number of functions.

[tpaviot]

What would the search criterion be ?

[tpaviot]

My question was : is there something, in the method signature, that could let us find the Handles that are returned by reference. For instance, considering

void ShapeAnalysis_FreeBounds::SplitWires(const Handle(TopTools_HSequenceOfShape)& wires,
					   const Standard_Real toler,
					   const Standard_Boolean shared,
					   Handle(TopTools_HSequenceOfShape)& closed,
					   Handle(TopTools_HSequenceOfShape)& open) 

we can imagine that closed and open are returned by reference, whereas wires is not because of the const keyword. Could it be the rule ?

[tpaviot]

And, in the previous example, the void return is a strong clue as well

[tpaviot]

I do think that looking for at least one non const Handle(XXX) & parameter would give the list of affected methods. I ran a simple grep, it's much more than 20 methods

[rainman110]

Yes, non const handle references are a potential return value.

[trelau]

imo based on my own frequency of encountering such methods...I’d say start by fixing on an as-needed basis...capturing it in a test to verify the behavior remains consistent with new OCCT versions...and if possible capture it in the binding generator.

I can’t remember which methods (can look later but traveling today) but I did attempt at one point to come up with some rules to auto detect this...but it was never robust...so I just fix as needed, add a test, and for now I add a manual patch in the binding generator I’ve been using.

With this approach, and if the known issue and likely fix is documented, I think the open source users can identify these methods as they are encountered in real work and then fixed as needed. In my experience the fixes are quick and trivial and over time will be more robust than a best guess at auto detection.

[adam-urbanczyk]

Slightly off-topic: @rainman110 @trelau I'd be willing to contribute if you would consider switching to pybind11 (and something like libclang).

[trelau]

@adam-urbanczyk i've been following your project...good stuff...why pybind11 and not SWIG though? should we capture some discussions somewhere other than this ticket? maybe create a GitHub wiki page in pyOCCT that we can document stuff? i have some basic "design considerations" in the pyOCCT docs..but it's outdated and there a whole bunch of edge cases that might be worth discussing up front for further work and maybe even pybind11/SWIG discussions?