Mesh - Progress indication and user break functionality for BRepMesh component

Branch [archived branch] has been created by Author.

[revision removed]

Branch CR25113 is ready to be reviewed.
Corresponding branch has also be pushed to products repository.

User break/progress indication functionality has been integrated to BRepMesh using code base applied by drazmyslovich.

Draw command incmesh has also been changed in order to provide necessary options for mesh algorithm (angular tolerance, relative flag and new option timeout intended to test user break functionality).

Branch [archived branch] has been updated by Author.

[revision removed]


      from 7346490 0025113: Progress indication and user break functionality for BRepMesh component
       new c6632be small correction of grammar


Detailed log of new commits:

[revision removed]
Author: Author
Date: Fri Aug 1 19:03:47 2014 +0400

    small correction of grammar

Oleg, I have a number of remarks:

- There are a lot of formatting changes (e.g. in BRepMesh_FastDiscret::Add()), I cannot be sure to identify all functional changes

- In BRepMesh_FastDiscretFace::AddInShape, try {} block lacks OCC_CATCH_SIGNALS in the beginning (it was the same before, but worth fixing anyway)

- Using exceptions to implement user break is rather controversial approach, it may be both dangerous if some resources are not released (or e.g. shape is not consistently updated) and inconvenient for debugging. I suggest you to consider more plain approach the progress indicator was designed for: just implement graceful exit from each function if UserBreak is signaled. Apart of safer exit, it may allow you to preserve model consistency keep at least some computed data.

- I suggest you to try the prototype multi-threaded version of the progress indicator, to be able to use it more naturally in the parallel code of BRepMesh. Note that in this case you would deal with Sentry objects rather than progress indicator handle, passing them in function arguments (i.e. not constructors).

At the end, would not it be logical to apply this change when you have your current effort on refactoring of BRepMesh completed?

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Fri Sep 26 17:05:20 2014 +0400

    0025113: Progress indication and user break functionality for BRepMesh component

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Dec 4 18:53:04 2014 +0300

    0025113: Progress indication and user break functionality for BRepMesh component
    
    Draft implementation of multi-thread progress indicator

Branch CR25113_2 is ready to be reviewed.

Support of progress indicator or timeout would be highly appreciated (see https://dev.opencascade.org/index.php?q=node/1185).

Current patch is not actual and will be implemented after integration of #0026106 feature waiting for review some time...

Parallel progress indicator is supposed for implementation within separate #0025748 issue and, IMHO, base version using mutexes should be intergrated first.

Dear Oleg, please take care of the issue after integration of patch for #0025748.

Andrey, please complete the patch.

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

src/BRepMesh/BRepMesh_ConstrainedBaseMeshAlgo.hxx
- 55: wrap long line

src/BRepMesh/BRepMesh_DelaunayDeflectionControlMeshAlgo.hxx
- 50, 69, 103: wrap long line
- 102: I think the cycle on triangles split is quick and you need no updating progress in it.

src/BRepMesh/BRepMesh_DelaunayNodeInsertionMeshAlgo.hxx
- 88: wrap long line
- 127: The method AddVertices can run long, so it is needed to pass the range there to check user break inside the method.

src/BRepMesh/BRepMesh_FaceDiscret.cxx
- 53: there is no corresponding delete, so memory leak will occur. Also, progress scope is not thread safe to be used concurrently. Consider creating a separate range for each parallel task in the main thread.

src/BRepMesh/BRepMesh_IncrementalMesh.hxx
- 60: wrap long line

src/IMeshTools/IMeshTools_MeshBuilder.cxx
- 86: why status is not set here?

Combine to one commit and put the commit message corresponding to workflow rules.

Branch [archived branch] has been created by Commenter 4.

[revision removed]


Detailed log of new commits:

Author: emv
Date: Fri Jul 10 14:19:31 2020 +0300

    add indicator to BRepMesh

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been created by Commenter 4.

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Thu Sep 3 13:07:06 2020 +0300

    fix

Author: emv
Date: Fri Jul 10 14:19:31 2020 +0300

    add indicator to BRepMesh

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Andrey, please revert irrelevant changes in src/XSControl/XSControl_TransferWriter.cxx

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Results of testing on Jenkins:
http://jenkins-test-12.nnov.opencascade.com/view/CR25113-master-Commenter 4/view/COMPARE/

src/BRepMesh/BRepMesh_FaceDiscret.hxx
- remove commented code of class ScopeFace.
- 101: wrap line
- 105-107: revert space changes.

src/BRepMesh/BRepMesh_FaceDiscret.cxx
- 80: wrap line

    Message_ProgressScope aPS(theRange, NULL, 1);
    if (!aPS.More())
    {
      aDFace->SetStatus(IMeshData_UserBreak);
      return;
    }
    aMeshingAlgo->Perform(aDFace, myParameters, aPS.Next());

No need to create aPS here.

tests/perf/mesh/bug25113_2
incmesh is called with the same options as IN bug25113_1, no parallel.

In products, put the commit message corresponding to workflow rules.

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Rebase on master and re-test.

Test results http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR25113-master-Commenter 4/view/COMPARE/

For integration:
occt - CR25113_4
products - CR25113_4

+    if (!aPS.More())
+    {
+      return;
+    }
 
     if (this->getParameters().ControlSurfaceDeflection &&
-        this->getStructure()->ElementsOfDomain().Extent() > 0)
+        this->getStructure()->ElementsOfDomain().Extent() > 0 &&
+        aPS.More())
+    {
+      optimizeMesh(theMesher, aPS.Next());
+    }
+    else
     {
-      optimizeMesh(theMesher);
+      aPS.Next();
     }

aPS.More() in if() statement looks confusing and redundant - as it is preceded by dedicated aPS.More() check.

-  virtual void postProcessMesh(BRepMesh_Delaun& theMesher) Standard_OVERRIDE
+  virtual void postProcessMesh (BRepMesh_Delaun& theMesher, 
+                                const Message_ProgressRange& theRange = Message_ProgressRange()) Standard_OVERRIDE
...
   //! Inserts nodes into mesh.
   Standard_Boolean insertNodes(
     const Handle(IMeshData::ListOfPnt2d)& theNodes,
-    BRepMesh_Delaun&                      theMesher)
+    BRepMesh_Delaun&                      theMesher,
+    const Message_ProgressRange&          theRange = Message_ProgressRange())
...
   Standard_EXPORT virtual Standard_Boolean performInternal (
     const Handle (IMeshData_Model)& theModel,
-    const IMeshTools_Parameters&    theParameters) Standard_OVERRIDE;
+    const IMeshTools_Parameters&    theParameters,
+    const Message_ProgressRange&    theRange = Message_ProgressRange()) Standard_OVERRIDE;

Is there any reason defining a default argument value for internal methods, which are supposed to be always called with non-default parameter?

-  OSD_Parallel::For(0, myModel->FacesNb(), *this, !(myParameters.InParallel && myModel->FacesNb() > 1));
...
+  std::vector<FaceScope> aSF;
+  OSD_Parallel::ForEach(aSF.begin(), aSF.end(), *this, !(myParameters.InParallel && myModel->FacesNb() > 1));

What is the purpose replacing a straight-forward array loop (optimally mapped to parallelization tools) with iterator loop (requiring proxy iterators and additional overhead)?

aSF is a vector - it can be iterated by index range as before.
Just need to define another structure with operator() instead of *this.

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

OSD_Parallel::For() was raplaced to OSD_Parallel::ForEach() cause method process, which is used further is the private method of BRepMesh_FaceDiscret class and we can not use it in operator() in structure

> OSD_Parallel::For() was raplaced to OSD_Parallel::ForEach()
> cause method process, which is used further
> is the private method of BRepMesh_FaceDiscret class
> and we can not use it in operator() in structure
This is just a design issue to fix - no reason to workaround the own internal classes hierarchy limitations.

   Standard_EXPORT virtual Standard_Boolean performInternal (
     const Handle (IMeshData_Model)& theModel,
-    const IMeshTools_Parameters&    theParameters) Standard_OVERRIDE;
+    const IMeshTools_Parameters&    theParameters,
+    const Message_ProgressRange&    theRange = Message_ProgressRange()) Standard_OVERRIDE;

There are still internal methods with confusing defaults.
Are these necessary?

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

+  void process(const Standard_Integer theFaceIndex, 
+               const Message_ProgressRange& theRange = Message_ProgressRange()) const;

No need to make default value for argument of private method.

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated by Commenter 5.

[revision removed]


Detailed log of new commits:

Author: Commenter 5
Date: Thu Sep 17 10:40:34 2020 +0300

    # remarks

Branch [archived branch] has been created by Commenter 5.

[revision removed]


Detailed log of new commits:

Author: emv
Date: Fri Jul 10 14:19:31 2020 +0300

    0025113: Mesh - Progress indication and user break functionality for BRepMesh component
    
    Added Progress Indicator to BRep_Mesh

Dear Commenter 1,

please take updated patch in OCCT branch CR25113_5 into IR.

For integration:
occt - CR25113_5
products - CR25113_4

Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 17528.97000000011 / 17378.390000000145 [+0.87%]
Products
Total CPU difference: 12127.32000000009 / 12079.540000000095 [+0.40%]
Windows-64-VC14:
OCCT
Total CPU difference: 18862.703125 / 18898.921875 [-0.19%]
Products
Total CPU difference: 13314.828125 / 13329.21875 [-0.11%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]