Skip to content

Fix the logic in KeepRegion() and change inSame to coincSame for clar… #1410

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.

Sign up for GitHub

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

Closed
phkahler wants to merge 1 commit into from
Closed

Fix the logic in KeepRegion() and change inSame to coincSame for clar… #1410

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/srf/boolean.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,30 +272,33 @@ static bool KeepRegion(SSurface::CombineAs type, bool opA, SShell::Class shell,
{
bool inShell = (shell == SShell::Class::SURF_INSIDE),
outSide = (shell == SShell::Class::SURF_OUTSIDE),
inSame = (shell == SShell::Class::SURF_COINC_SAME),
coincSame = (shell == SShell::Class::SURF_COINC_SAME),
coincOpp = (shell == SShell::Class::SURF_COINC_OPP),
inOrig = (orig == SShell::Class::SURF_INSIDE);

// This one line is not really part of this functions logic
if(!inOrig) return false;
Copy link
Copy Markdown
Member

@ruevs ruevs Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think the "line is not really part of this functions logic"?

Copy link
Copy Markdown
Member Author

@phkahler phkahler Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think the "line is not really part of this functions logic"?

Because keeping a region of a shell should only be dependent on the other flags. There are (at least) two places that call KeepEdge, and one of them preloads the things that become that flag with fixed values. I want to move that logic up to the calling functions so it doesn't even call KeepRegion when it knows that will cause an early exit, but I also want to take baby steps.

One of your other comments seems to indicate a difference of interpretation of opA. My understanding is that KeepRegion is asking "should this surface region of my shell be kept?" where opA is a flag indicating if my shell is operand A of the boolean. In this case regions of opA that are outside of the other shell are kept for difference operations, while regions of operand B ( or not opA) are discarded if they are outside the other shell.

Obviously regions that are coincident same need to be kept for union and intersection, but we have to make a choice to keep the regions from operand A or B, but this decision has to be consistent with other parts of the code. If you change this function to keep these regions with opA instead of not opA it will break some things badly.


switch(type) {
case SSurface::CombineAs::UNION:
if(opA) {
return outSide;
} else {
return outSide || inSame;
return outSide || coincSame;
}

case SSurface::CombineAs::DIFFERENCE:
if(opA) {
return outSide;
return outSide || coincOpp;
} else {
return inShell || inSame;
return inShell;
}

case SSurface::CombineAs::INTERSECTION:
if(opA) {
return inShell;
} else {
return inShell || inSame;
return inShell || coincSame;
}

default: ssassert(false, "Unexpected combine type");
Expand Down