View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0026931CommunityOCCT:Data Exchangepublic2015-11-27 13:042016-04-20 15:50
ReporterRoman Lygin Assigned Tobugmaster  
PrioritynormalSeveritycrash 
Status closedResolutionfixed 
Product Version6.9.1 
Target Version7.0.0Fixed in Version7.0.0 
Summary0026931: [Regression in 6.9.0] Exporting a face throws an exception
DescriptionThe 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.
Steps To Reproducebugs iges bug26931
TagsNo tags attached.
Test case numberbugs iges bug26931

Attached Files

  • b_14.zip (6,151 bytes)
  • bug26931.brep (24,758 bytes)

Relationships

related to 0026138 closedbugmaster Open CASCADE Problems with writing periodic BSplines into IGES 
related to 0026989 closedbugmaster Community [Regression in 6.9.0] Export of a reversed face leads to wrong data in 6.9.0 

Activities

Roman Lygin

2015-11-27 13:04

developer  

b_14.zip (6,151 bytes)

git

2015-12-03 11:39

administrator   ~0048717

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.

ika

2015-12-03 11:40

developer  

bug26931.brep (24,758 bytes)

ika

2015-12-03 11:41

developer   ~0048718

Dear GKA,
could you please review branch CR26931?

gka

2015-12-16 14:13

developer   ~0049206

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

git

2015-12-23 14:07

administrator   ~0049505

Branch CR26931 has been updated forcibly by ika.

SHA-1: a6a9f2f9b44fec4b0e7e5e5e67802c5f682edad2

ika

2015-12-23 14:09

developer   ~0049506

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?

Roman Lygin

2015-12-23 14:41

developer   ~0049507

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.

msv

2015-12-23 16:33

developer   ~0049525

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)".

ifv

2015-12-24 13:43

developer   ~0049558

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(...).

git

2015-12-25 13:25

administrator   ~0049592

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.

ika

2015-12-25 13:28

developer   ~0049593

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

Dear GKA,
could you please review the changes?

msv

2015-12-25 14:57

developer   ~0049596

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.

gka

2015-12-28 15:31

developer   ~0049638

I have not any remarks to modifications of the class GeomToIges_GeomSurface

git

2015-12-28 15:33

administrator   ~0049639

Branch CR26931 has been updated forcibly by ika.

SHA-1: b44636b572e30b528a014c3d36299fa47d5b3b57

ika

2015-12-28 15:36

developer   ~0049640

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?

msv

2015-12-28 16:13

developer   ~0049641

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).

Roman Lygin

2015-12-28 17:23

developer   ~0049649

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.

git

2015-12-28 19:17

administrator   ~0049658

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.

msv

2015-12-28 19:19

developer   ~0049659

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

apv

2016-01-12 13:19

tester   ~0049781

Last edited: 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%]

git

2016-04-17 13:46

administrator   ~0052952

Branch CR26931 has been deleted by kgv.

SHA-1: b44636b572e30b528a014c3d36299fa47d5b3b57

git

2016-04-17 13:46

administrator   ~0052953

Branch CR26931_1 has been deleted by kgv.

SHA-1: a462dd8ba686a8d61fda32e6e1594dd4a6cc74a5

Related Changesets

occt: master 369a38aa

2015-12-03 08:35:42

ika


Committer: bugmaster Details Diff		 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.		 Affected Issues
0026931
mod - src/Geom/Geom_BSplineCurve.cxx Diff File
mod - src/Geom/Geom_BSplineCurve.hxx Diff File
mod - src/Geom/Geom_BSplineSurface.cxx Diff File
mod - src/Geom/Geom_BSplineSurface.hxx Diff File
mod - src/Geom/Geom_BSplineSurface_1.cxx Diff File
mod - src/Geom2d/Geom2d_BSplineCurve.cxx Diff File
mod - src/Geom2d/Geom2d_BSplineCurve.hxx Diff File
mod - src/GeomToIGES/GeomToIGES_GeomSurface.cxx Diff File
add - tests/bugs/iges/bug26931 Diff File
mod - tests/bugs/moddata_1/bug14782 Diff File

Issue History

Date Modified Username Field Change
2015-11-27 13:04 Roman Lygin New Issue
2015-11-27 13:04 Roman Lygin Assigned To => gka
2015-11-27 13:04 Roman Lygin File Added: b_14.zip
2015-11-27 13:29 gka Assigned To gka => ika
2015-11-27 13:29 gka Status new => assigned
2015-11-27 14:30 abv Relationship added related to 0026138
2015-12-03 11:39 git Note Added: 0048717
2015-12-03 11:40 ika File Added: bug26931.brep
2015-12-03 11:41 ika Note Added: 0048718
2015-12-03 11:41 ika Assigned To ika => gka
2015-12-03 11:41 ika Status assigned => resolved
2015-12-03 11:41 ika Steps to Reproduce Updated
2015-12-13 13:29 Roman Lygin Relationship added related to 0026989
2015-12-16 14:13 gka Note Added: 0049206
2015-12-16 14:13 gka Assigned To gka => ika
2015-12-16 14:13 gka Status resolved => assigned
2015-12-23 14:07 git Note Added: 0049505
2015-12-23 14:09 ika Note Added: 0049506
2015-12-23 14:09 ika Assigned To ika => gka
2015-12-23 14:09 ika Status assigned => resolved
2015-12-23 14:41 Roman Lygin Note Added: 0049507
2015-12-23 16:33 msv Note Added: 0049525
2015-12-23 16:33 msv Assigned To gka => ika
2015-12-23 16:33 msv Status resolved => assigned
2015-12-24 13:43 ifv Note Added: 0049558
2015-12-25 13:25 git Note Added: 0049592
2015-12-25 13:28 ika Note Added: 0049593
2015-12-25 13:28 ika Assigned To ika => gka
2015-12-25 13:28 ika Status assigned => resolved
2015-12-25 14:57 msv Note Added: 0049596
2015-12-25 14:57 msv Assigned To gka => ika
2015-12-25 14:57 msv Status resolved => assigned
2015-12-28 15:31 gka Note Added: 0049638
2015-12-28 15:33 git Note Added: 0049639
2015-12-28 15:36 ika Note Added: 0049640
2015-12-28 15:36 ika Assigned To ika => msv
2015-12-28 15:36 ika Status assigned => resolved
2015-12-28 16:13 msv Note Added: 0049641
2015-12-28 16:13 msv Assigned To msv => bugmaster
2015-12-28 16:13 msv Status resolved => reviewed
2015-12-28 17:23 Roman Lygin Note Added: 0049649
2015-12-28 18:07 apv Assigned To bugmaster => apv
2015-12-28 19:08 msv Assigned To apv => msv
2015-12-28 19:08 msv Status reviewed => assigned
2015-12-28 19:17 git Note Added: 0049658
2015-12-28 19:17 msv Status assigned => resolved
2015-12-28 19:19 msv Note Added: 0049659
2015-12-28 19:19 msv Assigned To msv => bugmaster
2015-12-28 19:19 msv Status resolved => reviewed
2016-01-11 11:59 apv Assigned To bugmaster => apv
2016-01-12 13:16 apv Test case number => bugs iges bug26931
2016-01-12 13:19 apv Note Added: 0049781
2016-01-12 13:19 apv Assigned To apv => bugmaster
2016-01-12 13:19 apv Status reviewed => tested
2016-01-12 14:38 apv Note Edited: 0049781
2016-01-15 16:57 bugmaster Changeset attached => occt master 369a38aa
2016-01-15 16:57 bugmaster Status tested => verified
2016-01-15 16:57 bugmaster Resolution open => fixed
2016-04-17 13:46 git Note Added: 0052952
2016-04-17 13:46 git Note Added: 0052953
2016-04-20 15:43 aiv Fixed in Version => 7.0.0
2016-04-20 15:50 aiv Status verified => closed