problem with object tree; a bug or a feature?

[nickc92]

So I've encountered an issue which I don't know is a bug or a feature; the following code fails:

a = cube()
b = a + translate([1,0,0]) (a)
c = b + translate([0,1,0]) (b)
print(scad_render(c))

The reason seems to be due to what I think is an optimization: on line 3, we are creating a union of b with translate([0,1,0]) (b). But b itself is a union() object, so the __add__ magic method of the union class is being called. I'm guessing this is an optimization to prevent things like union() { union() { union() {... appearing in the rendered OpenSCAD code, but it has the unfortunate effect of making code like mine above fail. By commenting out the __add__() method of the union class, and letting things fall back to OpenSCADObject.__add__(), things seem to work. I don't know if this problem exists with other classes besides union, but it may.

Although the above code is contrived to make a simple example of the problem, it comes from real use cases; I've encountered it twice now. One can get around the problem by working in the functional, OpenSCAD style:

def a(): return cube()
def b(): return a() + translate([1,0,0]) (a())
c = b() + translate([0,1,0]) (b())
print(scad_render(c))

But I wasn't sure if it was intended for the user to be forced into this functional style.

[nickc92]

No thoughts?

[etjones]

Yes thoughts! Just... life; sorry for the delay.

BUT! You are exactly correct about the optimization and the reason for it. And, it looks like, about the problem it causes. I’m going to need to take a look at the code later this week, but it looks like there are two possibilities:

A) remove the optimization entirely and accept the extra SCAD code generated (e.g. union{ union{ union{a,b}, c}, d} rather than union{ a,b,c,d})

B) Can we think through the problem all the way down and preserve the optimization while avoiding the issues with multiple copies-by-reference? I think this might be possible by doing a deep copy on self-referential assignments (a = right(10)(a)), But that’s only a vague idea.

I’ll take a look at this issue and see if I can come up with anything; any ideas you’ve got would be very welcome, too!

[nickc92]

You're right- it would be nice to have the cake and eat it too; it would be great if you could keep the optimization and avoid the problems it is causing. Let me think about it too; though truth be told, it's one of those problems that makes my head hurt!

[nickc92]

Perhaps one thing that might work is if the __add__() method of union could scan the set of all child nodes, and in the event that one of the children == self, then it creates a new union object; otherwise, it acts as it is coded right now. It seems like this would prevent an OpenSCADObject from being a child of itself, which I think is what is causing the trouble with the infinite regress.

[nickc92]

So I think we can have our cake and eat it too. I replaced the union.__add__() method with this:

def __add__(self, x):
     new_union = union()
     for child in self.children:
         new_union.add(child)
     new_union.add(x)

     return new_union

And I think it accomplishes what we want. I think a fundamental problem with the way union.__add__() was before is that it modified the object itself. So if b is a union() object, and we say c = b + b, it modifies the b object; but that is not really what is intended I think. Give it a think, and let me know what you think! I created a new pull request for this issue.

[nickc92]

Did anyone try my suggested fix, with the simple replacement of union.__add__() like above? I've been using SolidPython with this fix for a while, and I haven't run into problems.

[etjones]

You're right. Just merged. Thanks!