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.

Sign up for GitHub

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

Add hash eq and neq operations to TShape #1375

Closed
wants to merge 2 commits into from
Closed

Add hash eq and neq operations to TShape #1375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b220ed2
Add hash eq and neq operations to TShape
JohannesVerherstraeten Oct 21, 2024
95c0f02
Merge branch 'tpaviot:master' into fix-tshape-hash
JohannesVerherstraeten Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/SWIG_files/wrapper/TopoDS.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1980,11 +1980,44 @@ Returns the type as a term of the shapeenum enum: vertex, edge, wire, face, ....
") ShapeType;
virtual TopAbs_ShapeEnum ShapeType();


%extend{
bool __ne_wrapper__(const opencascade::handle<TopoDS_TShape> & other) {
if (self!=other) return true;
else return false;
}
}
%pythoncode {
def __ne__(self, right):
try:
return self.__ne_wrapper__(right)
except:
return True
Comment on lines +1991 to +1995
Copy link

Choose a reason for hiding this comment

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

suggestion (bug_risk): Consider using more specific exception handling in ne and eq methods.

The current implementation catches all exceptions and returns a default value. This might hide unexpected errors. Consider catching specific exceptions that you expect might occur during the comparison.

Suggested change
def __ne__(self, right):
try:
return self.__ne_wrapper__(right)
except:
return True
def __ne__(self, right):
try:
return self.__ne_wrapper__(right)
except (TypeError, AttributeError):
return True

Copy link
Author

Choose a reason for hiding this comment

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

I copied these method implementations from TopoDS_Shape, but it's true that the try-except is a bit weird.

Unless there is a specific reason to do it this way, I would remove the try-except from both the Shape and TShape implementations.

Copy link

Choose a reason for hiding this comment

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

Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:

- Avoid suggesting changes that are not directly related to the functionality or correctness of the code.
- Ensure that suggestions are necessary and improve the code's robustness or readability.
- Do not include suggestions that might introduce unnecessary complexity or are based on personal preference unless they address a clear issue.

Copy link
Owner

Choose a reason for hiding this comment

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

Without the try except statement, something like:

a_topods_shape != "Hi!"

would segfault because the right member is not a TopoDS_Shape. Using ``ìsinstance``` would certainly be better

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes I see. I'll make a new commit using isinstance

}

%extend{
bool __eq_wrapper__(const opencascade::handle<TopoDS_TShape> & other) {
if (self==other) return true;
else return false;
}
}
%pythoncode {
def __eq__(self, right):
try:
return self.__eq_wrapper__(right)
except:
return False
}
};


%make_alias(TopoDS_TShape)

%extend TopoDS_TShape {
size_t __hash__() {
return opencascade::hash(self);}
};

Copy link
Owner

Choose a reason for hiding this comment

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

The TopoDS_Shape inherits from Standard_Transient, I think the __hash__ method could be moved to the base class, and would also benefit to all other classes derived from Standard_Transient

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I'll move the __hash__ to Standard_Transient together with the __eq__ and __neq__ methods.

Copy link
Owner

Choose a reason for hiding this comment

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

no need to, I did it there's a build going on

%extend TopoDS_TShape {
%pythoncode {
__repr__ = _dumps_object
Expand Down
32 changes: 32 additions & 0 deletions test/test_core_wrapper_features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,38 @@ def test_neq_operator() -> None:
assert shape_1 != "some_string"


def test_tshape_hash_operator() -> None:
shape_1 = BRepPrimAPI_MakeBox(10, 20, 30).Shape()
shape_2 = BRepPrimAPI_MakeBox(10, 20, 30).Shape()
# Create a shape that differs from shape_2, but has the same TShape
shape_3 = shape_2.Reversed()
assert hash(shape_1.TShape()) == hash(shape_1.TShape())
assert not hash(shape_1.TShape()) == hash(shape_2.TShape())
assert hash(shape_2.TShape()) == hash(shape_3.TShape())


def test_tshape_eq_operator() -> None:
shape_1 = BRepPrimAPI_MakeBox(10, 20, 30).Shape()
shape_2 = BRepPrimAPI_MakeBox(10, 20, 30).Shape()
# Create a shape that differs from shape_2, but has the same TShape
shape_3 = shape_2.Reversed()
assert shape_1.TShape() == shape_1.TShape()
assert not shape_1.TShape() == shape_2.TShape()
assert shape_2.TShape() == shape_3.TShape()
assert not shape_1.TShape() == "some_string"


def test_tshape_neq_operator() -> None:
shape_1 = BRepPrimAPI_MakeBox(10, 20, 30).Shape()
shape_2 = BRepPrimAPI_MakeBox(10, 20, 30).Shape()
# Create a shape that differs from shape_2, but has the same TShape
shape_3 = shape_2.Reversed()
assert not shape_1.TShape() != shape_1.TShape()
assert shape_1.TShape() != shape_2.TShape()
assert not shape_2.TShape() != shape_3.TShape()
assert shape_1.TShape() != "some_string"


def test_inherit_topods_shape() -> None:
class InheritEdge(TopoDS_Edge):
def __init__(self, edge: TopoDS_Edge) -> None:
Expand Down