revarbat / BOSL

The Belfry OpenScad Library - A library of tools, shapes, and helpers to make OpenScad easier to use.
https://github.com/revarbat/BOSL/wiki
BSD 2-Clause "Simplified" License
557 stars 62 forks source link

bottom_half: cp arg is confusing #46

Closed adrianVmariano closed 5 years ago

adrianVmariano commented 5 years ago

It took me about an hour to figure out that the cp scalar argument goes in the opposite direction as cp=[0,0,z]. I find this very surprising and am not sure what the motivation is here. I expect scalar cp to be a way to specify the z coordinate without the nuisance of giving the unnecessary x and y values.

Also note that the implementation is puzzling: why do you shift the children down and then shift them up instead of just shifting the cube by the adjusted amount?

revarbat commented 5 years ago

That would be a bug, not a feature.

revarbat commented 5 years ago

Actually, scalar cp in negative directional halves is arguably correct. It actually matches what the documentation states. But yes, the multiple translates are silly. I can remove two translates in each half function.

adrianVmariano commented 5 years ago

I know it matches the documentation. But it does not make intuitive sense to me. It took me a while to realize that the documentation described this unexpected behavior. So why do it like that?

revarbat commented 5 years ago

It’s moving the cut plane in the vector direction of the halving.

revarbat commented 5 years ago

All the *_half() modules share the same code with only a difference of the vector direction for the cut mask. Thus the scalar cp is defined in terms of that vector. If your getting the bottom_half(), the scalar cp is moving the cut plane away from the origin towards the bottom. It seemed fully consistent to me.

revarbat commented 5 years ago

Another way to think of it is that a positive scalar cp always masks away more of the part being halved.

revarbat commented 5 years ago

I could see an argument for inverting that, to make positive scalar cp make the part larger.

To be honest, having the scalar cp as just a shortcut for the coordinate cp didn’t even occur to me, as the scalar implied distance instead of position, and therefore the direction would be implied by the orientation of the cut.

adrianVmariano commented 5 years ago

I completely understand what you have done. I just find it unnatural, and unexpected (as indicated by the length of time I took me to figure it out...even after consulting the documentation, which is not unclear). I guess I just don't feel like there is an implied direction. I don't imagine this as a translation operation but as cutting at a designated position. To me it seems like

union() { top_half(cp=A) thing(); bottom_half(cp=A) thing() }

should be the whole thing(). I don't really understand the utility of the cp=[dummy1,dummy2,z] notation. I guess if you wanted to make a cube corner or something it would be handy, but mostly it seems like extra clutter. I think I probably would have done it as top_half(z=cut_location), right_half(x=location), left_half(x=location), etc.

revarbat commented 5 years ago

I guess it's because, in my head, it's all generalized from the half_of() module, where cp is an arbitrary point on the not-necessarily axis oriented cut plane. I can see merit in the x, y, or z notation, though.

revarbat commented 5 years ago

Added x, y, and z args to *_half() modules in BOSLv1 master branch commit 8c0f97953b1f565ed56285e572a543084bd75156

revarbat commented 5 years ago

Implemented in BOSL2 master branch commit a6a2ed520bbd834a02005f7a803f74e3eda636a8