Skip to content

Conversation

@zjiang0529
Copy link
Contributor

Hi! We have pushed a first version of adding multi-geometries features.
(Issue #109)
Thanks.

Copy link
Collaborator

@mtezzele mtezzele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests in order to verify that everything is working fine.
The occ version currently supported by pygem is 0.17. Let me know if you need the last version.

TopAbs_FORWARD, TopAbs_SHELL)
from OCC.TopExp import (TopExp_Explorer, topexp)
from OCC.gp import (gp_Pnt, gp_XYZ)
from OCC.BRep import *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See quantified code for suggestion on how to avoid the import using *

from OCC.gp import (gp_Pnt, gp_XYZ)
from OCC.BRep import *
from OCC.TColgp import (TColgp_Array1OfPnt, TColgp_Array2OfPnt)
from OCC.BRepOffsetAPI import *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

self.write_shape_to_file(compound, self.outfile)


def parse_face(self, topo_face):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You never use self in this method. Make it a static method (see quantified code). Also the methods below have the same problem.

:return: mesh_points_edge: it is a list of `n_points`-by-3 matrix
:rtype: numpy.ndarray and list
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is a dictionary

:param topo_face: the input Face
:return: mesh_points_face: it is a `n_points`-by-3 matrix containing the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge these two returns by creating a single variable that will be returned. Please check also the other methods.

@mtezzele
Copy link
Collaborator

Any news @zjiang0529 ?

@zjiang0529
Copy link
Contributor Author

Hello, thanks for the remind. We are going to commit a new version soon.

@zjiang0529
Copy link
Contributor Author

Hello, we have just committed some modification.
Now we think it would be time to discuss how to integrate and merge
with the parse() and write() functions in Nurbshandler class.

@mtezzele
Copy link
Collaborator

@zjiang0529 please add some tests in order to verify that everything is working fine.

@mtezzele mtezzele closed this Jun 16, 2017
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.

3 participants