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

Don't return empty group for "if" statement with failing condition and no "else". #3555

Merged
merged 1 commit into from
Dec 31, 2020

Conversation

thehans
Copy link
Member

@thehans thehans commented Dec 26, 2020

fixes #3452

@jordanbrown0
Copy link
Contributor

Does a failing "if" count as a child?

module count() {
    echo($children);
}

count() {
    if (false) {
        cube();
    }
}

@thehans
Copy link
Member Author

thehans commented Dec 26, 2020

Yes it does.

$children is currently based on AST nodes, not CSG tree, because it needs to be determined before/during the expression evaluation stage (ie it needs to be known at the time that if conditions will be evaluated, so it can't really depend on results of those conditions). Also see #3143

@t-paul t-paul merged commit 7618d4d into openscad:master Dec 31, 2020
@nophead
Copy link
Member

nophead commented Dec 31, 2020

What about the true if case when the child is not geometry, i.e. echo?

@thehans
Copy link
Member Author

thehans commented Jan 1, 2021

An if with true conditional will group any and all children as usual. If the children don't happen to contain geometry then the result will be the empty set due to implicit union on group nodes.

If you think groups without geometry shouldn't count as empty geometry, then you're basically asking for the lazy-union feature.

@nophead
Copy link
Member

nophead commented Jan 1, 2021

Does echo return a special "no geometry" node? If so, can it be propagated upwards so that if(true) echo() behaves the same as echo() and a module that wraps echo() behaves like echo() in an intersection?

I don't think it makes sense that echo() and if(false) echo() don't create an empty group but if(true) echo() does.

@jordanbrown0
Copy link
Contributor

I'm very nervous about deviating from the "one module invocation, one geometry node" rule, but I agree that once you do deviate from it for echo() and if(false) ..., you should also deviate from it for if(true)echo(); and similar. I suspect this also extends to for(...)echo and modules containing no "real" geometry.

Note that constructs that do contain real geometry, but that result in emptiness - intersections of non-overlapping objects, in particular - should continue to be represented as "real" geometry nodes.

@thehans
Copy link
Member Author

thehans commented Jan 1, 2021

So far, group nodes have basically functioned identically to union.
But it sounds like what people are asking for is that any group with no geometry nodes inside should be treated as a special case and removed from the render tree instead.
This might be feasible to implement without full lazy union support, but I also worry that this could open another can of worms of unintended consequences and regressions. Especially this late in a release cycle.

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

Successfully merging this pull request may close these issues.

Conditional geometry should be completely removed from render tree
4 participants