SolidCode / SolidPython

A python frontend for solid modelling that compiles to OpenSCAD
1.1k stars 171 forks source link

Unexpected behavior with hole and hull? #190

Open c-mauderer opened 2 years ago

c-mauderer commented 2 years ago

Hello,

I'm not sure whether the behavior is expected or not. But for me it was a bit of a surprise: If I do a hull on two objects with holes, the result is a hull of the object minus a hull of the holes. To make it more clear: Here is an example:

#!/usr/bin/env python3

from solid import *
from solid.utils import *

c = cylinder(r=10) - hole()(cylinder(r=5))
x = hull()(c, left(10)(c))
print(scad_render(x))

This generates one long hole:

difference(){
        hull() {
                difference() {
                        cylinder(r = 10);
                }
                translate(v = [-10, 0, 0]) {
                        difference() {
                                cylinder(r = 10);
                        }
                }
        }
        /* Holes Below*/
        hull(){
                union(){
                        cylinder(r = 5);
                }
                translate(v = [-10, 0, 0]){
                        union(){
                                cylinder(r = 5);
                        }
                }
        } /* End Holes */ 
}

Screenshot

I would have expected two round holes:


difference(){
        hull() {
                difference() {
                        cylinder(r = 10);
                }
                translate(v = [-10, 0, 0]) {
                        difference() {
                                cylinder(r = 10);
                        }
                }
        }
        /* Holes Below*/
        union(){
                cylinder(r = 5);
        }
        translate(v = [-10, 0, 0]){
                union(){
                        cylinder(r = 5);
                }
        } /* End Holes */ 
}

Screenshot

Is that expected behavior?

etjones commented 2 years ago

I agree with your analysis; two circular holes is what we would expect intuitively. What's happening is that the hull() invocation is getting added to the negative holes as well as the positive areas, and that isn't what we expect.

Let me have a think on this. We want this hull() to apply to the positive cylinders and NOT to the holes. But we could very reasonably want a hole that included a hull of negative space as well.

c-mauderer commented 2 years ago

Let me have a think on this. We want this hull() to apply to the positive cylinders and NOT to the holes. But we could very reasonably want a hole that included a hull of negative space as well.

That's correct. For example the following slightly modified code should generate one long "bar" with two slotted holes at the end. Instead it generates only one big hole with rounded edges.

#!/usr/bin/env python3

from solid import *
from solid.utils import *

slot = hull()([back(i)(cylinder(r=2)) for i in [-2, 2]])
c = cylinder(r=10) - hole()(slot)
x = hull()(c, left(50)(c))
print(scad_render(x))
jeff-dh commented 2 years ago

The code basicly traverses the graph twice to render a part (or the root object) containing holes. The first traversal renders everything but the holes, the second traversal renders only the holes.

To fix this issue I think we would need to create a list of all holes while traversing the tree and render each hole seperately (including all parent transforms) afterwards, create a union and substract it from the root object.

[edit: aeh aeh..... the formerly proposed work around can't work....]

etjones commented 2 years ago

The hole() mechanism does basically what you’ve proposed, Jeff. In this particular situation, we’d need to disambiguate between: a hole() with hull() children (where the hull should persist in what’s subtracted) and a hull() with hole() grandchildren, where the hull should only apply to positive geometry.

We could check for exactly that circumstance, but it feels hacky, like I don’t have a theoretical grasp of what’s going on there. That was some of the problem with the original implementation (there’s some “I think this works” comments in the source) but I’ve never fully understood the mechanics of how the hole setup should work in all situations. :-/

c-mauderer commented 2 years ago

Thanks for the explanations and that you have a look at all these problems. I don't know the code well enough that I can really help a lot solving the problem. I just noted that I have some more edge cases where I'm really not sure about what to expect from holes (for example using an object with a hole as a hole to some other object - there are multiple reasonable interpretations what's right in this case). Is there some documentation about how the holes should work in different situations?

PS: I solved my problem a bit different so that I don't need a workaround at the moment.

etjones commented 2 years ago

I’m glad you’ve got a workaround, Christian. I’ll keep thinking through how this and other edge cases would work. The issue is that I came up with a simple(-ish) solution to a common problem, cavities that would disappear when combined with other objects. But, as you’ve found, I didn’t really work out all the edge cases.

Maybe what I’m looking for is “after all positive shapes are added, subtrees marked with hole() should be subtracted. hole() subtrees should be affected by translational nodes (translate(), rotate(), multmatrix()), but not by CSG nodes ( union(), difference(), intersection())”

hull() alters geometry without being included in either the translational or CSG nodes I listed. It might be sufficient to add it to the list of ignored CSG nodes, but I need to think through how other one-off nodes like minkowski() should apply.

On Nov 28, 2021, at 9:28 AM, Christian Mauderer @.***> wrote:

 Thanks for the explanations and that you have a look at all these problems. I don't know the code well enough that I can really help a lot solving the problem. I just noted that I have some more edge cases where I'm really not sure about what to expect from holes (for example using an object with a hole as a hole to some other object - there are multiple reasonable interpretations what's right in this case). Is there some documentation about how the holes should work in different situations?

PS: I solved my problem a bit different so that I don't need a workaround at the moment.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

jeff-dh commented 2 years ago

On Sun, 28 Nov 2021 07:20:22 -0800 Evan Jones @.***> wrote:

The hole() mechanism does basically what you’ve proposed, Jeff. In this particular situation, we’d need to disambiguate between: a hole() with hull() children (where the hull should persist in what’s subtracted) and a hull() with hole() grandchildren, where the hull should only apply to positive geometry.

I don't think so, I express it as pseudo code:

hull(object - hole1 - hole2)

renders atm to:

hull(object) - hull(hole1 + hole2)

but if it would render to:

hull(object) - hull(hole1) - hull(hole2)

(and hole1 and hole2 could contain hulls because they would be treated as regular "entities").

everything would be fine I guess. I had a look at the code and couldn't come up with a easy code change to implement it. It would become a little(!) tricky to implement it because it would need some book keeping in _render_hole_children.

And well..... BOSL2 ;)

If you (Christian) are motivated you could have a look at this branch and check out how the BOSL2 version handles your cases and whether that's more what you expecet. I would be really interested whether it does. (Be aware you need to use the BOSL2 primitives)

https://github.com/jeff-dh/SolidPython/tree/master-2.0.0-beta-dev

https://github.com/jeff-dh/SolidPython/blob/5ec2aebe48e876343957d8c53ec88ea451023602/solid/examples/07-libs-bosl2.py#L53

jeff-dh commented 2 years ago
#! /usr/bin/env python

from solid import *
from solid.extensions.bosl2.std import cube, zcyl, diff

def bosl2_diff_hull():
    obj = cube([100, 100, 10], _tags="pos")
    hole1 = zcyl(d=10, l=100, _tags="neg").translate(10, 10, 0)
    hole2 = zcyl(d=10, l=100, _tags="neg").translate(50, 50, 0)

    return diff("neg", "pos")(hull()(obj + hole1 + hole2))

bosl2_diff_hull().save_as_scad()

handles it the same (unexpected) way as the solidpython implementation

jeff-dh commented 2 years ago

hull(object) - hull(hole1) - hull(hole2)

hmmmm this would create new "unexpected cases" if the holes are not convex shapes.....

Your approach seems to be the way to go....

jeff-dh commented 2 years ago

Try to change line solid/solidpython.py:212 like this:

    #from this:
    if self.name in non_rendered_classes:
        pass

    #to this:
    if self.name in non_rendered_classes or self.name == 'hull':
        pass

This seems to work at least for this examples:

from solid import *

c = cube([100, 100, 10])

h1 = translate([20, 20, -20])(cylinder(r=10, h=100))
h2 = translate([30, 30, -20])(cylinder(r=10, h=100))

h3 = hull()(translate([50, 50, 0])(h1 + h2))

hh = hull()(c - hole()(h1) - hole()(h2) - hole()(h3))

scad_render_to_file(hh)
jeff-dh commented 2 years ago

@etjones I'm with you concerning other transformations. I was also wondering whether there might be more. But we could extend the line I proposed above to if not self.name in translationalNodesList: pass that might do the trick.

etjones commented 2 years ago

I think there may be a little more to it: does your change differentiate between hull(a, b - hole()(c)) (where the hull should be excluded from the hole) and a - hole()(hull(b,c)), (where the hull should be included)? I don’t think the current tree traversal pays attention to whether objects are above or below a given hole object in the hierarchy.

Beyond that, I hacked this feature into place back in the day, and before I make any changes to fix it, I’d like a solution that includes all the corner cases. I’m not sure my positional vs mutating differentiation is complete.

On Nov 28, 2021, at 3:37 PM, jeff-dh @.***> wrote:

 @etjones I'm with you concerning other transformations. I was also wondering whether there might be more. But we could extend the line I proposed above to if not self.name in translationalNodesList: pass that might do the trick.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

jeff-dh commented 2 years ago

I think there may be a little more to it: does your change differentiate between hull(a, b - hole()(c)) (where the hull should be excluded from the hole) and a - hole()(hull(b,c)), (where the hull should be included)? I don’t think the current tree traversal pays attention to whether objects are above or below a given hole object in the hierarchy.

If I understand it correctly, yeah it does.

Take a look at the example snippet a posted above. This is the result of that code, which I think is what we would expect: hullholes

etjones commented 2 years ago

Excellent. Well, that’s a bit closer to a solution.