gumyr / cq_warehouse

A cadquery parametric part collection
Apache License 2.0
107 stars 23 forks source link

Modified extension line to allow for projected dimensioning. #52

Closed slobberingant closed 2 years ago

slobberingant commented 2 years ago

This is my first ever pull request. Let me know if I did something wrong. Had a crack at fixing issue #51. extension_line now takes a project_line variable which is a vector direction to project against. This allows 'horizontal' or 'vertical' dimensions as are commonly used in CAD packages. I have added a _project_wire method to accomplish this. Looking forward to feedback.

slobberingant commented 2 years ago

An example of a normal, horizontal and vertical dimension.

projected-dimension

gumyr commented 2 years ago

It's nice to see that Draft is getting used :) Thanks for the PR!

It has been about a year since I looked at this code so it will take me a little while to dive into into it again but at first glance this looks good. One minor thing I noticed project_line: Vector = None could be replaced with project_line: VectorLike = None and a <vectorized project_line> = Vector(project_line) internally which would allow the user to just enter a tuple for the direction.

Are you up to adding a unit test?

slobberingant commented 2 years ago

I'm using Cadquery for metal fabrication so Draft is necessary to communicate designs internally with welding staff or externally to clients. I would have application for Cadquery if I couldn't annotate dimensions.

I need some hand holding with this PR. I don't have any formal training in this space.

VectorLike is a good idea. I will review. I will also attempt to write a unit test.

I have a few questions regarding this PR.

  1. The _project_wire function I have added finds the startPoint and endPoint of a wire and projects that to project_line. This just dumbs down the wire to two points which may defeat the purpose of using a wire in the first place. Do you see this approach as an issue? Does Cadquery have a better built in approach to project a Wire?

  2. Lines 618 and 619 where the 3d vector is converted to a two-tuple seem wrong. Is there a neater way to do this?

  3. The lines of code that create the extension lines (620 to 629) have copied the original offset code which used a 1.5mm and 3mm fixed value. I don't think that this offset should be hard coded. It should either be passed to the function (eg. dimension_offset as opposed to extension_offset) with a 3mm default or calculated based on overall size of the item being dimensioned. When the parts become very large, 3mm gap is not visible. What are your thoughts on this?

gumyr commented 2 years ago

I'm very happy that you're using and willing to improve cq_warehouse; don't worry about the details of the PR we'll figure it out together. As someone who is using Draft for real; I appreciate your guidance in figuring out what's the right thing to do here.

1) Edges aren't necessarily straight lines - like this is diagram image but there is code already there to find a circle (or part of a circle) so line 589-606 are handling non circles and creates a straight line which may be a diagonal line. I would expect that diagonals should be allowed and your improvement would be adding a pair of lines aligned with the X and Y axis. 2) Vector(1, 2, 3).toTuple()[:2] 3) Hard-coding does seem like a bad idea. How about adding new attributes to the Draft class?

slobberingant commented 2 years ago

Changes have been made as per our discussion. Looking forward to feedback.

gumyr commented 2 years ago

Sorry to take so long - I was way behind on cq_warehouse stuff. I'll try to get this merged ASAP. PS. please make the PRs against the dev branch not main - but I think I should be able to fix it.

slobberingant commented 2 years ago

No worries. I'll make sure future PRs are against the dev branch.

gumyr commented 2 years ago

I reviewed your code and it seems good except for one thing - the offsets when creating horizontal and vertical dimension lines are global values not relative to the selected edge. Here is an example:

import cadquery as cq
from cq_warehouse.drafting import Draft

# Create an object with two diagonal edges and extract them
target = (
    cq.Workplane("XY").sketch().trapezoid(80, 60, 120, angle=180).finalize().extrude(-1)
)
top_left_edge = target.faces(">Z").edges("<X").val()
top_right_edge = target.faces(">Z").edges(">X").val()

# Initialize the Drafting class - with a new custom extension_gap
metric_drawing = Draft(decimal_precision=1, extension_gap=2)

# Create a diagonal extension line
left_extension_line = metric_drawing.extension_line(
    object_edge=top_left_edge, offset=10
)

# Create two perpendicular extension lines
right_horizontal_extension_line = metric_drawing.extension_line(
    # object_edge=top_right_edge, offset=40, project_line=(1, 0)
    object_edge=top_right_edge,
    offset=0,
    project_line=(1, 0),
)
right_vertical_extension_line = metric_drawing.extension_line(
    # object_edge=top_right_edge, offset=85, project_line=(0, 1)
    object_edge=top_right_edge,
    offset=0,
    project_line=(0, 1),
)

# If running from within the cq-editor, show the extension lines
if "show_object" in locals():
    show_object(target, name="target")
    show_object(top_left_edge, name="bottom_left_edge")
    show_object(top_right_edge, name="bottom_right_edge")
    show_object(left_extension_line, name="left_extension_line")
    show_object(right_horizontal_extension_line, name="right_horizontal_extension_line")
    show_object(right_vertical_extension_line, name="right_vertical_extension_line")

which creates this: image

For the diagonal extension line the offset of 10 is relative to the line itself. To get the new lines in the "correct" position I had to use the commented out lines which uses global values and therefore won't automatically move with the part if it changes dimensions.

I do find it can be difficult to get the extension lines to go where one wants them to be. Did you find it easier to use global values?

slobberingant commented 2 years ago

I didn't realise I was using global values. The test part I was using was a centered box so my testing was too simplistic to pick it up! The values definitely need to be relative. A decision will have to be made as to what point to use as the start of the dimension line (first point, mid point of edge or last point). I will attempt to fix this and report back.

slobberingant commented 1 year ago

I have pushed some new code for this branch. Your example code now creates the dimensions as shown below. The extension line is now more robust. See commit message for specifics.

fixed-extension

Should I create a new pull request?