SolidCode / SolidPython

A python frontend for solid modelling that compiles to OpenSCAD
1.12k stars 174 forks source link

Calls to OpenSCAD code imported into SolidPython can only be used as CSG objects #111

Closed n-e-g closed 3 years ago

n-e-g commented 5 years ago

When function is imported, especially function that renders into array, generated code is incorrect. Example: import file transform.scad which contain function

function translation(translation=[], x=0, y=0, z=0) = [
    [1, 0, 0, is_undef(translation[0]) ? x : translation[0])],
    [0, 1, 0, is_undef(translation[1]) ? x : translation[1])],
    [0, 0, 1, is_undef(translation[2]) ? x : translation[2])],
    [0, 0, 0, 1]
];

use transform scad:

c = cube([20, 20, 30])
t = translation(x=10, y=20, z=30)
d = multmatrix(t)(c)
print(scad_render(d))

leads to the following output:

multmatrix(m = <__main__.translation object at 0x7fbba4f22240>) {
    cube(size = [20, 20, 30]);
}

which is obviously incorrect

etjones commented 5 years ago

Yep, that's definitely not right. I think this is a recent regression. Should be a quick fix.

etjones commented 5 years ago

Nope, not a regression. There's actually nothing in the code to use imported OpenSCAD code as parameters to functions (e.g. multmatrix( translation([xyz])(c) rather than members of the body of an OpenSCAD block, like union()(translation([x,y,z])). But... that's an artificial limitation. Working on a fix

Equidamoid commented 4 years ago

@etjones any news? Or maybe some a dev branch to play around with? =)

Equidamoid commented 4 years ago

I took a shot at it and... removing one semicolon is hard. If someone wants a quick and dirty hack, here you go:

diff --git a/solid/solidpython.py b/solid/solidpython.py
index 020ceaf..fc90174 100755
--- a/solid/solidpython.py
+++ b/solid/solidpython.py
@@ -759,6 +759,8 @@ from . import objects

 def py2openscad(o: Union[bool, float, str, Iterable]) -> str:
+    if isinstance(o, OpenSCADObject):
+        return o._render()[:-1]
     if type(o) == bool:
         return str(o).lower()
     if type(o) == float:

Getting rid of [:-1] the proper way is a bit too much for me right here right now :(

etjones commented 4 years ago

This suggestion will work for a single OpenSCAD object, like n-e-g's original prompt. But if the argument to multmatrix() were a whole tree object, then you would need to have arbitrarily long OpenSCAD code (including blocks & semicolons) in the arguments to other OpenSCAD functions. I'm not convinced that's a feature that would add a lot to this system.

So, removing the semicolon from a single object works. Using the original example:

c = cube([20, 20, 30])
t = translation(x=10, y=20, z=30)
d = multmatrix(t)(c)
print(scad_render(d))

would yield:

multmatrix(m = 
translation(x = 10, y = 20, z = 30)) {
        cube(size = [20, 20, 30]);
}

Despite some unclear indentation, this would work.

But if the transform we pass to multmatrix is more complex:

c = cube([20, 20, 30])
t = translate([1,2,3])(rotate(a=[90,45,0])(translation(x=10, y=20, z=30)))
d = multmatrix(t)(c)
print(scad_render(d))

yields arbitrarily complex OpenSCAD code in the args to multmatrix:

multmatrix(m = 
translate(v = [1, 2, 3]) {
        rotate(a = [90, 45, 0]) {
                translation(x = 10, y = 20, z = 30);
        }
) {
        cube(size = [20, 20, 30]);
}

Assuming we could fix the indentation & internal semicolons, this still seems pretty convoluted to me. I'm inclined to close this as a wontfix, but maybe there are some reasons for this feature I haven't thought through all the way. Can you tell me why you'd find it useful to pass OpenSCAD objects as function arguments to others?

Equidamoid commented 4 years ago

@etjones, here is my usecase: BOSL library contains modules for gears. Now, as I make a couple of gears, I need to know how far away to position them. To achieve that, BOSL has a function pitch_radius which accepts the same agruments as gear module that creates a gear. So in OpenSCAD I write code like this:

gear(<gear1_parameters>);
translate([pitch_radius(<gear1_parameters>) + pitch_radius(gear2_parameters), 0, 0])
  gear(<gear2_parameters);

and get 2 perfectly spaced gears (or, similarly, places to put axes). My problem here is having ugly copypaste of <gearX_parameters>. So, I turned to SolidPython.

Here this transforms to something like this:

g1_params = dict(<gear1_params>)
g2_params = dict(<gear1_params>)

x = union()(
    g.gear(**g1_params),
    translate(g.pitch_radius(**g1_params) + g.pitch_radius(**g2_params))(
      g.gear(**g2_params)
    )
)

, which doesn't work.

Equidamoid commented 4 years ago

I suspect this approach (module + some functions returning parameters of this module) may be widespread in existing OpenSCAD code as there is no way to get derived values (like "radius of a gear with X teeth and pitch of Y") without making such functions. And these functions will probably pretty often be used in transforms.

offtopic: I solved this problem for now by partially porting the OpenSCAD gear code to python and exposing the values as additional attributes of the OpenSCADObject instances I return. This way, however, is not really sustainable for everyone since such porting cound be a pretty annoying pain in the back.

etjones commented 4 years ago

Thanks for that! I think that's a strong argument in favor of solving this bug. I'll take a look at this today.

etjones commented 4 years ago

Hmm. In both the original issue example and in yours, we're calling OpenSCAD code to return non-solid values, a matrix and a number respectively. It absolutely makes sense that we'd want to be able to use those pre-made calculations without having to re-write them in Python.

...Aaaand, I'm not really sure how to make that work just yet. Currently a line like:

pr = pitch_radius(**g1_params) + pitch_radius(**g1_params)

yields this OpenSCAD:

union(){
    pitch_radius(**g1_params);
    pitch_radius(**g1_params);
};

since __add__() is overloaded to create a union() in SolidPython. We might be able to get around that if we allowed imported SCAD objects to be cast to floats, say. But sometimes a SCAD function returns a number, sometimes a data structure like the matrix in the original example, and sometimes a 3D OpenSCAD object like involute_gears.gear() does.

The assumption in SolidPython has always been that imported SCAD objects are CSG objects. This issue points out why that was a mistaken assumption. Hmm...

Equidamoid commented 4 years ago

@etjones Whoops, I accidentally made it even more complicated then I thought... This case has a relatively easy workaround to avoid arithmetics: do 2 translate()s

My hack worked only for non-arithmetic case where gears were moved along different axes:

union()(
    g.gear(**g1_in_params),
    translate([0, 0, th])(
        g.gear(**g1_out_params)
    ),
    translate([g.pitch_radius(**g1_in_params), 0, g.pitch_radius(**motor_gear_params)])(
        rotate([0, -90, 0])(
            g.gear(**motor_gear_params)
        )
    )
)

Doing proper expression handling feels like waaaay too much work (probably comparable to rewriting OpenSCAD to just use python directly :D)

etjones commented 3 years ago

I've released a fix for this issue, now available via Pip in SolidPython v1.0.3. The fix doesn't resolve some of the deeper issues I described above of composite objects all specified in arguments to some other object, but for most uses of "This calculation was done in OpenSCAD and I want to use it in SolidPython", this should take care of things. Sorry for the long, long wait