Skip to content

Conversation

@yuval-k
Copy link

@yuval-k yuval-k commented May 5, 2024

Each face can choose a F or M connector. you can also print a snap for F-F connections.

Example:
gridfinity-rebuilt-baseplate

related: #67

@EmperorArthur
Copy link
Collaborator

Neat!

I do have a few suggestions as someone who's recently been in that code pretty heavily. See #187 which is not quite compatible with your PR.

First, is it really worth it to allow choosing each side? Our current code is all or nothing for everything else, and it seems like more trouble than it's worth. I do like the approach of combining them all in a list, and I am doing something similar in the hole cutter code myself.

Second, documentation. I know it's not sexy, but commenting functions and any non-obvious parameters helps us all when reading later. It's saved my but more times than I'd like to admit when I ask myself WTF I was thinking 6 months down the line.

Third, it's probably worth it to replace the "print_snap" with a separate file that calls pattern_linear(...){snap()}. You could move snap into "gridfinity-rebuilt-utility.scad", but that makes having the function and parameters documented even more important.

Fourth, and this is minor, adder_snaps and cutter_snaps don't seem to really do anything useful compared to just calling snaps_on_sides directly. I'd agree with the idea of having cutter and adder interfaces to swap out the holes with snaps, but that's not really something OpenSCAD is designed to do.

I'd also ask if the unit tests ran, but those are also in #187 and are far from complete. Instead would you mind including pictures of what some of the other modes, like with holes, looks like.

@nirvdrum
Copy link

@yuval-k Do you think you'll be continuing development on this PR? If not, would you be open to someone continuing and extending your work? It looks like a great start and I'd love to see something like this integrated. I'm happy to help in any way I can.

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

Successfully merging this pull request may close these issues.

3 participants