rsheldiii / openSCAD-projects

Repository to archive my openSCAD endeavors (and YASP endeavors, if I get that done)
GNU Lesser General Public License v3.0
54 stars 19 forks source link

Fixed brimmed(), minor enhancements #22

Closed MylesMetzler closed 6 years ago

MylesMetzler commented 6 years ago

Fixed:

Added:

rsheldiii commented 6 years ago

Thanks for making this PR! looks like a lot of good work, sorry the brim feature was broken :/

I want this library to be easy to understand, modify and extend for personal use, so I'm going to have to be a bit of a stickler as to what I allow in. the brimming work is fantastic, that's definitely going to go in. I like the idea of having multiple rows of text, and the text feature has definitely been something I've been looking at improving, but I was thinking of doing multiple text positions (four corners and the center) and having those be separate variables. maybe revive the legend function so that doesn't pollute the key function's argument signature.

The base key function is nice, but uses search, so it's not compatible with the current stable release of openSCAD, which I believe is what's used on Thingiverse's servers. it's also tied in to the split-string approach of doing things, so if we were to do the text positions it'd have to be modified but not by much.

The bump is really cool but I'm not sure if I want to be that opinionated about features in key.scad. Perhaps I could spin off a features() folder and features.scad for stuff like that - inset bumps, dxf renderers and such.

If you could cut this PR down to the brim stuff, or submit a new PR with the brim stuff in it, I'll happily approve that. If I made a features folder, would you be cool with having the homing bump in there?

MylesMetzler commented 6 years ago

Removed everything except the brimmed fix/enhancement.

MylesMetzler commented 6 years ago

I'd be fine with placing the homing bump anywhere. I was adding it as its something I consider a requirement for the project I am working on.

Having 4 corners + center would definitely be the way to go for the text (I'm playing with that now actually...)

MylesMetzler commented 6 years ago

See commit for legend implementation with positioning and multiple labels: dcs_row(3) cherry() legend("X", "top", "left", 3) legend("X") key(true); image

rsheldiii commented 6 years ago

thanks for doing this! just wanted to give a quick update, I'll get around to merging this soon. just been hard to find time after work :)

rsheldiii commented 6 years ago

Hey thanks again for this! going to be merging now, pushing for a release soon.

If it's alright with you I'm going to follow up on this with a PR to change just a few things, mostly switching to a placement array for the legends (so like, [0, 0] for center and [-1, -1] for top left etc) and removing the redox keys file.

I'm also going to follow up with the features folder too! seems like a good idea.

rsheldiii commented 6 years ago

oh wow, that's a neat trick with the for loop in the legends function

rsheldiii commented 6 years ago

ah, and I understand the problem with siloing the bump out of key.scad. I'm realizing that the way I'm approaching additions / subtractions to the keycap is suboptimal - not sure what the best way is given the limitations of openSCAD but ideally you'd be able to choose inset / outset per object. I could enforce 2 children to the key() function, one for additive and one for subtractive, but I'm not a huge fan of that since it's rather unintuitive. I'm gonna leave the bump mostly untouched for now, but I'm hoping in the future to make stuff like this super flexible

rsheldiii commented 6 years ago

https://github.com/rsheldiii/openSCAD-projects/pull/27