search

vector76 / gridfinity_openscad

MIT License
274 stars 49 forks source link

modularized root cups #10

Closed benzvan closed 2 years ago

benzvan commented 2 years ago

I'd love for the root objects like gridfinity_basic_cup to be useable as modules in other projects. This would mean moving some of the variables like height, length, width, withLabel, etc. into parameters in the basic_cup module. The existing variables could be changed to default_length, etc. and used as default values.

I'm happy to open a PR if this seems worthwhile to you.

benzvan commented 2 years ago

I guess as I look at this a little more, it would just be basic_cup and baseplate that would make sense. The others are purpose built objects.

vector76 commented 2 years ago

I do generally prefer the approach of using parameters instead of global variables because it's more reusable that way. I think this is a positive direction and I would be interested to see the specifics in a PR.

benzvan commented 2 years ago

Cool. I'll get something going.

benzvan commented 2 years ago

Here's my idea: https://github.com/vector76/gridfinity_openscad/pull/11

I also locally created options for a shorter label tab and to truncate the cups for printing larger than the available print area that I could submit.

vector76 commented 2 years ago

Thank you for the PR. I agree with propagating the parameters instead of using global variables, but there are some aspects that I'm not sure about. The variables are called defaults, but they are really overrides and the defaults (values used if the model were imported with a use statement) are still magic numbers in the module definition.

I took your idea and went for a deeper change, where the cup is promoted to its own file in the modules folder, and the basic gridfinity_basic_cup still exists and references it. The defaults live in the module file, and the overrides are in the client file. Having the cup as a module that can be used is also perhaps more inviting for modified versions.

I haven't made this official yet. I committed 755f032b601f20839cadca3ea7b952fcb8463a32 to a branch and I'm interested in your opinion.

vector76 commented 2 years ago

Resolved by #12 Thanks for the input, this was a good idea.