Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior with hole and hull? #190

Open
c-mauderer opened this issue Nov 27, 2021 · 14 comments
Open

Unexpected behavior with hole and hull? #190

c-mauderer opened this issue Nov 27, 2021 · 14 comments

Comments

@c-mauderer
Copy link

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
Copy link
Contributor

etjones commented Nov 27, 2021

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
Copy link
Author

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
Copy link
Contributor

jeff-dh commented Nov 28, 2021 via email

@etjones
Copy link
Contributor

etjones commented Nov 28, 2021

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
Copy link
Author

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
Copy link
Contributor

etjones commented Nov 28, 2021 via email

@jeff-dh
Copy link
Contributor

jeff-dh commented Nov 28, 2021 via email

@jeff-dh
Copy link
Contributor

jeff-dh commented Nov 28, 2021

#! /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
Copy link
Contributor

jeff-dh commented Nov 28, 2021

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
Copy link
Contributor

jeff-dh commented Nov 28, 2021

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
Copy link
Contributor

jeff-dh commented Nov 28, 2021

@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
Copy link
Contributor

etjones commented Nov 28, 2021 via email

@jeff-dh
Copy link
Contributor

jeff-dh commented Nov 28, 2021

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
Copy link
Contributor

etjones commented Nov 28, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants