[Regression in 6.9.0] Exporting a face throws an exception

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

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Thu Dec 3 11:35:42 2015 +0300

    0026931: [Regression in 6.9.0] Exporting a face throws an exception
    Add check for cutting segment, which is more than period.

Dear GKA,
could you please review branch CR26931?

Seems that this fix can not resolve this problem in the common case

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

[revision removed]

Writing periodic BSpline surfaces to IGES:
Replace segmentation of surface to setting new origin.
Fix face bounds if its length (in U or V) is more than period.

Segmentation of BSpline curve/surface:
Throw exception if segment length more than period.

Fix test case bugs moddata_1 bug14782:
bounds of segmentation must be the same as curve bounds, according to issue description.

Dear GKA,
could you please review branch CR26931?

Quick comment on using Standard_DomainError_Raise_if.
OCC's *_Raise_if() syntax is similar to assert() - triggers an event in debug mode and does not do anything in release. Its purpose is to verify pre-/post-condition.

In your case you apparently want to enforce the condition and want to not do anything meaningful if the condition is not respected.
In this case you are rather to throw a real exception and hence, use
Standard_DomainError::Raise (...) which will work both in release and debug modes.

You might want to verify with your modeling experts if this is the proper or best case to do here.

1) I agree with Roman. To put a base under this requirement, I would like to add that we should take into consideration the statements written in documentation (doxygen) to methods. If documentation says that the method raises exception it must raise in all modes of work.
In this regard, I kindly ask Irina to make the code (in three concerned classes) respecting this rule.

2) One more remark concerns comments in doxygen style. Please take into account that doxygen doesn't pay attention to new line characters in comments. Everything will be joined together. So, when you insert a line in doxygen comment, and the previous line doesn't contain a ending punctuation (a point), put it there by yourself.

3) In all comments you wrote "...if ((U2 - U1) - Period) more than tolerance for periodic...". It is unclear which tolerance do you mean. It is better to write "...if U2-U1 exceeds the period for periodic...", and then add a precise formula, like "i.e. (U2-U1)-Period > PConfusion", or "i.e. (U2-U1)-Period > Resolution(Confusion)".

I think, using Resolution(...) to find 2d tolerance does not seem to be good idea. In Resolution(...) calculation of 2d preciion is base on majorant estimation of first derivative, which can be rather irrelevant for BSplines with "bad" parametrization. In my opinion, it is better using something like Epsilon(period) to be in correspondence with other similar checking for BSpline parameters. For example, knot checking: CKnots (I+1) - CKnots (I) <= Epsilon (Abs(CKnots (I))) - see metthod CheckCurveData(...).

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

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Fri Dec 25 13:23:53 2015 +0300

    0026931: [Regression in 6.9.0] Exporting a face throws an exception
    
    Replace Standard_DomainError_Raise_if by Standard_DomainError::Raise.
    Replace Resolution by Precision::PConfusion().
    Fix comments.

Dear colleagues,
thank you for your remarks, branch CR 26931 was updated, according to it.

Dear GKA,
could you please review the changes?

src\Geom\Geom_BSplineSurface.hxx

1) My second remark in the comment #0026931 was not followed. Please put point in the end of sentence in the lines 554 and 574.

src\Geom2d\Geom2d_BSplineCurve.cxx
src\Geom\Geom_BSplineCurve.cxx

2) I would like also the code to be put in accordance with the first remark completely. For that, please replace *_Raise_if macros with unconditional exceptions in the methods SetOrigin in the above files.

I have not any remarks to modifications of the class GeomToIges_GeomSurface

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

[revision removed]

Dear MSV,

The follow changes were made in classes Geom2d_BSplineCurve, Geom_BSplineCurve, Geom_BSplineSurface:
Replace *Raise_if macros with unconditional exceptions.
Add some missing comments in .hxx files about raised exceptions:
- Geom_BSplineSurface::VKnot() (the same as in UKnot),
- Geom2d_BSplineCurve::LocalD0() (the same as in other LocalDX).
Update comments for methods Knots() and KnotSequence() for Geom(2d)_BSplineCurve, according to condition in .cxx files.
Delete some comments in .hxx files about exceptions, which are not raised in .cxx files.

could you please review branch CR26931?

Reviewed with the note: we can get performance regression, since very light methods like Geom_BSplineCurve::Knot now spend time checking the index for being in range even in release mode. If this risk goes on we will have to revert such checks for light methods. Anyway, exception will be raised as said in the header, though its type will have another nature (access violation signal instead of Standard_OutOfRange).

The modifications are abusing Raise() now and definitively has performance impact, as Mikhail points out. Please revert the access methods and simple checks related to validation of input parameters!

Access methods, like Pole(), Knot(), etc which are on hot path should not use explicit checks and raising exceptions in release mode. They should behave as assert (which could be even better than Raise_If in this case). The classical example is std::vector::at() or NCollection_Array1::Value().

I believe we discussed the Segment() method in the particular periodic case. Now it's difficult to say, as the branch has been rebased with enforced option - although it should have been renamed with _1, _2 suffix per git guidelines. That case in Segment() is on a cold path, and there explicit exception could be justified as long as you really want to enforce that and anticipate that you cannot handle the case otherwise. In other places, using Raise() is an overkill and need to be reverted.
Thank you.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Thu Dec 3 11:35:42 2015 +0300

    0026931: [Regression in 6.9.0] Exporting a face throws an exception
    
    Writing periodic BSpline surfaces to IGES:
    Replace segmentation of surface to setting new origin.
    Fix face bounds if its length (in U or V) is more than period.
    
    Segmentation of BSpline curve/surface:
    Throw exception if segment length more than period.
    
    Fix test case bugs moddata_1 bug14782:
    bounds of segmentation must be the same as curve bounds, according to issue description.
    
    Changes in classes Geom2d_BSplineCurve, Geom_BSplineCurve, Geom_BSplineSurface:
    - Replace *Raise_if macros with unconditional exceptions where it does not affect on performance.
    - Update comments in .hxx files in regard of raised exceptions.

Please test the branch CR26931_1. It contains the changes that follow the above comments of Roman and me.

Dear Commenter 1,

Branch CR26931_1 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
[revision removed]

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 134 (134 on master)
products component:
   Linux: 37 (37 on master)
   Windows: 0 (0 on master)

Regressions/Differences:
Not detected

Testing cases:
bugs iges bug26931 - OK
http://occt-tests/CR26931-1-master-occt-64/Debian70-64/bugs/iges/bug26931.html
http://occt-tests/CR26931-1-master-occt-64/Windows-64-VC10/bugs/iges/bug26931.html

Testing on Linux:
Total MEMORY difference: 89448197 / 89882721 [-0.48%]
Total CPU difference: 19142.439999999995 / 19263.71000000009 [-0.63%]

Testing on Windows:
Total MEMORY difference: 57210554 / 57466619 [-0.45%]
Total CPU difference: 17919.678868998977 / 18535.477216399115 [-3.32%]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]