build123d icon indicating copy to clipboard operation
build123d copied to clipboard

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

Open bernhard-42 opened this issue 1 year ago • 8 comments

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://github.com/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

bernhard-42 avatar Jul 07 '24 14:07 bernhard-42

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 avatar Sep 03 '24 18:09 jdegenstein

closing with https://github.com/gumyr/build123d/commit/ba348d5d985e5b23a698f99f1ab1150c6be13aa4

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

jdegenstein avatar Sep 07 '24 14:09 jdegenstein

@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 avatar Sep 08 '24 10:09 bernhard-42

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

jdegenstein avatar Sep 08 '24 15:09 jdegenstein

no worries and thank you!

bernhard-42 avatar Sep 08 '24 16:09 bernhard-42

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

jdegenstein avatar Sep 17 '24 15:09 jdegenstein

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

bernhard-42 avatar Sep 17 '24 21:09 bernhard-42

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.

jdegenstein avatar Sep 19 '24 21:09 jdegenstein