Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Implemented transparent handles #581

Closed

Conversation

rainman110
Copy link
Collaborator

@rainman110 rainman110 commented Nov 15, 2018

The OCCT handle smart pointer API is a C++ specific detail and should not be exposed to Python. Instead it should be completely invisible. This PR almost completely removes Handles_ from pythonOCC.

This makes the use of pythonOCC much cleaner and less verbose!

I used the std::shared_ptr swig library as a basis for the implementation.
To be compatible with most of the "old" code, GetHandle() and GetObject() methods still exist but do nothing except from a deprecation warning. In general, I tried to make sure that the code is backwards compatible as possible.

I adapted already the unit tests to not include the legacy code. The demos are working unmodified but should be adapted to avoid deprecation warnings.

The only thing that is not backwards compatible is, when previously an Null-Handle was returned - e.g. a non successful computation. Before, the object was a handle but .IsNull() was true. Now, it is a None type, which is more Pythonic.

The "magic" is implemeted in new swig typemaps in OccHandle.i.

Closes #539

@rainman110
Copy link
Collaborator Author

rainman110 commented Nov 15, 2018

BTW: Obviously, I made most of the changes with an adapted pythonocc-generator. I still have an issue with the PCDM_Document class, which mistakenly is detected as type of Standard_Transient. If this is fixed, I will send you the PR of the generator as well.

The OCCT handle smart pointer API is a C++ specific detail
and should not be exposed to Python. Instead it should be
completely invisible.
I used the std::shared_ptr swig library as a basis for the
implementation.
To be compatible with most of the "old" code, GetHandle() and
GetObject() methods still exist but do nothing except from
a deprecation warning. In general, I tried to make sure
that the code is backwards compatible as possible.

Closes tpaviot#539
@rainman110 rainman110 changed the title Implemented transparent handles WIP: Implemented transparent handles Nov 16, 2018
@rainman110
Copy link
Collaborator Author

There still seems to be an issue with python 2.7 and the automatic upcasting of handles. Concretely what currently does not work is:
c = Geom2d_TrimmedCurve(...)
geomapi_to3d(c, surface)

Somehow, swig does not see, that the handle_geom2d_trimmed curve IS A handle_geom2d_curve.

@tpaviot
Copy link
Owner

tpaviot commented Nov 16, 2018

@rainman110 Thank you for this contribution, it's a huge achievement. I remember working on removing handles some time ago, but without success ... This really makes things more pythonic, those f**** handles constantly reminds the python user he works with something coming from C/C++. GetHandle()/GetObject() was only an ugly hack to makes things working.

I would like the generator to keep consistent with the pythonocc-core SWIG files, is it possible that you also submit a PR on pythonocc-generator ?

@rainman110
Copy link
Collaborator Author

Sure... as I said I have to fix one issue in the generator regarding the class PCDM_Document. Then I'll send you a PR as well.

@tpaviot
Copy link
Owner

tpaviot commented Nov 16, 2018

Yes, I read your note regarding the PCDM_Document class. Don't spend to much time on this issue. My solution : after 15mn working on a class that I don't manage to get wrapped, I remove it from the wrapper !

@rainman110
Copy link
Collaborator Author

I sent you the PR for the generator. The issue with the one class (and obviously others where also affected) is fixed.

@rainman110
Copy link
Collaborator Author

Lets summarize the current status:

  • It builds
  • All tests succeed
  • Some examples fail. Reasons are
    • I did not wrap SMESH, therefore all SMESH examples fail
    • Still the issue with upcasting on Python 2.7
    • Segmentation fault during tests on macOS with python=3.7. I think this is the most critical issue!

@rainman110
Copy link
Collaborator Author

@tpaviot How do I add SMESH support? Which version of SMESH do I have to use?

@tpaviot
Copy link
Owner

tpaviot commented Nov 16, 2018

@rainman110 SMESH wrappers are generated the same way that other modules, using pythonocc-generator. You have to use smesh-676 (https://github.com/tpaviot/smesh/releases/tag/6.7.6). Running the pythonocc-gnererator, just set i the .conf file the path where are located the smesh headers.

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

@rainman110 The BRepMesh_DiscretRoot class seems to be missing (according to the issues on travis-ci.org), did you explicitly remove this class from the list of wrapped class ?

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

ok, the BRepMesh_DiscretRoot class is explicitely removed from the wrapper (see my comment tpaviot/pythonocc-generator@3fadc87#diff-c300b71a1f11fa6df1ad67021b5ef754R439)

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

@rainman110 which os/python version did you use to generate the SWIG files ? I'm running a Linux Ubuntu 18.04 machine, and running the generator creates a huge diff compared to your result.

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

@rainman110 There's a trailing GetHandle in DataExchange which should be removed. Something like that (not tested):

diff --git a/src/Extend/DataExchange.py b/src/Extend/DataExchange.py
index a9450c4..23f890b 100644
--- a/src/Extend/DataExchange.py
+++ b/src/Extend/DataExchange.py
@@ -26,7 +26,7 @@ from OCC.Core.IGESControl import IGESControl_Reader, IGESControl_Writer
 from OCC.Core.STEPControl import STEPControl_Reader, STEPControl_Writer, STEPControl_AsIs
 from OCC.Core.Interface import Interface_Static_SetCVal
 from OCC.Core.IFSelect import IFSelect_RetDone, IFSelect_ItemsByEntity
-from OCC.Core.TDocStd import Handle_TDocStd_Document
+from OCC.Core.TDocStd import TDocStd_Document
 from OCC.Core.XCAFApp import XCAFApp_Application
 from OCC.Core.XCAFDoc import (XCAFDoc_DocumentTool_ShapeTool,
                               XCAFDoc_DocumentTool_ColorTool,
@@ -110,14 +110,13 @@ def read_step_file_with_names_colors(filename):
     # the list:
     output_shapes = []
     # create an handle to a document
-    h_doc = Handle_TDocStd_Document()
+    doc = TDocStd_Document()
 
     # Create the application
     app = XCAFApp_Application.GetApplication().GetObject()
-    app.NewDocument(TCollection_ExtendedString("MDTV-CAF"), h_doc)
+    app.NewDocument(TCollection_ExtendedString("MDTV-CAF"), doc)
 
     # Get root assembly
-    doc = h_doc.GetObject()
     h_shape_tool = XCAFDoc_DocumentTool_ShapeTool(doc.Main())
     h_color_tool = XCAFDoc_DocumentTool_ColorTool(doc.Main())
     h_layer_tool = XCAFDoc_DocumentTool_LayerTool(doc.Main())
@@ -131,7 +130,7 @@ def read_step_file_with_names_colors(filename):
 
     status = step_reader.ReadFile(filename)
     if status == IFSelect_RetDone:
-        step_reader.Transfer(doc.GetHandle())
+        step_reader.Transfer(doc)
 
     shape_tool = h_shape_tool.GetObject()
     shape_tool.SetAutoNaming(True)

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

I created my own branch, cherry-picked from yours, to test on my own see review/tp-transparent-handles

Your contribution results in much smaller and readable SWIG files, that's really how things should have been done from the beginning

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

Examples have been ported to this new feature tpaviot/pythonocc-demos#9

@rainman110
Copy link
Collaborator Author

@tpaviot Great that you take care of it!

@rainman110 which os/python version did you use to generate the SWIG files ? I'm running a Linux Ubuntu 18.04 machine, and running the generator creates a huge diff compared to your result.

I created it on windows. I regularly have seen, that the order of includes can be pretty arbitrary. It could also depend on the python version used to run the generator!

@rainman110
Copy link
Collaborator Author

@tpaviot Is there anything, I still have to do? We could also use your new branch now and I will create PRs into this branch, when I have to change anything.

@tpaviot
Copy link
Owner

tpaviot commented Nov 17, 2018

@rainman110 In my branch, I re-generated the swig files using the same machine, you can see that the diffs are more readable. I also noticed a different output from Windows and Linux, I think this comes from the glob function, that does not return the header files in the same order. This might have to do with the OS case sensitivity, but I'm not sure about that.

Currently I've been waiting the travis and appveyor builds to complete, then I'll check if the refactored examples run.

I think the next step is to check if the docstrings have to be modified in the generator as well. I do think that method docstrings refer to Handle_*, and this should be removed to be consistent with the API.

@tpaviot
Copy link
Owner

tpaviot commented Nov 18, 2018

Superseeded by PR #583

@tpaviot tpaviot closed this Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants