SolidCode / SolidPython

A python frontend for solid modelling that compiles to OpenSCAD
1.12k stars 174 forks source link

extrude_along_path contains gaps in rotation between first and last point #201

Closed ChrisCooper closed 1 year ago

ChrisCooper commented 1 year ago

I don't believe this is really a bug in the implementation of extrude_along_path, but it is inconvenient and I would love a way around it. (Or pointers on what I'm doing wrong!)

When using extrude_along_path to make a simple rotation, for example to make a torus, the first and last faces in the path are at the "wrong" rotation. It seems like they just face the direction of the one neighboring point, which leaves a gap in a completed loop. I see this making sense if connect_ends=False, but when connect_ends=True, it seems better to treat the points as a loop rather than a path with geometry connecting the ends.

I modified the "No Scale" example from the examples file to have a lower num_points in the path, which highlights the issue.

connect_ends=False connect_ends=False

connect_ends=True connect_ends=True

If this behavior can't be changed due to backwards compatibility concerns, perhaps I could add a simple new function to replicate what rotate_extrude does in OpenSCAD? Having to manually define a circle_points function, then use it extrude, seems like extra work compared to how simple that built-in function is.

Alternatively, if there's something simple I've overlooked to fix this, I'd love to know!

etjones commented 1 year ago

Thanks for such a good write-up. I’ll have to look at the code a little, but I think this is really a bug; you’re absolutely right that connect_ends should take tangency into account. I can’t remember offhand if this is something I accounted for originally, and which isn’t working right, or if I just overlooked this originally. I’ll report back once I give it a look

ChrisCooper commented 1 year ago

Awesome! Thanks for such a quick response

On Fri, Jan 6, 2023, 8:40 PM Evan Jones @.***> wrote:

Thanks for such a good write-up. I’ll have to look at the code a little, but I think this is really a bug; you’re absolutely right that connect_ends should take tangency into account. I can’t remember offhand if this is something I accounted for originally, and which isn’t working right, or if I just overlooked this originally. I’ll report back once I give it a look

— Reply to this email directly, view it on GitHub https://github.com/SolidCode/SolidPython/issues/201#issuecomment-1374339261, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACTY4ML4K3SIP6FBINVNP3WRDCR3ANCNFSM6AAAAAATTTXNDM . You are receiving this because you authored the thread.Message ID: @.***>

etjones commented 1 year ago

Hmm. I've looked at this a little and I need to a little thinking about what correct behavior should be.

There are a few different cases to consider

I think case A makes sense with current behavior; end caps are normal to the first/last segments

Current behavior:

A: Planes of first/last section are normal to the line between them

Screen Shot 2023-01-09 at 1 31 16 PM

B: Planes of first/last section are normal to the first segment/ last segment

Screen Shot 2023-01-09 at 1 38 27 PM

C: Planes of first/last section are normal to the line between them (But we'd prefer behavior A)

Screen Shot 2023-01-09 at 1 38 40 PM

D: Planes of first/last section are normal to the first segment/ last segment (But we'd prefer replacing first/last sections with one section normal to the line between 2nd & 2nd-to-last sections)

Screen Shot 2023-01-09 at 1 38 50 PM

The two examples you showed, both of which look wonky, seem to be case C & D. I think the desired behavior is that both C & D should look like smooth rings, as if they'd been created with rotate_extrude().

So I think the new rule should be: if the first and last points on the rail curve are the same, close the shape, and let the first/last faces lie normal to the line between the 2nd & 2nd-to-last points.

That should be a single-line code change. I'm wondering if there's a simpler way to conceptualize this though.

ChrisCooper commented 1 year ago

Would a simpler way be to say that when connect_ends=True , there isn't really a "first" or "last" point, and it's instead treated as a ring? That would make it so to make a pentagon, you'd pass 5 rail curve points, to make a hexagon, you'd pass 6, etc. which feels more familiar than giving an extra identical point.

I.e. These rail curves would produce identical results when connect_ends=True:

path_pts = [(1,2,3), (4,5,6), (7,8,9), (10,11,12)]
path_pts = [(4,5,6), (7,8,9), (10,11,12), (1,2,3)]
path_pts = [(7,8,9), (10,11,12), (1,2,3), (4,5,6)]
path_pts = [(10,11,12), (1,2,3), (4,5,6), (7,8,9)]
ChrisCooper commented 1 year ago

Woohoo! Thanks for getting to this