openscad / MCAD

OpenSCAD Parametric CAD Library (LGPL 2.1)
http://reprap.org/wiki/MCAD
578 stars 192 forks source link

Make spin() act more intuitively. Fix duplicate() #28

Closed sarman1998 closed 9 years ago

sarman1998 commented 9 years ago

spin() doesn't work the way I expected which is also implied by duplicate() and linear_multiply. Default behavior now is to space $no - 1 duplicates evenly around $axis. If $angle is given, rotate $no-1 items around $axis by $angle. children(0) is not moved. To easily visualize, you can add this line above the rotate(... line color( [(1/no)_i,(1/no)_i,(1/no)*i ])

hyperair commented 9 years ago

Hmm, I don't completely agree with the behavioural change you're proposing.

The original documentation says spin() should spin an item evenly around an axis, for the angle specified. In other words, spin(5, 360, Z) should give you 5 items rotated around Z evenly, i.e. (360/5)°. Two rounds would be 720°, and the spacing would be (720/5)°. The angle parameter should be the total rotation, rather than the $fa equivalent.

On the other hand, I agree that children(0) should not be moved.

In any case, it looks like it's broken, and behaves in a rather erratic manner. I propose this change instead.

diff --git a/array/along_curve.scad b/array/along_curve.scad
index 432594d..ead6fac 100644
--- a/array/along_curve.scad
+++ b/array/along_curve.scad
@@ -6,15 +6,13 @@
  */

 include <MCAD/units/metric.scad>
-use <MCAD/general/utilities.scad>

 // TODO check that the axis parameter works as intended
 // Duplicate everything $no of times around an $axis, for $angle/360 rounds
 module spin(no, angle=360, axis=Z){
-   for (i = [1:no]){
-       rotate(normalized_axis(axis)*angle*no/i) union(){
-           for (i = [0 : $children-1]) children(i);
-       }
+   for (i = [0:no-1]){
+       rotate(angle*i/(no-1), axis)
+                children ();
    }
 }

With this change,

use <MCAD/array/along_curve.scad>

spin (3, 90)
translate ([5, 0, 0])
cylinder (d = 1, $fn = 20);

becomes screenshot18

What do you think?

sarman1998 commented 9 years ago

That behavior seems reasonable, but I still think the behavior when angle is 360 is non-intuative

spin (3)
translate ([5, 0, 0])
cylinder (d = 1, $fn = 20);

becomes this, where there are 3 objects, 2 in the same location.

image

Also, do we have to worry about the div/zero if someone sets no=1? How do you feel about changing the comment which to me says that spin() will give me $no more objects.

hyperair commented 9 years ago

Hmm, okay, this gets tricky. I guess your method gives us a less ambiguous API, which would be good for the new MCAD API I'm working on (new symbols are renamed to something like mcad_foo_bar_baz instead of fooBarBaz or foo_bar_baz for consistency and namespacing). The comment still seems a bit confusing though. How about:

// Place children $no times around $axis, with the first duplicate being unmoved
// from its original spot. $angle is the angle of rotation between children(n)
// and children(n-1).

In any case, this change won't be compatible with the old API (as per the master branch), causing old designs to be broken. I'll merge this with some API migration changes I guess.