Fix the logic in KeepRegion() and change inSame to coincSame for clar…#1410
Fix the logic in KeepRegion() and change inSame to coincSame for clar…#1410phkahler wants to merge 1 commit intosolvespace:masterfrom
Conversation
|
It makes sense. The original logic for difference was wrong. Then I simplified it here #484 keeping the (original wrong) logic. Then when I implemented intersection in #595 (correctly it seems in this particular aspect) I realized that difference was "more broken" than intersection, but could not figure it out. Your change makes sense and it actually improves the difference operation on these models: The model I created while implementing intersection #35 (comment)IntersectionReallyReallyReallyNastyTest_DifferenceFailsBecauseItShould.slvs Before
After
An "improvement" of the above test I have not posted on GitHub until now:IntersectionReallyReallyReallyReallyNastyTest_DifferenceFailsBecauseItShould.zip Before
After
|
| inOrig = (orig == SShell::Class::SURF_INSIDE); | ||
|
|
||
| // This one line is not really part of this functions logic | ||
| if(!inOrig) return false; |
There was a problem hiding this comment.
Why do you think the "line is not really part of this functions logic"?
There was a problem hiding this comment.
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.
|
Merged |
...by fixing the logic in KeepRegion() to properly keep/discard regions where two surfaces coincide. Now difference works as well as intersection in this respect. Change inSame to coincSame for clarity.
…ity.
This makes some real improvement on @rpavlik bug_repro file from issue #297. Still not fixed, but fewer missing surface areas.