MantisBT - Community
View Issue Details
0026931Community[OCCT] OCCT:Data Exchangepublic2015-11-27 13:042016-04-20 15:50
Roman Lygin 
bugmaster 
normalcrash 
closedfixed 
[OCCT] 6.9.1 
[OCCT] 7.0.0[OCCT] 7.0.0 
bugs iges bug26931
0026931: [Regression in 6.9.0] Exporting a face throws an exception
The root-cause is a modification introduced in 0026138.

GeomToIGES_GeomSurface::TransferSurface() invokes Segment() which creates too near knots in B-Spline. Afterwards when taking an isocurve the ctor of Geom_BSplineCurve throws an exception Standard_ConstructionError on these two near knots.

The original B-Rep is an excerpt of resulting model coming from STEP.

Version 6.8.0 and earlier succeeds while 6.9.0 throws an exception.
bugs iges bug26931
No tags attached.
related to 0026138closed bugmaster Open CASCADE Problems with writing periodic BSplines into IGES 
related to 0026989closed bugmaster Community [Regression in 6.9.0] Export of a reversed face leads to wrong data in 6.9.0 
zip b_14.zip (6,151) 2015-11-27 13:04
https://tracker.dev.opencascade.org/
? bug26931.brep (24,758) 2015-12-03 11:40
https://tracker.dev.opencascade.org/
Issue History
2015-11-27 13:04Roman LyginNew Issue
2015-11-27 13:04Roman LyginAssigned To => gka
2015-11-27 13:04Roman LyginFile Added: b_14.zip
2015-11-27 13:29gkaAssigned Togka => ika
2015-11-27 13:29gkaStatusnew => assigned
2015-11-27 14:30abvRelationship addedrelated to 0026138
2015-12-03 11:39gitNote Added: 0048717
2015-12-03 11:40ikaFile Added: bug26931.brep
2015-12-03 11:41ikaNote Added: 0048718
2015-12-03 11:41ikaAssigned Toika => gka
2015-12-03 11:41ikaStatusassigned => resolved
2015-12-03 11:41ikaSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=12421#r12421
2015-12-13 13:29Roman LyginRelationship addedrelated to 0026989
2015-12-16 14:13gkaNote Added: 0049206
2015-12-16 14:13gkaAssigned Togka => ika
2015-12-16 14:13gkaStatusresolved => assigned
2015-12-23 14:07gitNote Added: 0049505
2015-12-23 14:09ikaNote Added: 0049506
2015-12-23 14:09ikaAssigned Toika => gka
2015-12-23 14:09ikaStatusassigned => resolved
2015-12-23 14:41Roman LyginNote Added: 0049507
2015-12-23 16:33msvNote Added: 0049525
2015-12-23 16:33msvAssigned Togka => ika
2015-12-23 16:33msvStatusresolved => assigned
2015-12-24 13:43ifvNote Added: 0049558
2015-12-25 13:25gitNote Added: 0049592
2015-12-25 13:28ikaNote Added: 0049593
2015-12-25 13:28ikaAssigned Toika => gka
2015-12-25 13:28ikaStatusassigned => resolved
2015-12-25 14:57msvNote Added: 0049596
2015-12-25 14:57msvAssigned Togka => ika
2015-12-25 14:57msvStatusresolved => assigned
2015-12-28 15:31gkaNote Added: 0049638
2015-12-28 15:33gitNote Added: 0049639
2015-12-28 15:36ikaNote Added: 0049640
2015-12-28 15:36ikaAssigned Toika => msv
2015-12-28 15:36ikaStatusassigned => resolved
2015-12-28 16:13msvNote Added: 0049641
2015-12-28 16:13msvAssigned Tomsv => bugmaster
2015-12-28 16:13msvStatusresolved => reviewed
2015-12-28 17:23Roman LyginNote Added: 0049649
2015-12-28 18:07apvAssigned Tobugmaster => apv
2015-12-28 19:08msvAssigned Toapv => msv
2015-12-28 19:08msvStatusreviewed => assigned
2015-12-28 19:17gitNote Added: 0049658
2015-12-28 19:17msvStatusassigned => resolved
2015-12-28 19:19msvNote Added: 0049659
2015-12-28 19:19msvAssigned Tomsv => bugmaster
2015-12-28 19:19msvStatusresolved => reviewed
2016-01-11 11:59apvAssigned Tobugmaster => apv
2016-01-12 13:16apvTest case number => bugs iges bug26931
2016-01-12 13:19apvNote Added: 0049781
2016-01-12 13:19apvAssigned Toapv => bugmaster
2016-01-12 13:19apvStatusreviewed => tested
2016-01-12 14:38apvNote Edited: 0049781bug_revision_view_page.php?bugnote_id=49781#r12733
2016-01-15 16:57bugmasterChangeset attached => occt master 369a38aa
2016-01-15 16:57bugmasterStatustested => verified
2016-01-15 16:57bugmasterResolutionopen => fixed
2016-04-17 13:46gitNote Added: 0052952
2016-04-17 13:46gitNote Added: 0052953
2016-04-20 15:43aivFixed in Version => 7.0.0
2016-04-20 15:50aivStatusverified => closed

Notes
(0048717)
git   
2015-12-03 11:39   
Branch CR26931 has been created by ika.

SHA-1: b99d2c3dc6df6d3d84fd4f975e446b92054b8750


Detailed log of new commits:

Author: ika
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.
(0048718)
ika   
2015-12-03 11:41   
Dear GKA,
could you please review branch CR26931?
(0049206)
gka   
2015-12-16 14:13   
Seems that this fix can not resolve this problem in the common case
(0049505)
git   
2015-12-23 14:07   
Branch CR26931 has been updated forcibly by ika.

SHA-1: a6a9f2f9b44fec4b0e7e5e5e67802c5f682edad2
(0049506)
ika   
2015-12-23 14:09   
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?
(0049507)
Roman Lygin   
2015-12-23 14:41   
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.
(0049525)
msv   
2015-12-23 16:33   
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)".
(0049558)
ifv   
2015-12-24 13:43   
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(...).
(0049592)
git   
2015-12-25 13:25   
Branch CR26931 has been updated by ika.

SHA-1: be5c0b2b36b42f32cb9a7473d91f7b48477e1742


Detailed log of new commits:

Author: ika
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.

(0049593)
ika   
2015-12-25 13:28   
Dear colleagues,
thank you for your remarks, branch CR 26931 was updated, according to it.

Dear GKA,
could you please review the changes?
(0049596)
msv   
2015-12-25 14:57   
src\Geom\Geom_BSplineSurface.hxx

1) My second remark in the comment 0026931:0049525 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.
(0049638)
gka   
2015-12-28 15:31   
I have not any remarks to modifications of the class GeomToIges_GeomSurface
(0049639)
git   
2015-12-28 15:33   
Branch CR26931 has been updated forcibly by ika.

SHA-1: b44636b572e30b528a014c3d36299fa47d5b3b57
(0049640)
ika   
2015-12-28 15:36   
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?
(0049641)
msv   
2015-12-28 16:13   
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).
(0049649)
Roman Lygin   
2015-12-28 17:23   
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.
(0049658)
git   
2015-12-28 19:17   
Branch CR26931_1 has been created by msv.

SHA-1: a462dd8ba686a8d61fda32e6e1594dd4a6cc74a5


Detailed log of new commits:

Author: ika
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.
(0049659)
msv   
2015-12-28 19:19   
Please test the branch CR26931_1. It contains the changes that follow the above comments of Roman and me.
(0049781)
apv   
2016-01-12 13:19   
(edited on: 2016-01-12 14:38)
Dear BugMaster,

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

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%]

(0052952)
git   
2016-04-17 13:46   
Branch CR26931 has been deleted by kgv.

SHA-1: b44636b572e30b528a014c3d36299fa47d5b3b57
(0052953)
git   
2016-04-17 13:46   
Branch CR26931_1 has been deleted by kgv.

SHA-1: a462dd8ba686a8d61fda32e6e1594dd4a6cc74a5