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

Conditional geometry should be completely removed from render tree #3452

Closed
t-paul opened this issue Oct 2, 2020 · 5 comments · Fixed by #3555
Closed

Conditional geometry should be completely removed from render tree #3452

t-paul opened this issue Oct 2, 2020 · 5 comments · Fixed by #3555
Projects

Comments

@t-paul
Copy link
Member

t-paul commented Oct 2, 2020

Reported via forum post

intersection() {
    cube();
   
    if(false)
        sphere();
}

The if() should take out the whole subtree, so the result is the cube, not the empty intersection between a cube and an empty object.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@jordanbrown0
Copy link
Contributor

Note that this will in theory affect children-based patterns:

    module foo() {
        children(1);
    }

    foo() {
        if (false) sphere();
        cube();
    }

will generate a cube today, and with this would be an error.

@nophead
Copy link
Member

nophead commented Oct 2, 2020

True, I wonder how many people do that as it looks ambiguous, compared to how many do it in an intersection?

You can either reference the last child instead of the second one or add an else empty() to force an empty child.

Or perhaps if(false) should still add an empty group() node but that should not get converted to an empty() node because it is not the same as empty geometry.

@jordanbrown0
Copy link
Contributor

And since children aren't evaluated until they are referred to with children(), you can't know how many objects they will create until you refer to them, and you can't refer to them because you don't know their indexes because you don't know how many objects their earlier siblings will have returned. And you can't know the value of $children until you've evaluated all of them.

@bneuen
Copy link

bneuen commented Dec 3, 2020

I agree with @nophead here. In the example @jordanbrown0 provided, if you want children(1) to give you an empty(), then you need to add an else that adds the empty. Another way to look at this, is what if you want the if to actually change/determine that first child? So I think the if conditional should completely remove its subtree. I'm running in to this with a module I wrote that worked great on previous releases of OpenSCAD, but is now broken.

@bneuen
Copy link

bneuen commented Dec 3, 2020

So given the current structure of the code, I'm not sure having the if remove its subtree is going to be easily done, so might have to look at what @nophead mentioned.

It seems reasonable to interpret a null geometry differently from an empty geometry group, but I'm not familiar what existing interpretations/assumptions come with these. From what little bit of the code that I have seen, it seems like nullptr (null geometry) is return on most error conditions. Are there cases where an empty geometry group is returned to indicate something?

I think evaluation of a false 'if' expression is currently returning nullptr; so would it be safe to make that an empty group like @nophead said, and then ignore empty groups in operations (certainly the boolean ones, but probably minkowski too).

@t-paul t-paul added this to To do (get into master) in 2021.01 via automation Dec 27, 2020
2021.01 automation moved this from To do (get into master) to Ready (pick to branch pending) Dec 31, 2020
t-paul added a commit that referenced this issue Dec 31, 2020
Don't return empty group for "if" statement with failing condition and no "else" (fixes #3452).
@t-paul t-paul moved this from Ready (pick to branch pending) to Done-RC4 in 2021.01 Dec 31, 2020
@t-paul t-paul mentioned this issue Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2021.01
  
Done-RC4
Development

Successfully merging a pull request may close this issue.

4 participants