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

Use non-optimal bounding box to determine size of Joint symbol #657

Closed
bernhard-42 opened this issue Jul 7, 2024 · 8 comments
Closed

Use non-optimal bounding box to determine size of Joint symbol #657

bernhard-42 opened this issue Jul 7, 2024 · 8 comments
Assignees

Comments

@bernhard-42
Copy link
Collaborator

Currently, build123d uses the optimal bounding box for determining the size a Joint symbol.
Even for not so complex objects this can easily take 0.5 to 1 sec. If you have 5 joints on such an object, building the symbol in build123d easily takes several seconds. As an example, use https:/bernhard-42/bd_animation/blob/main/examples/engine.py for the animation group crank

I'd say Shape.bounding_box needs to get the optimal=True keyword. This would keep the current behaviour, but allow in symbol ,to set optimal to False

@jdegenstein
Copy link
Collaborator

Is this still a change you would like to see? Seems like a fairly simple one since Shape.bounding_box depends on BoundBox._from_topo_ds which does have the optimal keyword.

@jdegenstein
Copy link
Collaborator

closing with ba348d5

@bernhard-42 can you give it a try and let us know if this works as expected from a performance standpoint?

@bernhard-42
Copy link
Collaborator Author

@jdegenstein Thanks for fixing the parameter. It is also needed to tell the symbol methods to use parameter False

diff --git a/src/build123d/joints.py b/src/build123d/joints.py
index bf2b8ca..0c21842 100644
--- a/src/build123d/joints.py
+++ b/src/build123d/joints.py
@@ -69,7 +69,7 @@ class RigidJoint(Joint):
     @property
     def symbol(self) -> Compound:
         """A CAD symbol (XYZ indicator) as bound to part"""
-        size = self.parent.bounding_box().diagonal / 12
+        size = self.parent.bounding_box(optimal=False).diagonal / 12
         return Compound.make_triad(axes_scale=size).locate(self.location)

     def __init__(
@@ -228,7 +228,7 @@ class RevoluteJoint(Joint):
     @property
     def symbol(self) -> Compound:
         """A CAD symbol representing the axis of rotation as bound to part"""
-        radius = self.parent.bounding_box().diagonal / 30
+        radius = self.parent.bounding_box(optimal=False).diagonal / 30

         return Compound(
             [
@@ -652,7 +652,7 @@ class BallJoint(Joint):
     @property
     def symbol(self) -> Compound:
         """A CAD symbol representing joint as bound to part"""
-        radius = self.parent.bounding_box().diagonal / 30
+        radius = self.parent.bounding_box(optimal=False).diagonal / 30
         circle_x = Edge.make_circle(radius, self.angle_reference)
         circle_y = Edge.make_circle(radius, self.angle_reference.rotated((90, 0, 0)))
         circle_z = Edge.make_circle(radius, self.angle_reference.rotated((0, 90, 0)))

Without, the symbol still uses the default for optimal which is True

@bernhard-42 bernhard-42 reopened this Sep 8, 2024
@jdegenstein
Copy link
Collaborator

@bernhard-42 Sorry about closing prematurely, I will open another PR to add these changes too

@bernhard-42
Copy link
Collaborator Author

no worries and thank you!

@jdegenstein
Copy link
Collaborator

@bernhard-42 I just made the changes you provided above, can you confirm if they are working in the latest dev?

@bernhard-42
Copy link
Collaborator Author

I am away from any keyboard for the next 2 weeks.
I can earliest test that end of Sept / beginning of Oct.

@jdegenstein
Copy link
Collaborator

I did a quick test of the optimal keyword, and I am seeing results that are actually slower for the optimal=False case. I think we should discuss this further as I am wondering what scenarios optimal=False should be faster.

Not a rush, but just wanted to share what I observed.

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

No branches or pull requests

2 participants