checkgeometry->bopcheck in background thread

[TheMarkster]

IMHO: I would not add a parameter for controlling the multithread of the bopcheck. I would just enable it for the occt versions that have it and let it ride. Can always add it later if it is needed for some strange reason.

I fully agree!. AFAIK some OCCT algorithms (e.g. BRepMesh_IncrementalMesh) are parallelized for some years now and we have enabled it there too by hard-coding it.

I'm onboard with it. Full speed ahead, damn the torpedoes and all that. After a few tests I don't think my own CPU temps have entered the danger zone. I also noticed during a compile of freecad with vs2017 my cpu was at 100% for quite a while, so this is probably something that happens routinely anyway. Latest PR has the parameter removed. Only change to the original code, far as I know, is setting setrunparallel() to true. Everything else should still be the same. Merge and close when you're ready. I'm gonna give up on running as a background task at this time. The goal is to make freecad more stable, not less stable, and I think backgrounding this task wouldn't be furthering that goal.

If I can work out how to use a progress indicator with bopcheck I'll start a new PR for that. I'd really like to see progress indicators with cancel options used more in freecad. Users having to force kill freecad with a task manager because something is taking too long and because they're not sure if it's an infinite loop or if it's just taking a long time to do... is just not a good look. Even if a cancel option isn't there just the reassurance that the process is still ongoing would be a big step in the right direction.

[wmayer]

I'd really like to see progress indicators with cancel options used more in freecad

The problem with OCCT is that there are only a few algorithms where a progress indicator can be set.

[tanderson69]

I'd really like to see progress indicators with cancel options used more in freecad

The problem with OCCT is that there are only a few algorithms where a progress indicator can be set.

BopAlgo_Options will take a progress indicator and the BopAlgo_ArgumentAnalyzer is a subclass. So it looks like there is a chance that it works(occt 7.3), but I haven't tried.https://dev.opencascade.org/doc/refman/ ... lyzer.html

[TheMarkster]

I'd really like to see progress indicators with cancel options used more in freecad

The problem with OCCT is that there are only a few algorithms where a progress indicator can be set.

BopAlgo_Options will take a progress indicator and the BopAlgo_ArgumentAnalyzer is a subclass. So it looks like there is a chance that it works(occt 7.3), but I haven't tried.https://dev.opencascade.org/doc/refman/ ... lyzer.html

I'm not sure if it's implemented. Here is the relevant source code from BOPAlgo_Options.cxx:

Code: Select all

 130 //=======================================================================
 131 //function : SetProgressIndicator
 132 //purpose  : 
 133 //=======================================================================
 134 void BOPAlgo_Options::SetProgressIndicator
 135   (const Handle(Message_ProgressIndicator)& theObj)
 136 {
 137   if (!theObj.IsNull()) {
 138     myProgressIndicator = theObj;
 139   }
 140 }
 141 //=======================================================================
 142 //function : UserBreak
 143 //purpose  : 
 144 //=======================================================================
 145 void BOPAlgo_Options::UserBreak() const
 146 {
 147   if (myProgressIndicator.IsNull()) {
 148     return;
 149   }
 150   if (myProgressIndicator->UserBreak()) {
 151     throw Standard_NotImplemented("BOPAlgo_Options::UserBreak(), method is not implemented");
 152   }
 153 }

UserBreak() simply returns void if the pi is null and throws a not implemented exception otherwise. Could be subclasses, such as BOPalgo_ArgumentAnalyzer are expected to override and implement this themselves?

Here is the implementation of UserBreak() in Message_ProgressIndicator.cxx:

Code: Select all

 165 //=======================================================================
 166 //function : UserBreak
 167 //purpose  : 
 168 //=======================================================================
 169 
 170 Standard_Boolean Message_ProgressIndicator::UserBreak () 
 171 {
 172   return Standard_False;
 173 }
 174     

It returns false in all cases and makes no effort to check if the user canceled.

Seems to me you should be able to call BOPCheck_ArgumentAnalyzer::SetProgressIndicator() passing it the handle to the progress indicator and it would update it periodically during Perform(), but I don't see any code (maybe I'm just missing it) that would do any such thing. A call *is* made to UserBreak() between each check during the Perform() operation, but its return value is ignored. Maybe the expectation is it throws an exception to stop the operation? Otherwise the only thing it could do to stop Perform() would be to put something in myResult and set myStopOnFirst to true, which is the kind of hackery I might would do, but not professional coders. I'm thinking (and hoping to be wrong) it's just incomplete code, but I think it has been 2 years, so not much hope of it getting completed any time soon.

Here is the Perform() function from BOPAlgo_ArgumentAnalyzer.cxx:

Code: Select all

// ================================================================================
 152 // function: Perform
 153 // purpose:
 154 // ================================================================================
 155 void BOPAlgo_ArgumentAnalyzer::Perform()
 156 {
 157   try {
 158     OCC_CATCH_SIGNALS
 159     myResult.Clear();
 160     //
 161     UserBreak();
 162     //
 163     // 1. Prepare
 164     Prepare();
 165     //
 166     UserBreak();
 167     //
 168     // 2. Test types
 169     if(myArgumentTypeMode) {
 170       TestTypes();
 171     }
 172     //
 173     UserBreak();
 174     //
 175     // 3. Test self-interference
 176     if(mySelfInterMode) {
 177       TestSelfInterferences();
 178     }
 179     //
 180     UserBreak();
 181     //
 182     // 4. Test small edges
 183     if(mySmallEdgeMode) {
 184       if(!(!myResult.IsEmpty() && myStopOnFirst))
 185         TestSmallEdge();
 186     }
 187     //
 188     UserBreak();
 189     //
 190     // 5. Test possibility to rebuild faces
 191     if(myRebuildFaceMode) {
 192       if(!(!myResult.IsEmpty() && myStopOnFirst))
 193         TestRebuildFace();
 194     }
 195     //
 196     UserBreak();
 197     //
 198     // 6. Test tangent
 199     if(myTangentMode) {
 200       if(!(!myResult.IsEmpty() && myStopOnFirst))
 201         TestTangent();
 202     }
 203     //
 204     UserBreak();
 205     //
 206     // 7. Test merge vertices
 207     if(myMergeVertexMode) {
 208       if(!(!myResult.IsEmpty() && myStopOnFirst))
 209         TestMergeVertex();
 210     }
 211     //
 212     UserBreak();
 213     //
 214     // 8. Test merge edges
 215     if(myMergeEdgeMode) {
 216       if(!(!myResult.IsEmpty() && myStopOnFirst))
 217         TestMergeEdge();
 218     }
 219     //
 220     UserBreak();
 221     //
 222     // 9. Test shapes continuity
 223     if(myContinuityMode) {
 224       if(!(!myResult.IsEmpty() && myStopOnFirst))
 225         TestContinuity();
 226     }
 227     //
 228     UserBreak();
 229     //
 230     // 10. Test validity of the curves on the surfaces
 231     if(myCurveOnSurfaceMode) {
 232       if(!(!myResult.IsEmpty() && myStopOnFirst))
 233         TestCurveOnSurface();
 234     }
 235   }
 236   catch(Standard_Failure) {
 237     BOPAlgo_CheckResult aResult;
 238     aResult.SetCheckStatus(BOPAlgo_CheckUnknown);
 239     myResult.Append(aResult);
 240   }
 241 }

As a sidenote:
Notice: myStopOnFirst refers to the stopOnFirstFaulty() option, so I think that option really does halt the check on finding the first fault. (Stands to reason since we wouldn't be getting fewer results with it enabled.) Problem could be, that first check is the one that is consuming the most time (just a hunch, nothing to back that up with except for your findings earlier than using the option wasn't speeding things up any). If the first check is the costly one, then stopping after that check doesn't really speed things up. And if no faults are found at all, then obviously it won't help in those cases, either.

Another sidenote: I experimented with removing the copy operation and everything still worked. Could be an older version of occt needed it? I'm not really seeing any performance benefits from removing it, but maybe the objects I tested it don't take long to copy. Should be faster if we remove the copy stuff, I can't see how it wouldn't be (unless maybe something in the copy process makes the bopcheck run faster) but the risk would be it might break things for some users.

I didn't see UserBreak() defined in BOPAlgo_Analzyer.cxx, so I guess the parent class (BOPAlgo_Options) is the one that gets run. But if that's the case I did try creating an instance of a progress indicator and passing the handle to SetProgressIndicator():

Handle(Message_ProgressIndicator) pi = new Part::ProgressIndicator(100);
BOPCheck.SetProgressIndicator(pi);
BOPCheck.Perform()

Seems to me I should have gotten a not implemented exception unless my handle is null that I'm passing in, (but I'm pretty sure that's not the case, read on.)

I removed that code and tried another tactic: showing progress in between checks for cases where multiple objects have been selected to be checked. I do have a (semi-working) progress indicator that updates as each check finishes, so that's why I don't think I was passing a null handle. Relevant code in TaskGeometryCheck.cpp:

Code: Select all

#include "../App/ProgressIndicator.h"
#include <QApplication>

Code: Select all

void TaskCheckGeometryResults::goCheck()
{
    Gui::WaitCursor wc;
    int selectedCount(0), checkedCount(0), invalidShapes(0);
    std::vector<Gui::SelectionSingleton::SelObj> selection = Gui::Selection().getSelection();
    std::vector<Gui::SelectionSingleton::SelObj>::iterator it;
    ResultEntry *theRoot = new ResultEntry();
    Handle(Message_ProgressIndicator) pi = new Part::ProgressIndicator(selection.size());

    for (it = selection.begin(); it != selection.end(); ++it)
    {
        std::string msg = std::string("Checking ") + selection[selectedCount].DocName + std::string(".") + selection[selectedCount].FeatName+std::string("...");
        pi->NewScope(selectedCount, msg.c_str());
        qApp->processEvents();
        pi->Show();


        selectedCount++;
        Part::Feature *feature = dynamic_cast<Part::Feature *>((*it).pObject);
        if (!feature)
            continue;
            
        /*remaining code is unchanged until the bottom of the for loop, where I have this: */
        pi->EndScope();
        

bopcheck-pi.gif (224.13 KiB) Viewed 1940 times

[wmayer]

Here is the implementation of UserBreak() in Message_ProgressIndicator.cxx:

There it's supposed to use a subclass of Message_ProgressIndicator. In Part/App we have this subclass ProgressIndicator which on UserBreak does something else. Our implementation internally uses Base::SequencerLauncher and this shows a Qt progress bar if the GUI is up which is able to react on ESC key events.

[wmayer]

The way how Message_ProgressIndicator is used inside the BOP algo is a bit weird. It does never increase the current progress but instead always invokes UserBreak. So, in this case we have to implement a new sub-class where we can e.g. show a QProgressDialog (with an undetermined range) and update it e.g. only every 500ms. The FreeCAD class ProgressIndicator cannot be used for this.

[TheMarkster]

Another angle I'm looking into is calling BOPCheck.Perform() multiple times, running each individual test type separately. This is easily controlled by setting the test type parameters to false for all but the test type we want to run. We could then check for user cancellation ourselves between each call to BOPCheck.Perform() and cancel at that time if the user wants to cancel. The problem is the one test that seems to be taking up the lion's share of the time is the self-interference test, which is probably also the most useful of the 9 tests. The other 8 tests seems to perform very quickly (at least with the very limited testing I've done with BOPCheck.SelfInterMode() = false).

If it's true the self-intersection test is the one that always uses up all the time, then really there's no benefit to even having a progress indicator with a user break. BOPCheck.Perform() isn't checking for UserBreak() *during* any of the tests, it's checking in *between* tests. It would be useful where each test, more or less, took the same amount of time, but that doesn't appear to be the case. If somebody wants to verify this, it would be appreciated. Compile with the BOPCheck.SetInterMode()=true commented out. It defaults to false. Check some models that show multiple error types and that take a long time to check.

If I'm correct in this (and I could certainly be wrong) then perhaps it would better to make RunBOPCheck default to true and have a parameter for checking for self-intersections and have it defaulted to false. Or simply use the RunBOPCheck parameter to decide whether to check for self-intersections since users already know to enable RunBOPCheck to get more complete results from the geometry check.

[wmayer]

In git commit d5c63cf211 I have added a progress dialog where the user can abort the task when it takes too long.

[TheMarkster]

Awesome work!

I think it's likely checks for UserBreak() *are* being done inside the tests themselves and not just in between tests. Here is some code from BOPArgumentAnalyzer::TestSelfInterferences():

Code: Select all

 356     BOPCol_ListOfShape anArgs;
 357     BOPAlgo_CheckerSI aChecker;
 358     //
 359     anArgs.Append(aS);
 360     aChecker.SetArguments(anArgs);
 361     aChecker.SetNonDestructive(Standard_True);
 362     aChecker.SetRunParallel(myRunParallel);
 363     aChecker.SetFuzzyValue(myFuzzyValue);
 364     aChecker.SetProgressIndicator(myProgressIndicator);
 365     //
 366     aChecker.Perform();
 367     Standard_Boolean hasError = aChecker.HasErrors();

It looks like the aChecker object is doing most of the heavy lifting for the self-intersection test. And aChecker does call UserBreak().

[Kunda1]

We should mention something in the 0.18 Release page on the wiki about this very cool feature