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?

[adam-urbanczyk]

I don't have any specific technical reason @trelau - pybind11 feels more natural than SWIG.

[rainman110]

Here are my two cents to the pybind / swig discussion: Personally, I really like pybind11. It is very easy to use and powerful.

But there are also some downsides when switching to pybind:

• The generated pybind code tends to be slower and the compilation could also be slower.
• Switching to pybind breaks some libraries. TiGL for example uses SWIG for it's own bindings and relies on having the SWIG interface files of pythonocc. Switching to pybind would be a major issue for TiGL and potentially also other libs as well.
• pybind is python only. With the SWIG pythonocc code base, also other programming languages are not very far to be implemented.
• Alltough SWIG is a bit ugly to use, it is very robust and industry proven.

[adam-urbanczyk]

@rainman110 thanks for your point of view! Regarding the performance - do you have some benchmarks? I have heared opposite claims as well (but haven't seen real data). If you have other libraries depending on SWIG/PythonOCC it is indeed a deal breaker. Regarding maturity, I'd argue that pybind11 is industry proven as well (DOLFIN/FEniCS is a nice example of a large project that switched from SWIG to pybind11).

[janbrouwer]

Does this mean that ConnectEdgesToWires is broken in 7.4.0?
Sorry for the newbie question...
I got this IfcOpenShell sample running on 7.4.0 and the only thing that fails is ConnectEdgesToWires. It runs without issue but does not return any wires.
http://academy.ifcopenshell.org/using-ifcopenshell-and-pythonocc-to-generate-cross-sections-directly-from-an-ifc-file/

toptool_seq_shape = OCC.Core.TopTools.TopTools_SequenceOfShape()
for edge in section_edges: toptool_seq_shape.Append(edge)
edges_handle = OCC.Core.TopTools.TopTools_HSequenceOfShape(toptool_seq_shape)
wires = OCC.Core.TopTools.TopTools_SequenceOfShape()
wires_handle = OCC.Core.TopTools.TopTools_HSequenceOfShape(wires)
OCC.Core.ShapeAnalysis.ShapeAnalysis_FreeBounds.ConnectEdgesToWires(edges_handle, 1e-5, False, wires_handle)

[adam-urbanczyk]

In case someone would find it useful: I think I'll go for the following solution. Seems to be working so far and in principle should be OK for any method using handle&.

[](TopTools_HSequenceOfShape& edges, const Standard_Real toler, const Standard_Boolean shared, TopTools_HSequenceOfShape&  wires ){
   
  auto e = opencascade::handle<TopTools_HSequenceOfShape>();
    e = &edges;
    
   auto w = opencascade::handle<TopTools_HSequenceOfShape>();
    w = &wires;
    
    ShapeAnalysis_FreeBounds::ConnectEdgesToWires(e,toler,shared,w);
    
    edges = *e.get();
    wires = *w.get();
    
 }

[tpaviot]

Thank you @adam-urbanczyk

[Casey-Connors]

@adam-urbanczyk Could you explain how to implement the solution you mentioned above to fix the ConnectEdgesToWires method?

[jf---]

I got bitten by this as well, when making @trelau AFEM compatible with pythonocc ( a friendly fork, the API design is duly impressive... ). I sidestepped the issue by using another API call to generate a similar result. @janbrouwer basically, yes, which can be tested by the OCC.Core.TopTools.TopTools_SequenceOfShape().Size()

from OCC.Core.BRepBuilderAPI import BRepBuilderAPI_MakeWire

_wire = BRepBuilderAPI_MakeWire()
for e in edges:
    _wire.Add(e.object)
_wire.Build()

[lcpt]

Hi, there.
I'm using pythonocc 7.5.3, and I'm getting the same problem. The ConnectEdgesToWires runs smoothly, but it doesn't return any wires:

        wires= OCC.Core.TopTools.TopTools_HSequenceOfShape() # output
        wires_handle= OCC.Core.TopTools.Handle_TopTools_HSequenceOfShape(wires)
        # The edges are copied to the sequence
        for edge in section_edges: edges.Append(edge)

        OCC.Core.ShapeAnalysis.ShapeAnalysis_FreeBounds.ConnectEdgesToWires(edges, 1e-5, False, wires)

Any workaround?

[adam-urbanczyk]

AFAICT you need to wrap it in more friendly C++ function by hand (see on of my comments above). The problem is passing of smart pointers by reference: https://dev.opencascade.org/doc/refman/html/class_shape_analysis___free_bounds.html#a4809fe812b6a663a286df71b4d7bda3b .

[lcpt]

Hi @adam-urbanczyk. Thanks for your reply (and sorry for replying so late).

I understand the problem and can write some C++ code as a workaround, but the question is: where I must write this code? In what file?

[adam-urbanczyk]

Likely here: https://github.com/tpaviot/pythonocc-core/blob/master/src/SWIG_files/wrapper/ShapeAnalysis.i but I'm not the right person to ask that question. You'll probably need to read the docs of SWIG.

[lcpt]

Thanks, @adam-urbanczyk. I use boost.python, so SWIG is quite mysterious to me. I'll try to understand how it works.

[RobinVanTol]

Has this issue been fixed? I'm running into the same issue using 7.6.2, I give it edges but get the same empty wire object that I give as input.
@Casey-Connors did you manage to implement the change suggested by @adam-urbanczyk, and if so could you give some steps?
@jf--- I tried using your workaround but end up with a different type of wire object, can this easily be changed to a TopTools_HSequenceOfShape object?

[abdullahkhan-z]

Seems like the issue is still present in 7.7.0

[mcapocci]

+1 if someone has the time and expertise to fix this.

[tpaviot]

Fixed by 56eb733

[mcapocci]

Thank you! Just hit this a few weeks ago and the fix will be a great help.