BelfrySCAD / BOSL2

The Belfry OpenScad Library, v2.0. An OpenSCAD library of shapes, masks, and manipulators to make working with OpenSCAD easier. BETA
https://github.com/BelfrySCAD/BOSL2/wiki
BSD 2-Clause "Simplified" License
1.01k stars 115 forks source link

zrot_copies ignores cp.z #1442

Closed RAMilewski closed 5 months ago

RAMilewski commented 5 months ago
include<BOSL2/std.scad>
$fn = 32;

zrot_copies(n = 10, cp = [0,0,120], r = 25) color("blue") sphere(2);
Screenshot 2024-06-13 at 1 14 19 AM
adrianVmariano commented 5 months ago

I think this is the correct answer. cp is specifying the location of the axis to rotate around and that axis doesn't depend on cp.z

pipatron commented 5 months ago

This is also exactly what I would expect. What do you think should happen?

RAMilewski commented 5 months ago

OK. But in that case I'm not clear as to why the cp is specified as a [x,y,z] rather than just [x,y]. Consistency with rot_copies, xrot_copies, and yrot_copies maybe. But in the example, it is in fact rotating around [0,0,0], not [0,0,120]. Should the docs clarify that it's not rotating around a point, but about an axis located at [cp.x,cp.y]?

adrianVmariano commented 5 months ago

You don't have to give cp.z, that is, giving just x,y is fine. A zrot() operation is rotating around a line, not a point. There's no meaningful sense in which it is "in fact rotating around [0,0,0]". It's rotating around the line parallel to the Z axis that passes through [0,0,0]. You can the same line if you give a different cp.z value. Or, to turn this around, what would you expect zrot() to do "around the point [0,0,120]"?

RAMilewski commented 5 months ago

For what I was trying to do, it would negate the need to prefix the rotate_sweep() with an up(120). But this late in the game, changing it would just cause backward compatibility problems. Although I would change the docs so it says it's rotating around a z-oriented axis at x,y, instead of saying explicitly (in this case) that it's rotating around a point 120 units above it. If it's ok with you and Revar I can make those changes.

adrianVmariano commented 5 months ago

There is nowhere in the docs where it says you have to give a 3-vector as cp. If you give cp=[3,4] that works fine. The only thing that suggests that cp should be 3d is the listed default of [0,0,0].

Sure, go fix the docs. Presumably you need to change all three rotation distributors to clarify how the axis is determined from cp, and you can note the optionality of cp.z for specifying the axis to zrot_copies. Not optional for the other ones, though.

RAMilewski commented 5 months ago

I've added it to my to-do list right after the Bezier and texturing with images tutorials.