Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Wed Jul 28 13:19:22 2021 +0300

    0032448: Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface
    
    Add '-exact' option to checkshape command to use exact method to validate edges using BRepLib_ValidateEdge class. Default mode is calculating in finite number of points.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Wed Jul 28 13:19:22 2021 +0300

    0032448: Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface
    
    Add '-exact' option to checkshape command to use exact method to validate edges using BRepLib_ValidateEdge class. Default mode is calculating in finite number of points.

Michael, please make a preliminary review of the code in the CR32448_1 branch. There are no test results in the branch because the server is currently off.
Newly added tests are local OK

src/BRepLib/BRepLib_ValidateEdge.hxx

  //! BRepLib_CheckCurveOnSurface class (if theIsExact is true, slowly, but more correctly. 

Close parenthesis is missed.

  //! Default method is exact

This is not true.
Also, add a note that exact method is used only when edge is SameParameter.

Instead of adding the method SetParallel add the optional parameter theMultiThread in Process method. By the way, myIsParallel is not initialized in the constructor.

src/BRepTest/BRepTest_CheckCommands.cxx
Add option -exact into help.

src/BRepCheck/BRepCheck_Result.hxx

  Standard_Boolean myIsParallel;

No need to add this flag. Check for parallel by myMutex.IsNull().

src/BRepCheck/BRepCheck_Edge.cxx
The field myIsExactMethod is not initialized.

src/BRepCheck/BRepCheck_Analyzer.hxx
Please add description of parameters theIsParallel and theIsExact.

Please add option -exact in user guide.

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Wed Jul 28 13:19:22 2021 +0300

    0032448: Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface
    
    Add '-exact' option to checkshape command to use exact method to validate edges using BRepLib_ValidateEdge class. Default mode is calculating in finite number of points.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Sep 30 17:12:13 2021 +0300

    0032448: chrono

The task is generally solved, but new tests bug32448_2 bug32448_5 bug32448_10 are killed by CPU limit (300 sec) on Debian80-64. I did a benchmark comparison for test models for windows and Debian in all modes (see Attachment 3 (XLSX)). Tests showed a decrease in performance on Unix up to 10 times. However, all tests were successful on both sysmems. I assume that the problem of poor performance on Unix is outside of this task, but it is necessary to check which area of the code is causing the slowdown.
The branch CR32448_2 contains, in addition to the main commit, 2 temporary ones: with additional tests and containing the execution time logging. The file Attachment 2 (LOG) contains results of windows time logging, but it is necessary to carry out similar ones for unix, compare the results and make a decision on this issue, for example, just increase cpulimit for failed tests.

I propose to increase CPU limit to pass tests in this bug, and create a new bug to investigate the cause of slow work on Linux in compare to Windows.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Mon Oct 11 13:44:57 2021 +0300

    increase cpulimit

Author: Author
Date: Wed Jul 28 13:19:22 2021 +0300

    0032448: Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface
    
    Add '-exact' option to checkshape command to use exact method to validate edges using BRepLib_ValidateEdge class. Default mode is calculating in finite number of points.

Author: Author
Date: Thu Sep 2 11:41:49 2021 +0300

    0032448: temporary tests

Author: Author
Date: Thu Sep 30 17:12:13 2021 +0300

    0032448: chrono

Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Mon Oct 11 16:16:35 2021 +0300

    update tests for use only short info

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch for review:
OCCT - CR32448_4

Results of testing:
http://jenkins-test-occt.nnov.opencascade.com/view/CR32448_3-master-Commenter 3/view/COMPARE/

+  //! Calculating in finite number of points
+  void ProcessApprox();
+
+  //! Calculating by using BRepLib_CheckCurveOnSurface class
+  void ProcessExact(Standard_Boolean theIsMultiThread);

It is preferred naming private/protected methods starting with lower-case.

+  void SetExactMethod(Standard_Boolean theIsExact)
+  {
+    myIsExactMethod = theIsExact;
+  }

Adding a getter as well would look more natural.

+      Standard_Boolean aRunParallel = !myMutex.IsNull();

toRunParallel.

+  void ProcessExact(Standard_Boolean theIsMultiThread);
+
   Handle(Adaptor3d_Curve) myReferenceCurve;

Please put a "private: " separator between methods and class fields.

+  Standard_EXPORT void Process(Standard_Boolean theIsMultiThread = Standard_False);

This is unusual to pass multi-threading flag as an argument to Perform()/Process() methods - normally flag is stored as a class property with dedicated getter/setter. The same, I guess, is applicable to BRepCheck_Analyzer.
But I would follow Mikhail suggestion on this aspect to decide if this specific class should be considered as an exception, as class looks internal anyway.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Wed Oct 27 13:15:18 2021 +0300

    remarks from kgv

Author: Commenter 3
Date: Tue Oct 12 16:14:08 2021 +0300

    0032448: increase cpulimit

Author: Author
Date: Wed Jul 28 13:19:22 2021 +0300

    0032448: Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface
    
    Add '-exact' option to checkshape command to use exact method to validate edges using BRepLib_ValidateEdge class. Default mode is calculating in finite number of points.

dox/user_guides/draw_test_harness/draw_test_harness.md
src/BRepTest/BRepTest_CheckCommands.cxx
Update the checkshape command line with the new option.

tests/heal/begin
# To prevent loops limit to 5 minutes
The comment is not true. Avoid exact value in it.

  BRepCheck_Analyzer (const TopoDS_Shape& S,
                      const Standard_Boolean GeomControls = Standard_True,
                      const Standard_Boolean theIsParallel = Standard_False,
                      const Standard_Boolean theIsExact = Standard_False)

It is a bad practise to make heavy constructor API when adding a new feature. Please, add corresponding get/set methods. The same is for isParallel flag, even if it was added earlier.

I support the proposal of Kirill to remove the flag theIsMultithread/isParallel from Perform/Process/Put methods.

Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Wed Oct 27 17:50:42 2021 +0300

    remarks from msv

Branch for review:
OCCT - CR32448_4

  Standard_EXPORT Standard_Boolean GetParallel()
  {
    return myIsParallel;
  }

Do not use Standard_EXPORT in inline methods (carefully check all your changes).
Getter of boolean flag is better to name IsParallel.

  Standard_EXPORT Standard_Boolean GetExactMethod()

The same remarks.

Why in src/BRepLib/BRepLib_ValidateEdge.hxx you missed the long description of the method SetExactMethod? Also, the comments to SetParallel are absent and to GetParallel are wrong.

Please change src/BRepLib/BRepLib_CheckCurveOnSurface.cxx also to add set/get methods of parallel flag.

  //! Gets validate edge method
  Standard_EXPORT Standard_Boolean SetExactMethod()

IsExactMethod

In src/BRepCheck/BRepCheck_Analyzer.hxx add the comments to Set/IsParallel methods.

Update upgrade guide as public methods are changed.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Thu Oct 28 19:23:37 2021 +0300

    0032448: Modeling Algorithms - Provide exact validating (as option) using GeomLib_CheckCurveOnSurface
    
    Add '-exact' option to checkshape command to use exact method to validate edges using BRepLib_ValidateEdge class. Default mode is calculating in finite number of points.

In the classes BRepCheck_Analyzer, BRepLib_CheckCurveOnSurface, initialize the new flags in the constructor (as it is done usually for fields) rather than in the method Init.

In BRepLib_ValidateEdge, you forgot to initialize the new flag in the constructor.

In GeomLib_CheckCurveOnSurface, remove initialization of myIsParallel in the method Init. It must be initialized only in the constructor. The user has the setter to change it if needed.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

BRepLib_CheckCurveOnSurface default constructor does not initialize the new flag.

In the class BRepLib_ValidateEdge, the fields have the following order:
  Standard_Boolean myIsExactMethod;
  Standard_Boolean myIsMultiThread;
But in the constructor you initialize them in another order:
  myIsMultiThread(Standard_False),
  myIsExactMethod(Standard_False)
It will generate warning on some gcc version. The fields must be initialized in the order of their definition.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

BRepLib_CheckCurveOnSurface()
  {
    myIsParallel = Standard_False;
  }

Please use field initialization instead of assignment.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

For integration:
occt - CR32448_5
products - none

As the target version has changed to 7.7.0 then please update the upgrade.md, placing the new notes to a new chapter.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

upgrade.md has been updated

For integration:
occt - CR32448_5
products - none

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: 17763.090000000444 / 17742.320000000425 [+0.12%]
Products
Total CPU difference: 11658.550000000125 / 11669.660000000133 [-0.10%]
Windows-64-VC14:
OCCT
Total CPU difference: 19745.71875 / 19762.140625 [-0.08%]
Products
Total CPU difference: 13066.0625 / 12843.21875 [+1.74%]


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]