MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0026931Community[OCCT] OCCT:Data Exchangepublic2015-11-27 13:042016-04-20 15:50
ReporterRoman Lygin 
Assigned Tobugmaster 
PrioritynormalSeveritycrash 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version[OCCT] 6.9.1 
Target Version[OCCT] 7.0.0Fixed in Version[OCCT] 7.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 Fileszip file icon b_14.zip (6,151 bytes) 2015-11-27 13:04
? file icon bug26931.brep (24,758 bytes) 2015-12-03 11:40

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

-  Notes
(0048717)
git (administrator)
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 (developer)
2015-12-03 11:41

Dear GKA,
could you please review branch CR26931?
(0049206)
gka (developer)
2015-12-16 14:13

Seems that this fix can not resolve this problem in the common case
(0049505)
git (administrator)
2015-12-23 14:07

Branch CR26931 has been updated forcibly by ika.

SHA-1: a6a9f2f9b44fec4b0e7e5e5e67802c5f682edad2
(0049506)
ika (developer)
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 (developer)
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 (developer)
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 (developer)
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 (administrator)
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 (developer)
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 (developer)
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 (developer)
2015-12-28 15:31

I have not any remarks to modifications of the class GeomToIges_GeomSurface
(0049639)
git (administrator)
2015-12-28 15:33

Branch CR26931 has been updated forcibly by ika.

SHA-1: b44636b572e30b528a014c3d36299fa47d5b3b57
(0049640)
ika (developer)
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 (developer)
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 (developer)
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 (administrator)
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 (developer)
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 (tester)
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 (administrator)
2016-04-17 13:46

Branch CR26931 has been deleted by kgv.

SHA-1: b44636b572e30b528a014c3d36299fa47d5b3b57
(0052953)
git (administrator)
2016-04-17 13:46

Branch CR26931_1 has been deleted by kgv.

SHA-1: a462dd8ba686a8d61fda32e6e1594dd4a6cc74a5

- Related Changesets
occt: master 369a38aa
Timestamp: 2015-12-03 08:35:42
Author: 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.
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 View Revisions
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 View Revisions
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 user533 Fixed in Version => 7.0.0
2016-04-20 15:50 user533 Status verified => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker