inputlabs / prototypes_cad

Other
2 stars 5 forks source link

build123d discussion and reimplementation of existing `wheel.py` file #35

Closed jdegenstein closed 6 months ago

jdegenstein commented 6 months ago

First of all, great work on the design so far. I had a lot of fun reimplementing it using the builder API along with quite a few changes to the approach. There was only one small change to the object geometry (AFAIK) related to the tolerance between the core and core cutout (described below).

image

Here are my general observations:

  1. Use of build123d-builtins can often make the code a lot simpler and less error-prone. In particular I was able to create the same star-wheel shape with almost zero trigonometry necessary. I did this through the use of a construction line and PolarLocations (essentially a polar pattern, also consider GridLocations for grid/linear patterns in the future).
  2. Use of planes for splitting objects is often a superior approach than e.g. subtracting Rectangles as done in wheel.py since all you need to know is the plane + offset
  3. Use of offset to apply tolerances. This enables re-use of an existing sketch as I did in the included core_s sketch. The only change to the existing actual geometry occurred here by the way -- the existing wheel.py did not have tolerance applied to one of the cutout edges (the long one that roughly divided the core_s sketch into a semicircle).
  4. The benefit of my approach to the Trapezoidal holder is debatable, but I included it in this version for your consideration. The existing approach is a pretty good idea too.
  5. Using a built-in slot object to cut the axle "rest" is superior to the existing approach
  6. Another big discussion is when it is best to reference existing geometry as part of creating new geometry. I personally tend to prefer avoiding referencing geometry, but there are really good arguments on both sides. From a pragmatic standpoint, referencing existing geometry like e.g. with BuildSketch(someface) ... can have the unintended consequence of also changing the center position. Instead I used e.g. with BuildSketch(Plane.XY.offset(width + contactRightWidth)) which is referenced against the same variables used to create the extrusion distances. Also, referencing existing geometry also requires robust selectors, and might be impacted by code reorganization. On the positive side referencing existing geometry definitely does capture the design intent more clearly.
  7. builder vs. algebra APIs -- this is a big topic as well but in the end both APIs are equally valid IMHO. In the existing algebra-based wheel.py the number of lines is similar or slightly less than this reworked version. I would argue that my implementation is probably easier to read and probably easier for understanding how the design evolves with version control.
  8. Star import -- I used it here but it is worth considering how the script will be used and the risks involved -- further discussion from build123d's perspective here https://build123d.readthedocs.io/en/latest/tips.html#isnt-from-build123d-import-bad-practice
  9. I included some STEP export statements as well
  10. I have opened an issue to add a property to RegularPolygon to access minor_radius instead of needing to use cos(pi/6) as done here. https://github.com/gumyr/build123d/issues/585