MantisBT - Community
View Issue Details
0025124Community[OCCT] OCCT:Modeling Algorithmspublic2014-08-04 14:212015-05-14 15:32
Roman Lygin 
bugmaster 
normalfeature 
closedfixed 
 
[OCCT] 6.9.0[OCCT] 6.9.0 
bugs modalg_5(010) bug25124_1, bug25124_2, bug25124_3, bug25124_4, bug25124_5, bug25124_6, bug25124_7
0025124: [Feature request] Removal of continuity checks for offset geometries
Problem statement:
Open CASCADE offset geometries (surfaces and 3D/2D curves) may only be constructed on C1 underlying geometries. This is enforced by the respective constructors (e.g. Geom_OffsetSurface) which throw an exception if the condition is not respected. Verification of C1 continuity is performed with the help of Geom_Surface::Continuity(). In the case of B-Spline surface, Geom_BSplineSurface::Continuity() simply uses multiplicity of the knots. If any knot has a multiplicity equal to degree then it returns C0. However even when multiplicity of the knot equals degree the surface can still be G1 (if the normal/tangent direction is continuous) or even C1 (if additionally the magnitude is continuous).

As an example, often offset surfaces coming from the Parasolid or ACIS-based CAD systems lie on the B-Spline surfaces which OCC considers to be C0 (see a couple of examples attached). Thus, although the definition of the offset surface is common in all systems, the underlying surfaces valid in one kernel may be considered invalid in OCC. This creates necessity to modify the underlying surface what changes the original design intent and may have adverse consequences downstream (e.g. the edge curve becomes located farther from the surface leading to edge tolerance increase what may lead to further loss of accuracy and so on).

The suggestion is to remove the enforced check for C1 continuity of the underlying surface. Of course, the requirement for sufficient smoothness is still to be documented and it must be a user responsibility to supply appropriate underlying geometries. This requirement will still be enforced in run-time during evaluation of the respective coordinates and derivatives. For instance, today to compute the first derivative the underlying surface must be C2-continuous however this is not enforced during construction.
Thus, no safety will be compromised - evaluation of the surface (D0, D1,...) may still throw an exception (Geom_UndefinedValue) if the respective value cannot be computed. However this will enable creation of topological faces which would lie within a fully defined and Cn-continuous domain of the underlying and offset surface.

This functionality may be relevant in OCC importers (IGES, STEP, etc) and in customer projects requiring this functionality.

Implementation will likely simply require a removal of the checks/exceptions in the constructor (and removed attempt to upgrade continuity made by Ms. ika in 0023683).
Offset must be built and must be different from source shape.

  SCRIPT # 1
Draw[]> restore a_2999_draw a1
Draw[]> getsurfcontinuity a1
a1 has C0 continuity.

Draw[]> offset o1 a1 20
Draw[]> fit


  SCRIPT # 2
Draw[]> restore a_15592_draw a2
Draw[]> getsurfcontinuity a2
a2 has C0 continuity.

Draw[]> offset o2 a2 20



In additional, please use scripts from message http://tracker.dev.opencascade.org/view.php?id=25124#c32001 [^] for new test creation.
No tags attached.
zip underlying_surfaces_for_offset.zip (1,611) 2014-08-04 14:21
https://tracker.dev.opencascade.org/
Issue History
2014-08-04 14:21Roman LyginNew Issue
2014-08-04 14:21Roman LyginAssigned To => ifv
2014-08-04 14:21Roman LyginFile Added: underlying_surfaces_for_offset.zip
2014-08-18 14:04ifvAssigned Toifv => nbv
2014-08-18 14:04ifvStatusnew => assigned
2014-08-19 12:44gitNote Added: 0030875
2014-08-19 16:05gitNote Added: 0030890
2014-08-19 18:21gitNote Added: 0030902
2014-08-20 09:12nbvNote Added: 0030911
2014-08-20 09:12nbvAssigned Tonbv => ifv
2014-08-20 09:12nbvStatusassigned => resolved
2014-08-20 11:02ifvNote Added: 0030916
2014-08-20 11:02ifvAssigned Toifv => nbv
2014-08-20 11:02ifvStatusresolved => feedback
2014-08-21 12:23gitNote Added: 0030987
2014-08-21 12:29nbvNote Added: 0030988
2014-08-21 12:29nbvAssigned Tonbv => ifv
2014-08-21 12:29nbvStatusfeedback => resolved
2014-08-21 13:30ifvNote Added: 0030998
2014-08-21 13:30ifvStatusresolved => feedback
2014-08-21 13:55gitNote Added: 0031002
2014-08-21 13:57nbvNote Added: 0031003
2014-08-21 13:57nbvStatusfeedback => resolved
2014-08-21 14:04ifvNote Added: 0031005
2014-08-21 14:04ifvStatusresolved => reviewed
2014-08-21 14:23nbvNote Added: 0031006
2014-08-21 14:23nbvAssigned Toifv => nbv
2014-08-21 14:23nbvStatusreviewed => assigned
2014-08-21 14:23nbvNote Edited: 0031006bug_revision_view_page.php?bugnote_id=31006#r7923
2014-08-21 18:10gitNote Added: 0031011
2014-08-22 10:07gitNote Added: 0031022
2014-08-22 16:10gitNote Added: 0031038
2014-08-22 17:07gitNote Added: 0031044
2014-08-22 17:13nbvNote Added: 0031045
2014-08-22 17:13nbvAssigned Tonbv => ifv
2014-08-22 17:13nbvStatusassigned => resolved
2014-08-22 17:35nbvAssigned Toifv => nbv
2014-08-22 17:35nbvStatusresolved => assigned
2014-08-23 10:46Roman LyginNote Added: 0031056
2014-08-25 17:02gitNote Added: 0031065
2014-08-26 16:46gitNote Added: 0031102
2014-08-27 10:39gitNote Added: 0031117
2014-08-28 17:23gitNote Added: 0031165
2014-09-11 18:26abvTarget Version => 7.1.0
2014-09-19 16:27gitNote Added: 0031913
2014-09-19 17:53nbvNote Added: 0031922
2014-09-19 17:53nbvAssigned Tonbv => ifv
2014-09-19 17:53nbvStatusassigned => resolved
2014-09-19 18:34ifvNote Added: 0031923
2014-09-19 18:34ifvAssigned Toifv => nbv
2014-09-19 18:34ifvStatusresolved => feedback
2014-09-23 13:59gitNote Added: 0031999
2014-09-23 14:26nbvNote Added: 0032001
2014-09-23 14:26nbvAssigned Tonbv => ifv
2014-09-23 14:26nbvStatusfeedback => resolved
2014-11-18 11:50gitNote Added: 0034469
2014-11-18 18:17gitNote Added: 0034490
2014-11-18 18:24gitNote Added: 0034492
2014-11-18 18:27ifvAssigned Toifv => msv
2014-11-18 18:28ifvNote Added: 0034494
2014-11-30 21:44msvNote Added: 0034858
2014-11-30 21:44msvAssigned Tomsv => ifv
2014-11-30 21:44msvStatusresolved => assigned
2014-12-01 09:38nbvAssigned Toifv => nbv
2014-12-01 12:23gitNote Added: 0034862
2014-12-01 13:13nbvNote Added: 0034870
2014-12-01 13:13nbvAssigned Tonbv => msv
2014-12-01 13:13nbvStatusassigned => resolved
2014-12-01 13:13nbvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=8689#r8689
2014-12-01 13:36ifvNote Added: 0034874
2014-12-01 13:45nbvNote Added: 0034876
2014-12-01 13:49ifvNote Added: 0034878
2014-12-01 13:51nbvAssigned Tomsv => nbv
2014-12-01 13:51nbvStatusresolved => assigned
2014-12-01 13:59gitNote Added: 0034879
2014-12-01 14:04nbvNote Added: 0034880
2014-12-01 14:04nbvAssigned Tonbv => msv
2014-12-01 14:04nbvStatusassigned => resolved
2014-12-01 14:37nbvNote Added: 0034882
2014-12-01 14:37nbvAssigned Tomsv => nbv
2014-12-01 14:37nbvStatusresolved => feedback
2014-12-03 12:45msvNote Added: 0034963
2014-12-03 12:47msvNote Added: 0034964
2014-12-04 14:58gitNote Added: 0035009
2014-12-04 15:16nbvNote Added: 0035011
2014-12-04 15:16nbvAssigned Tonbv => msv
2014-12-04 15:16nbvStatusfeedback => resolved
2014-12-04 16:28gitNote Added: 0035017
2014-12-04 17:11gitNote Added: 0035022
2014-12-04 17:25gitNote Added: 0035025
2014-12-05 09:26gitNote Added: 0035036
2014-12-05 09:45msvNote Added: 0035037
2014-12-05 09:45msvAssigned Tomsv => bugmaster
2014-12-05 09:45msvStatusresolved => reviewed
2014-12-05 10:00gitNote Added: 0035038
2014-12-05 10:01nbvNote Added: 0035039
2014-12-05 10:01nbvNote Edited: 0035039bug_revision_view_page.php?bugnote_id=35039#r8747
2014-12-05 16:56mkvAssigned Tobugmaster => apv
2014-12-05 19:20gitNote Added: 0035090
2014-12-05 19:21apvNote Added: 0035091
2014-12-09 17:05gitNote Added: 0035221
2014-12-09 17:07apvNote Added: 0035222
2014-12-09 17:07apvAssigned Toapv => nbv
2014-12-09 17:07apvStatusreviewed => feedback
2014-12-09 17:08apvTest case number => bugs modalg_5 bug25124_1, bug25124_2, bug25124_3, bug25124_4, bug25124_5
2014-12-09 17:08apvTest case numberbugs modalg_5 bug25124_1, bug25124_2, bug25124_3, bug25124_4, bug25124_5 => bugs modalg_5(010) bug25124_1, bug25124_2, bug25124_3, bug25124_4, bug25124_5
2014-12-10 10:22gitNote Added: 0035242
2014-12-10 10:23nbvAssigned Tonbv => Roman Lygin
2014-12-10 10:23nbvAssigned ToRoman Lygin => apv
2014-12-10 10:30nbvNote Added: 0035244
2014-12-10 12:42gitNote Added: 0035253
2014-12-10 12:44apvTest case numberbugs modalg_5(010) bug25124_1, bug25124_2, bug25124_3, bug25124_4, bug25124_5 => bugs modalg_5(010) bug25124_1, bug25124_2, bug25124_3, bug25124_4, bug25124_5, bug25124_6, bug25124_7
2014-12-10 13:37gitNote Added: 0035258
2014-12-10 13:39nbvNote Added: 0035259
2014-12-10 13:40nbvAssigned Toapv => nbv
2014-12-10 13:40nbvAssigned Tonbv => apv
2014-12-10 15:08apvNote Added: 0035263
2014-12-10 15:08apvAssigned Toapv => bugmaster
2014-12-10 15:08apvStatusfeedback => tested
2014-12-10 16:18gitNote Added: 0035268
2014-12-10 16:19nbvNote Added: 0035269
2014-12-16 16:46bugmasterChangeset attached => occt master 3d58dc49
2014-12-16 16:46bugmasterStatustested => verified
2014-12-16 16:46bugmasterResolutionopen => fixed
2015-01-19 16:23bugmasterTarget Version7.1.0 => 6.9.0
2015-01-26 12:35gitNote Added: 0036578
2015-01-26 12:38gitNote Added: 0036628
2015-01-26 12:38gitNote Added: 0036634
2015-01-26 12:38gitNote Added: 0036636
2015-05-14 15:29aivStatusverified => closed
2015-05-14 15:32aivFixed in Version => 6.9.0

Notes
(0030875)
git   
2014-08-19 12:44   
Branch CR25124 has been created by nbv.

SHA-1: 29fa55f23d37f29eaddbb91350dbe0bad945e06b


This branch includes the following new commits:

       new 29fa55f 0025124: [Feature request] Removal of continuity checks for offset geometries


Detailed log of new commits:

commit 29fa55f23d37f29eaddbb91350dbe0bad945e06b
Author: nbv
Date: Tue Aug 19 12:23:41 2014 +0400

    0025124: [Feature request] Removal of continuity checks for offset geometries
    
    Sometimes curve or surface, which is defined as C0, has continuity G1 or above. Offset can be built from these shapes.
    Therefore, this extended checking was added into SetBasisCurve and SetBasisSurface methods.
    
    Test cases was changed according to their new behavior.
(0030890)
git   
2014-08-19 16:05   
Branch CR25124 has been updated forcibly by nbv.

SHA-1: 0be6fbcb9054776192efa8f1dea8bded538c9528
(0030902)
git   
2014-08-19 18:21   
Branch CR25124 has been updated forcibly by nbv.

SHA-1: 5dd00eae94c0b2cc7cbe6c4223c9880674ce639f
(0030911)
nbv   
2014-08-20 09:12   
CR25124 is ready to review.
(0030916)
ifv   
2014-08-20 11:02   
1. It is necessary to treat correctly trimmed curves and surfaces.
2. Checking G1 for surfaces does not seem to be optimal - it is not necessary to use nested loops.
(0030987)
git   
2014-08-21 12:23   
Branch CR25124_1 has been created by nbv.

SHA-1: 26d6619aa91dd96f8234c0852f2123a6a24eb8df


Detailed log of new commits:

Author: nbv
Date: Thu Aug 21 12:21:56 2014 +0400

    0025124: [Feature request] Removal of continuity checks for offset geometries
    
    Sometimes curve or surface, which is defined as C0, has continuity G1 or above. Offset can be built from these shapes.
    Therefore, this extended checking was added into SetBasisCurve and SetBasisSurface methods.
    
    Main changes in function BRepOffset_Tool::ExtentFace(...):
    * "return" is added if intersection (in 2D-space) between two edges in a face cannot be found.
    
    Test cases was changed according to their new behavior.
(0030988)
nbv   
2014-08-21 12:29   
CR25124_1 is ready to review.
(0030998)
ifv   
2014-08-21 13:30   
Basis curve of Ext and Rev surfaces can be trimmed curve, please take it in account.
(0031002)
git   
2014-08-21 13:55   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 59ed4fe7e712b9554d3d03a0ac4c492454c4d8b0
(0031003)
nbv   
2014-08-21 13:57   
Done in branch CR25124_1.
(0031005)
ifv   
2014-08-21 14:04   
Ok
(0031006)
nbv   
2014-08-21 14:23   
It is necessary to add this checking for Adaptor Curves (Geom2dAdaptor_Curve::Continuity() method for example).

(0031011)
git   
2014-08-21 18:10   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 4b27fc72d778841b7c46346ea8ad795565ec5927
(0031022)
git   
2014-08-22 10:07   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: e6b187f994e2b5cd279cacb88168b9129e17ac7a
(0031038)
git   
2014-08-22 16:10   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 31a210c4fbc0db279db97dc4c3ecf4105cc8c3c3
(0031044)
git   
2014-08-22 17:07   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 4b38c62a51ab935f3aae7e6381a63a6388ec9ac9
(0031045)
nbv   
2014-08-22 17:13   
CR25124_1 is ready to review.
(0031056)
Roman Lygin   
2014-08-23 10:46   
Folks,

Thank you for your draft version. I had a look at the branch CR25124_1 in git.

1. First off, I believe there is no need for explicit check for G1 continuity in offsets. I referred to G1 in the description as an example of cases when offsets can still be allowed.
Adding this check into offset does not make significant good comparing to existing implementation. For instance, a topological face may be defined outside of the domain where the basis surface is still C0. The new check
will still deny this underlying surface.
Instead my idea was/is to allow creation on any geometry (even on pure C0) and defer the check to run-time (just like it is for D1() computation today).


2. Is support of G1 check can be put into B-Splines instead ?
In that case Geom_BSplineCurve::Continuity() could return G1 (not just C0 or C1 as today). This would eliminate the need to store explicit value in offsets. Perhaps, some algorithms may make implicit assumptions that Continuity() would only return Cn values (e.g. ShapeUpgrade). If so, they could be updated to use
IsCn() instead.


3. Check for point proximity in IsCurveG1() is redundant. The curve is always C0, so the check can be removed:
    if(aP1.SquareDistance(aP2) > Precision::Confusion()*Precision::Confusion())
+ {
+ return Standard_False;
+ }
+

4. Check for G1 continuity is done via hard-coded value (1-1e-15). This can be too rigid or too rough in different scenarios. Instead, for instance you might want to have a static variable that would default to the value of your choide and a possibility to set/get it. Also it would be good to have a static API method that would accept that value explicitly, e.g.
   static Standard_Boolean Geom_BSplineCurve::IsG1 (const Handle_Geom_BSplineCurve& theCurve,
       const double theAngularTolerance);
   (same for 2D curve and surface).
Implementations would just call those methods with static value of angular tolerance. The user would have a possibility to call those methods explicitly in a thread-safe manner.
   
5. Why did you favor the check using a dot product instead of using gp_Vec::IsParallel() ?
   The latter explicitly uses the angular tolerance and can be more streamlined. Performance gain of the former is hardly worth it given that this whole branch is on a cold path.

6. Not certain why other files beyond Geom* need to be updated, but this is up to you. Just in case, is the BRepOffsetAPI_MakeThickSolid affected too ?

Implementation notes:
7. There are 3 copies of IsCurveG1() - Geom_OffsetSurface, _OffsetCurve, Geom2d_OffsetCurve. This is obviously bad for maintenance (risk of discrepancy, size bloat, etc). Please consider creating a single template implementation that would be called.
 
8. A lot of if's in Geom_OffsetSurface (for revs, linear extrusion, trimmed surface/curvces,...).
   As reflected in one of the comments above, there can always be a risk of forgetting some particular cases (e.g. how about a trimmed surface on a surface of linear extrusion built on a trimmed curve on an offset curve on a trimmed curve on a bspline) ?
One efficient technique to deal with such arbitrary nesting is a visitor pattern. In this case you would only need to redefine a method Visit() accepting a bspline curve. The rest of visiting would be a job of either the classes themselves (if the Visit() method is put into the class itself or an external visitor applier). We can talk more face-to-face.
(0031065)
git   
2014-08-25 17:02   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 34947164a3b8b68f619a812de08467467d5fbafe
(0031102)
git   
2014-08-26 16:46   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 690e964629df291f0f2d7b37bda8fc8a73bda0d4
(0031117)
git   
2014-08-27 10:39   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 1ee3cfda2b949cc14a5def2ffe70abda4ce0c1d1
(0031165)
git   
2014-08-28 17:23   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: f90fa2de324c0ee11ed8c66001d2b48826906e8c
(0031913)
git   
2014-09-19 16:27   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: 17dbe3e4d3bd2b3b48f0f8b1294b28501934a4df
(0031922)
nbv   
2014-09-19 17:53   
Branch CR25124_1 is ready to be reviewed.
(0031923)
ifv   
2014-09-19 18:34   
There are some part of code to be corrected
(0031999)
git   
2014-09-23 13:59   
Branch CR25124_1 has been updated forcibly by nbv.

SHA-1: fb371da1f3e4f993b7905d439e7dce8d1ab573f2
(0032001)
nbv   
2014-09-23 14:26   
Dear Igor!
Please review branch CR25124_1.

Dear Testers!
Ask to take into consideration following three scripts when you will create tests (ask to pay attention on the output of DUMP command).

--- SCRIPT 0000001
Draw[]> point pp 0 0 1
Draw[]> circle cc 20 40 30 20
Draw[]> trim cc cc 1 6
Draw[]> offsetcurve oc cc 10 pp
Draw[]> trim cc oc 2 5

Draw[]> dump cc


*********** Dump of cc *************
Trimmed curve
Parameters : 2 5
Basis curve :
OffsetCurveOffset : 10
Direction : 0, 0, 1
Basis curve :
Trimmed curve
Parameters : 1 6
Basis curve :
Circle...

Draw[]> offsetcurve oc cc 10 pp

Draw[]> dump oc


*********** Dump of oc *************
OffsetCurveOffset : 20
Direction : 0, 0, 1
Basis curve :
Trimmed curve
Parameters : 2 5
Basis curve :
Circle...


--- SCRIPT 0000002
Draw[]> circle cc 20 40 20
Draw[]> trim cc cc 1 6
Draw[]> offset2dcurve oc cc 10
Draw[]> trim cc oc 2 5

Draw[]> dump cc


*********** Dump of cc *************
Trimmed curve
Parameters : 2 5
Basis curve :
OffsetCurveOffset : 10
Basis curve :
Trimmed curve
Parameters : 1 6
Basis curve :
Circle...
  
Draw[]> offset2dcurve oc cc 10
Draw[]> dump oc


*********** Dump of oc *************
OffsetCurveOffset : 20
Basis curve :
Trimmed curve
Parameters : 2 5
Basis curve :
Circle...



--- SCRIPT #3
Draw[]> sphere ss 30
Draw[]> trim ss ss 0.5 6 -1 1
Draw[]> offset os ss 10
Draw[]> trim ss os 1 5.5 -0.8 0.5

Draw[]> dump ss


*********** Dump of ss *************
RectangularTrimmedSurface
Parameters : 1 5.5 -0.8 0.5
BasisSurface :
OffsetSurface
Offset : 10
BasisSurface :
RectangularTrimmedSurface
Parameters : 1 5.5 -0.8 0.5
BasisSurface :
SphericalSurface...

Draw[]> offset os ss 10
Draw[]> dump os


*********** Dump of os *************
OffsetSurface
Offset : 20
BasisSurface :
RectangularTrimmedSurface
Parameters : 1 5.5 -0.8 0.5
BasisSurface :
SphericalSurface...



CONCLUSION:
Basis curve/surface for offset curve/surface cannot have OFFSET-type or TRIMMED from OFFSET-type.



--- SCRIPT #4 (Only surface of linear extrusion participates into this script. But analogical script should be created for surface of revolution)
Draw[]> point pp 0 0 1
Draw[]> circle cc 20 40 30 20
Draw[]> convert bc cc
Draw[]> trim bc bc 1 6
Draw[]> offsetcurve oc bc 10 pp
Draw[]> trim bc oc 2 5
Draw[]> extsurf se bc 0 0 1
Draw[]> offset os se 15
Draw[]> dump os


*********** Dump of os *************
OffsetSurface
Offset : 15
BasisSurface :
SurfaceOfLinearExtrusion
  Direction :0, 0, 1
  Basis curve :
Trimmed curve
Parameters : 2 5
Basis curve :
OffsetCurveOffset : 10
Direction : 0, 0, 1
Basis curve :
Trimmed curve
Parameters : 1 6
Basis curve :
BSplineCurve rational periodic
  Degree 2, 6 Poles, 4 Knots
Poles : ...

Knots :

   1 : 0 2
   2 : 2.0943951023932 2
   3 : 4.18879020478639 2
   4 : 6.28318530717959 2



Into this script the offset from surface of linear extrusion was built. But basis curve for this surface has C0-continuity.
(0034469)
git   
2014-11-18 11:50   
Branch CR25124_2 has been created by ifv.

SHA-1: d2e1325cc8c2934fcfd4b7d897bce57cce7a0bcf


Detailed log of new commits:

Author: ifv
Date: Mon Nov 17 17:06:04 2014 +0300

    Fix for 0024621 added

Author: ifv
Date: Mon Nov 17 15:22:48 2014 +0300

    New implementation

Author: ifv
Date: Thu Nov 13 10:16:31 2014 +0300

    Merge branch 'master' of git.dev.opencascade.org:occt into CR25124_2

Author: ifv
Date: Thu Nov 6 11:47:50 2014 +0300

    Merge branch 'master' into CR25124_2
    
    Conflicts:
        src/IntPatch/IntPatch_ImpImpIntersection_4.gxx
(0034490)
git   
2014-11-18 18:17   
Branch CR25124_2 has been deleted by ifv.

SHA-1: d2e1325cc8c2934fcfd4b7d897bce57cce7a0bcf
(0034492)
git   
2014-11-18 18:24   
Branch CR25124_5 has been created by ifv.

SHA-1: 9fddebe4177b9e47acb97769c719bc8ff94b6375


Detailed log of new commits:

Author: ifv
Date: Tue Nov 18 16:27:16 2014 +0300

    0025124: [Feature request] Removal of continuity checks for offset geometries
(0034494)
ifv   
2014-11-18 18:28   
Dear msv,
please rewiev branch CR25124_5
(0034858)
msv   
2014-11-30 21:44   
1. Geom2d_BSplineCurve_1.cxx:100-110 avoid try/catch without real need.
2. Geom2d_BSplineCurve::IsG1 method does not check if the curve is closed/periodic. In this case it is needed to check smoothness at curve begin/end.
3. Geom2d_OffsetCurve.cxx:149 irrelevant name aBasisSurf in the comment
4. Geom_OffsetCurve.cxx:222 irrelevant name aBasisSurf in the comment
(0034862)
git   
2014-12-01 12:23   
Branch CR25124_6 has been created by nbv.

SHA-1: addda0c54b43c932d23d9f4dc0dbb27e938d58e3


Detailed log of new commits:

Author: nbv
Date: Mon Dec 1 09:49:40 2014 +0300

    0025124: [Feature request] Removal of continuity checks for offset geometries
    
    Sometimes curve or surface, which is defined as C0, has continuity G1 or above. Offset can be built from these shapes.
    Therefore, this extended checking was added into SetBasisCurve and SetBasisSurface methods.
    
    Main changes in function BRepOffset_Tool::ExtentFace(...):
    * "return" is added if intersection (in 2D-space) between two edges in a face cannot be found.
    
    Basis curve/surface continuity value found (if G1-checking is OK) is set up as BasisContinuity (see myBasisCurveContinuity and myBasisSurfContinuity members which is returned by GetBasisCurveContinuity and GetBasisSurfContinuity() methods). This fact is used in Geom2dAdaptor and in GeomAdaptor classes.
    
    Possibility is entered, which allows for basis elements of offset curve/surface to avoid of C0-checking.
    
    Test cases was changed according to their new behavior.
(0034870)
nbv   
2014-12-01 13:13   
Dear Mikhail!

Please review branch CR25124_6.
(0034874)
ifv   
2014-12-01 13:36   
Removal of catching exception causes duble calculation and checking of vector magnitude, which is checked in method Angle().
(0034876)
nbv   
2014-12-01 13:45   
I agree with Igor to the full extent. But the way offered by Mikhail allow to avoid appearing exception. Therefore, in branch CR25124_6 I have deleted try-catch blocks.

But it leads to double checking, really. That might be no good.
(0034878)
ifv   
2014-12-01 13:49   
if(!IsPeriodic() && !IsClosed())
    return Standard_True;

It is not necessary to use !IsClosed(): only periodic curves must have smooth end connection.
(0034879)
git   
2014-12-01 13:59   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: 90c5e92ae1b79d15cb5e95b5307b003b725c9562
(0034880)
nbv   
2014-12-01 14:04   
Branch CR25124_6 was updated according to latest remark from ifv (IsClosed checking)
(0034882)
nbv   
2014-12-01 14:37   
It is necessary to analyze, what is the most effective way?

1. Use try-catch;
2. Use Done/Not done flag
3. Use double checking
(0034963)
msv   
2014-12-03 12:45   
Usage of try/catch is unjustified in our case. It slows execution very much if an exception occurs. To prove this I have written a small program. It measures the time of calculations of two variants, one is with try/catch block, and another is with checking for null vector.

    // using try/catch
    for (int i=1; i <= theNiter; i++)
    {
        try {
            gp_Vec aV1(0,0,1), aV2(1,0,0);
            if (theExcEach && (i % theExcEach) == 0)
                aV1.SetZ(0);
            a = aV1.Angle(aV2);
        }
        catch(Standard_Failure)
        {
            nbexc++;
        }
    }


    // using check for null
    for (int i=1; i <= theNiter; i++)
    {
        gp_Vec aV1(0,0,1), aV2(1,0,0);
        if (theExcEach && (i % theExcEach) == 0)
            aV1.SetZ(0);
        if (aV1.SquareMagnitude() < RealSmall() || aV2.SquareMagnitude() < RealSmall())
        {
            nbexc++;
            continue;
        }
        a = aV1.Angle(aV2);
    }


theNIter is the number of iterations;
theExcEach is the parameter allowing to control the frequency of occurrence of null vector.

Here are the results of testing:

D:\msv\util>test_try.bat 10000000 0
with try: 10000000 iterations, 0 exceptions, 0.436803 sec
no try: 10000000 iterations, 0 nulls, 0.436803 sec

D:\msv\util>test_try.bat 10000000 10000
with try: 10000000 iterations, 1000 exceptions, 0.452403 sec
no try: 10000000 iterations, 1000 nulls, 0.452403 sec

D:\msv\util>test_try.bat 10000000 100
with try: 10000000 iterations, 100000 exceptions, 0.858005 sec
no try: 10000000 iterations, 100000 nulls, 0.452403 sec


We can see that the frequent null vector occurs, the more time program spends using try/catch mechanism, and the same time it spends using checking for null vector.

One more note. You see that the check for null vector uses SquareMagnitude instead of Magnitude method. It allows to avoid call of sqrt function. I have also took an experiment with Magnitude, and it is presented below:

D:\msv\util>test_try.bat 10000000 0
with try: 10000000 iterations, 0 exceptions, 0.436803 sec
no try: 10000000 iterations, 0 nulls, 0.499203 sec


We can see that the time of "no try" way has increased a little.

Resume:
1) it is better to avoid try/catch constructions if it is possible to check for exception condition.
2) it is better to avoid calling Magnitude method if it is enough to evaluate SquareMagnitude.
(0034964)
msv   
2014-12-03 12:47   
Just in case, here I publish the whole source code of my small program.

#include <gp_Vec.hxx>
#include <OSD_Chronometer.hxx>

void test(int theNiter, int theExcEach)
{
    int nbexc = 0;
    double a;
    OSD_Chronometer aTime;
    aTime.Start();
    // using try/catch
    for (int i=1; i <= theNiter; i++)
    {
        try {
            gp_Vec aV1(0,0,1), aV2(1,0,0);
            if (theExcEach && (i % theExcEach) == 0)
                aV1.SetZ(0);
            a = aV1.Angle(aV2);
        }
        catch(Standard_Failure)
        {
            nbexc++;
        }
    }
    aTime.Stop();
    double t;
    aTime.Show(t);
    cout<<"with try: "<<theNiter<<" iterations, "<<nbexc<<" 
exceptions, "<<t<<" sec"<< endl;
    aTime.Reset();
    aTime.Start();
    nbexc = 0;
    // using check for null
    for (int i=1; i <= theNiter; i++)
    {
        gp_Vec aV1(0,0,1), aV2(1,0,0);
        if (theExcEach && (i % theExcEach) == 0)
            aV1.SetZ(0);
        if (aV1.Magnitude() < RealSmall() || aV2.Magnitude() < RealSmall())
        {
            nbexc++;
            continue;
        }
        a = aV1.Angle(aV2);
    }
    aTime.Stop();
    aTime.Show(t);
    cout<<"no try: "<<theNiter<<" iterations, "<<nbexc<<" 
nulls, "<<t<<" sec"<<endl;
}

int main(int n, char** a)
{
  if (n < 3) {
      cout<<"expected args: nb_iter exception_each_n"<<endl;
      return 1;
  }
  test(atoi(a[1]), atoi(a[2]));
  return 0;
}
(0035009)
git   
2014-12-04 14:58   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: 28d9673706e160d641604871280a41f846fed7cd
(0035011)
nbv   
2014-12-04 15:16   
In accordance with the result of the discussion, we exclude try-catch block using and try to use SquareMagnitude() method instead of Magnitude.

It was implemented into the updated CR25124_6 branch.



Dear Mikhail!

Please review it.
(0035017)
git   
2014-12-04 16:28   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: 1650a4be122a616f816d8d20db1a25fdb54597bd
(0035022)
git   
2014-12-04 17:11   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: 9bb2698bc0345efb50f0e7854195ccf327cd0e2c
(0035025)
git   
2014-12-04 17:25   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: 437402dcb6d99d6dea1762a3d5b0b9cca7d06300
(0035036)
git   
2014-12-05 09:26   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: a29c69f25da2d14442802ad1625280ce786d18cc
(0035037)
msv   
2014-12-05 09:45   
Ready for testing.
Dear nbv, please rebase on master.
(0035038)
git   
2014-12-05 10:00   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: f4d9c4184819975ee802e770654a285f61c27ead
(0035039)
nbv   
2014-12-05 10:01   
CR25124_6 was rebased on the current master (all commits were squashed).

(0035090)
git   
2014-12-05 19:20   
Branch CR25124_6 has been updated forcibly by apv.

SHA-1: f10416a769017895719ef3c2b2ef8ce0c862ba19
(0035091)
apv   
2014-12-05 19:21   
Branch CR25124_6 has been rebased on the current master
(0035221)
git   
2014-12-09 17:05   
Branch CR25124_6 has been updated by apv.

SHA-1: c54baa1b368395c5d1a08865c10fb467ff2bfb22


Detailed log of new commits:

Author: apv
Date: Tue Dec 9 17:05:25 2014 +0300

    Test-cases for issue 0025124

(0035222)
apv   
2014-12-09 17:07   
Dear nbv,

Please review test-cases for this issue.
(0035242)
git   
2014-12-10 10:22   
Branch CR25124_6 has been updated by nbv.

SHA-1: d299027a9637f343093d4f4c58c76ee5c78b6eb5


Detailed log of new commits:

Author: nbv
Date: Wed Dec 10 10:22:07 2014 +0300

    Small correction of test cases after review.

(0035244)
nbv   
2014-12-10 10:30   
Test cases created were reviewed with small correction. However, There are some remarks:

1. Please show picture in every test cases created. Necessary for showing shapes have already been prepared (donly command was added).

2. Please create test cases with using the shapes attached by Roman Lygin. These scripts should look like bug25124_4 (check, if offset exist and offset surface is not equal to the source surface).
(0035253)
git   
2014-12-10 12:42   
Branch CR25124_6 has been updated by apv.

SHA-1: 6a79414615e92cdc9d084b82d65f8d03e250764d


Detailed log of new commits:

Author: apv
Date: Wed Dec 10 12:42:45 2014 +0300

    Update of test-cases for issue

(0035258)
git   
2014-12-10 13:37   
Branch CR25124_6 has been updated by nbv.

SHA-1: e16715bb0f05b695732eccfb772c7661fc139db8


Detailed log of new commits:

Author: nbv
Date: Wed Dec 10 13:37:09 2014 +0300

    Update of test-cases after reviewing

(0035259)
nbv   
2014-12-10 13:39   
Some test cases were changed.

Main remark after reviewing:
It is necessary for test "bugs modalg_5 bug25124_2" to use 2D-viewer instead of 3D used. Please update.
(0035263)
apv   
2014-12-10 15:08   
Dear BugMaster,

Branch CR25124_6 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
SHA-1: f10416a769017895719ef3c2b2ef8ce0c862ba19

Number of compiler warnings:
occt component:
   Linux: 18 (18 on master)
   Windows: 0 (0 on master)
products component :
   Linux: 11 (11 on master)
   Windows: 0 (1 on master)

Regressions/Differences:
Not detected

Testing cases:
bugs modalg_5(010) bug25124_1 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_1.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_1.html [^]
bugs modalg_5(010) bug25124_2 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_2.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_2.html [^]
bugs modalg_5(010) bug25124_3 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_3.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_3.html [^]
bugs modalg_5(010) bug25124_1 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_4.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_4.html [^]
bugs modalg_5(010) bug25124_5 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_5.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_5.html [^]
bugs modalg_5(010) bug25124_6 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_6.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_6.html [^]
bugs modalg_5(010) bug25124_7 - OK
http://occt-tests/CR25124-6-master-occt/Debian60-64/bugs/modalg_5/bug25124_7.html [^]
http://occt-tests/CR25124-6-master-occt/Windows-32-VC10/bugs/modalg_5/bug25124_7.html [^]

Testing on Linux:
Total MEMORY difference: 363920196 / 363473864
Total CPU difference: 47466.610000000146 / 47536.8399999999

Testing on Windows:
Total MEMORY difference: 277271144 / 277124952
Total CPU difference: 33895.609375 / 41681.34375
(0035268)
git   
2014-12-10 16:18   
Branch CR25124_6 has been updated forcibly by nbv.

SHA-1: f27ebabad68ccb82b175aafe15189f36fd863e40
(0035269)
nbv   
2014-12-10 16:19   
All commits were squashed.
(0036578)
git   
2015-01-26 12:35   
Branch CR25124_6 has been deleted by inv.

SHA-1: f27ebabad68ccb82b175aafe15189f36fd863e40
(0036628)
git   
2015-01-26 12:38   
Branch CR25124_5 has been deleted by inv.

SHA-1: 9fddebe4177b9e47acb97769c719bc8ff94b6375
(0036634)
git   
2015-01-26 12:38   
Branch CR25124_1 has been deleted by inv.

SHA-1: fb371da1f3e4f993b7905d439e7dce8d1ab573f2
(0036636)
git   
2015-01-26 12:38   
Branch CR25124 has been deleted by inv.

SHA-1: 5dd00eae94c0b2cc7cbe6c4223c9880674ce639f