MantisBT - Open CASCADE
View Issue Details
0026265Open CASCADE[OCCT] OCCT:Modeling Algorithmspublic2015-05-25 19:442020-09-14 22:55
[OCCT] 6.9.0 
[OCCT] 7.6.0* 
0026265: Incorrect result of 2d offset
Incorrect result of 2d offset operation.
It is reproduced in the test case "offset wire_unclosed_outside_0_025 B4".
offset wire_unclosed_outside_0_025 B4
No tags attached.
related to 0027890closed kgv Open CASCADE BndLib_Add2dCurve::Add(..) works incorrect on some curves 
related to 0028571new msv Community BRepOffsetAPI_MakeOffset build invalid wire 
png offset_KO.PNG (9,997) 2015-05-25 19:44
png offset_OK.PNG (9,341) 2015-05-25 19:45
? offset_OK.brep (87,761) 2015-05-25 19:46
Issue History
2015-05-25 19:44msvNew Issue
2015-05-25 19:44msvAssigned To => msv
2015-05-25 19:44msvFile Added: offset_KO.PNG
2015-05-25 19:45msvFile Added: offset_OK.PNG
2015-05-25 19:46msvFile Added: offset_OK.brep
2015-05-25 19:48msvNote Added: 0041530
2015-05-26 10:45msvAssigned Tomsv => isn
2015-05-26 10:45msvStatusnew => assigned
2015-07-24 15:48gitNote Added: 0043418
2015-07-24 17:24gitNote Added: 0043431
2015-07-27 11:32gitNote Added: 0043446
2015-07-27 11:35gitNote Added: 0043447
2015-07-27 14:08gitNote Added: 0043558
2015-07-27 20:25gitNote Added: 0043606
2015-07-28 15:02gitNote Added: 0043636
2015-08-05 13:27gitNote Added: 0043843
2015-08-05 14:08gitNote Added: 0043847
2015-08-06 13:05gitNote Added: 0043876
2015-08-07 17:23gitNote Added: 0043927
2015-08-11 17:26gitNote Added: 0043984
2015-08-11 19:05gitNote Added: 0043999
2015-08-12 10:48gitNote Added: 0044019
2015-08-12 11:03gitNote Added: 0044020
2015-09-24 17:10gitNote Added: 0046098
2015-09-24 17:11msvNote Added: 0046099
2016-09-13 16:04gitNote Added: 0057746
2016-09-28 11:22gitNote Added: 0058237
2016-09-28 11:23isnAssigned Toisn => msv
2016-09-28 11:23isnStatusassigned => resolved
2016-09-28 11:37isnNote Added: 0058239
2016-09-28 11:38isnNote Edited: 0058239bug_revision_view_page.php?bugnote_id=58239#r14820
2016-09-28 14:45msvRelationship addedrelated to 0027890
2016-09-28 14:46msvNote Added: 0058262
2016-09-28 15:09isnNote Added: 0058264
2016-09-28 15:10isnNote Edited: 0058264bug_revision_view_page.php?bugnote_id=58264#r14827
2016-09-28 21:12msvNote Added: 0058289
2016-09-28 21:12msvAssigned Tomsv => isn
2016-09-28 21:12msvStatusresolved => assigned
2016-10-28 16:28msvTarget Version7.1.0 => 7.2.0
2017-02-14 12:21gitNote Added: 0063793
2017-02-14 12:33isnAssigned Toisn => msv
2017-02-14 12:33isnStatusassigned => feedback
2017-03-20 09:53msvRelationship addedrelated to 0028571
2017-03-21 17:51msvNote Added: 0064587
2017-03-21 17:51msvAssigned Tomsv => isn
2017-03-21 17:51msvStatusfeedback => assigned
2017-05-24 11:18msvNote Added: 0066503
2017-07-24 09:22msvTarget Version7.2.0 => 7.3.0
2017-12-05 17:09msvTarget Version7.3.0 => 7.4.0
2019-08-12 16:44msvTarget Version7.4.0 => 7.5.0
2020-09-14 22:55msvTarget Version7.5.0 => 7.6.0*

2015-05-25 19:48   
The correct result should look as in the picture offset_OK.png
The shape offset_OK.brep has been got by manually splitting intersected edges in Draw and collecting necessary edges in the compound.
2015-07-24 15:48   
Branch CR26265 has been created by isn.

SHA-1: c18ad6f7d4cc17cd308c7c2f21917c688dbb3571

Detailed log of new commits:

Author: isn
Date: Fri Jul 24 12:54:16 2015 +0300

    0026265: Incorrect result of 2d offset
    very draft version
2015-07-24 17:24   
Branch CR26265 has been updated by isn.

SHA-1: 20251080e1742bc6117bb3b31809cf426ec6920c

Detailed log of new commits:

Author: isn
Date: Fri Jul 24 17:24:36 2015 +0300

    get rid of mutable key on listoflinks

2015-07-27 11:32   
Branch CR26265 has been updated by isn.

SHA-1: 70e6ed80d4eb27deeab3a4050d2c0768ba81d88a

Detailed log of new commits:

Author: isn
Date: Mon Jul 27 11:32:31 2015 +0300

    locations of curves have been added

2015-07-27 11:35   
Branch CR26265 has been updated forcibly by isn.

SHA-1: 62bfe55abac418895a07053618a25a7a027423c6
2015-07-27 14:08   
Branch CR26265 has been updated forcibly by isn.

SHA-1: 69e05a4acbf0d9631ca64f76c1edebaca4a8375b
2015-07-27 20:25   
Branch CR26265 has been updated by isn.

SHA-1: 33135e743db92af19ec0599b9edea12d4b4a9f60

Detailed log of new commits:

Author: isn
Date: Mon Jul 27 20:25:30 2015 +0300

    1) more accurate method to find links with same nodes but diff. orientation
    2) take into account a self-intersected wires..

2015-07-28 15:02   
Branch CR26265 has been updated by isn.

SHA-1: 461217b0503237a20685b12330d3591d75e61ef7

Detailed log of new commits:

Author: isn
Date: Tue Jul 28 15:02:14 2015 +0300

    improving of stability

2015-08-05 13:27   
Branch CR26265 has been updated by isn.

SHA-1: d77104e39597eeb20bcbaffe07d2bc62bb579ed5

Detailed log of new commits:

Author: isn
Date: Wed Aug 5 13:27:09 2015 +0300


2015-08-05 14:08   
Branch CR26265 has been updated forcibly by isn.

SHA-1: 293d0f6ddebc674a17d6b7d2349d2071116ddfd8
2015-08-06 13:05   
Branch CR26265 has been updated by isn.

SHA-1: 9c5f8aa02426037afac80ca5e792610880347fb0

Detailed log of new commits:

Author: isn
Date: Thu Aug 6 11:25:53 2015 +0300

    get rid of std::max
    eliminate warnings
    check BRepLib_MakeEdge on IsDone()

2015-08-07 17:23   
Branch CR26265 has been updated by isn.

SHA-1: 9eb81cc39a67f44f14b9e4a5c12317f081622ef9

Detailed log of new commits:

Author: isn
Date: Fri Aug 7 17:22:22 2015 +0300

    remarks # 1

2015-08-11 17:26   
Branch CR26265 has been updated by isn.

SHA-1: 25c914a60422d285940f89bfc77c489d4a8418d7

Detailed log of new commits:

Author: isn
Date: Tue Aug 11 17:25:47 2015 +0300

    extract one-edge-loops from wire before passing to MakeLoops algo

2015-08-11 19:05   
Branch CR26265 has been updated forcibly by isn.

SHA-1: a02c7b3cd23472f0db71708f79a1ffe3309705f1
2015-08-12 10:48   
Branch CR26265 has been updated forcibly by isn.

SHA-1: dc494f8429d5880b8d4722cad3353536fa1205b9
2015-08-12 11:03   
Branch CR26265 has been updated forcibly by isn.

SHA-1: fc5d3cab9fd7d0613bb0c2977074b391f14e16f8
2015-09-24 17:10   
Branch CR26265_1 has been created by msv.

SHA-1: 707cb1776b2c8b9ec44ca3bbbe9dbe0438c3112b

Detailed log of new commits:

Author: isn
Date: Fri Jul 24 12:54:16 2015 +0300

    0026265: Incorrect result of 2d offset
    Additional treatment has been added as the last step of the offset procedure to remove wrong parts (loops) from the result.
2015-09-24 17:11   
The fix has been re-based on current master and combined in one commit.
2016-09-13 16:04   
Branch CR26265_2 has been created by isn.

SHA-1: 54c708c94594eab8930784724505a2559fb161eb

Detailed log of new commits:

Author: isn
Date: Fri Jul 24 12:54:16 2015 +0300

    0026265: Incorrect result of 2d offset
    Additional treatment has been added as the last step of the offset procedure to remove wrong parts (loops) from the result.
2016-09-28 11:22   
Branch CR26265_3 has been created by isn.

SHA-1: 73e83cf785f01bba2b32f368313e6d5656e4c8b5

Detailed log of new commits:

Author: isn
Date: Wed Sep 28 11:20:01 2016 +0300

    0026265: Incorrect result of 2d offset
    complete redesign of prev. version + optimizations + regressions

Author: isn
Date: Tue Sep 27 20:19:31 2016 +0300

    reversed meaning of -o option

Author: isn
Date: Mon Sep 26 11:48:39 2016 +0300

    actual fix

Author: isn
Date: Thu Sep 22 14:42:30 2016 +0300

    0027890: BndLib_Add2dCurve::Add(..) works incorrect on some curves
    new testcase: extension for 'gbounding' command. now this command allows to pass curve2d as args and works in two modes ('normal' and optimal)
2016-09-28 11:37   
(edited on: 2016-09-28 11:38)
this new branch is on top of cr27890, just because the changes from 27890 are not yet rebased on current master

2016-09-28 14:46   
Dear Ilya, please explain here what is relation with 27890.
2016-09-28 15:09   
(edited on: 2016-09-28 15:10)
The new method which called by BRepFill_OffsetWire algo uses bounding boxes to speed up intersection process between (offset) edges (more precisely, their pcurves). In some cases the retrieved result from BndLib_Add2dCurve::Add(..) is incorrect (wrong bnd box) which leads to reduction of number of found intersections. That's why this fix should be applied on top of the 27890...

2016-09-28 21:12   
- Please use a standard header before each function:
//function : <name>
//purpose :

- Please wrap long lines (> 100 characters).

- At 479-489: make all local functions static.

- At 1430: don't we need to put the new code under condition "!myIsOpenResult"?

- In many places local variables are named without prefix "a" starting with capital letter (IWires, HoleWires, ME2IP, IsMaxIsHole, Exp, etc.). Please, start them with "a", "is", or name starting with lowercase letter the names of iterators, like i, it, exp.

- Please make fake declarations in private section:
  BRepFill_BndBoxTreeSelector(const BRepFill_BndBoxTreeSelector& );
  BRepFill_BndBoxTreeSelector& operator=(const BRepFill_BndBoxTreeSelector& );

- In the method FindWireIntersections (line 3088), replace TopTools_SequenceOfShape with another container with quick direct access to items by index. I propose to use MapShapes to create IndexedMap.

- At 176, 255: creation of adaptor can be expensive operation, so it is better to cache them in some data map.

- Question at 3103: yes, use cache.

- At 194, it is better not to use 3d curves, instead use D0 of adaptor 2d, then D0 of the surface.

- At 276, location is not taken into account for p3d2.

- Why coefficient 1.01 at line 279 differs from the similar by meaning at line 206?

- Question at 183: if we create global map for all vertices at once then checking for coincidence will take too much time, and indeed we do not expect intersections with vertices from other edges here.

- Question at 214: it is OK.

- At 221: if end of one edge intersect the other edge in the middle we must split that edge, but not create new vertex. In your code, in such case the edge will not be split, which is incorrect.

- In the case of self-intersections, should not we consider the case of vertex-edge intersection?

- The method GetResult returns result map by assigning operator. It is not good. It is better to store the reference on the result container given in constructor, as it is done with mySeqOfEdges.

- Question at 322: for better readability you can use typedef.

- I see no need in using NCollection_DataMap<Standard_Real, TopoDS_Vertex> for each edge. Instead, you can use a list of structures containing param and vertex.

- At 3099: we know the size of BndBoxesOfEdges in advance. Therefore it is better to use Array1 instead of sequence for its type.

- Question at 3129: which "internal" do you mean? This function is static. It is enough to use local reshape.

- At 3136, use BRep_Tool::Range instead of Curve.

- At 3148: use std::sort instead of NCollection_QuickSort (see 0027915).

- Lines 3169-3170 can be moved after 3165.

- Question at 3189: Why reordering of wire is needed at all? Do some edges really need to be reversed? We should not allow reverse orientation of offsets of original edges. If this occurs with some edge we must skip such wire.

- Question at 3256: yes, use cached adaptor.

- Line 3277 is not needed, flags are initialized with this value by default.

- At 330: the name Poly_Helper makes me think that this class should be defined in the package Poly. Please use another prefix appropriate here.

- Line 338 is extra.

- At 345: no need to implement the copy constructor for this class. Make it and operator= private.

- Question at 3323: presence of hanging links may mean that some intersection/splitting was not done correctly. In this case, we can indeed skip the wire from fixing it, at least till the moment we know other cases when we should do something else.

- Question at 3342: non-closed wire cannot appear here; if it is then some error exists in the algorithm. You can put Standard_ASSERT_VOID here.

- Question at 3389: PerformInfinitePoint must never return status other than IN or OUT. You can put assert in other case.

- At 3356: it is unsafe to use a pure pointer. Please use here some kind of smart pointer, like NCollection_Handle.

- At 3395: why 0 is used for Eps parameter? Algorithm can run too long in this case.

- It is unclear why we reject the cases indicated by the condition on line 3418.

- At 3380-3391: It is unclear why cannot we determine the wire is hole or not by checking the sign of surface area. We could get rid of necessity of creation of FClass2d for each wire. Instead, just to remember 'af' face to create FClass2d later.

- Question at 3379: for the most outer wire the mass cannot be very small.
2017-02-14 12:21   
Branch CR26265_3 has been updated by isn.

SHA-1: f4729d5ded78c3d9528f3b545e68cffe02fad048

Detailed log of new commits:

Author: isn
Date: Thu Jan 12 19:28:29 2017 +0300

    processing of E/E and E/V intersection segments

Author: isn
Date: Mon Oct 3 11:03:28 2016 +0300

    first remarks

2017-03-21 17:51   

- Please use a standard header before each function.
- make methods IsEqual and HashCode inline instead of static.
- empty ctor of BRepFill_IntersectionParam must initialize the field param.
- 224: this optimization may not help, leave for a while.
- 298: use Max(TolE1, TolE2)
- 310: use Square(TolV+aFutTol)
- 413: in this case it is better to avoid usage of map, just get two vertices and compare. BTW, put computation of invariant out of the loop.
- 435: not for a while
- 439: the declared here check is not performed. Can the method ProcessIPoints be adopted for this case?
- 649: add 'static' declaration.
- 650-687: such declarations are used when the usage of function precedes its definition. You placed definitions before their usage. It can (and does!) lead to errors. Look at the function FindWireIntersections. Its last argument 'theAddtEdgesInt' has different types in declaration and in definition, and this fact makes declaration not relevant to the definition without any compilation warning. You should either remove all these declarations (this is preferred) or place functions definitions in different order.
- 1628: don't we need to put the new code under condition "!myIsOpenResult"?
- 3345: yes, if all splits are considered. And the argument theAddtEdgesInt is extra here. Moreover, I think the map theAddtEdgesInt is not needed at all. It just makes the code complex. And it duplicates info from ME2IP.
- 3357: use TopExp::Vertices to get vertices of an edge.
- 3365,3366: too long lines.
- 3366-3375: why indentation is 3?
- 3382-3383: use TopExp::Vertices to get both vertices of an edge.
- 3411-3421: avoid map rebuilding, just skip unnecessary items when it is used.
- 3434: use const &
- 3467: make the function BRepFill_ParamComp inline.
- 3499: it's better to insert operator<() in the struct BRepFill_IntersectionParam and use function sort with two arguments.
- 3478: remove this comment.
- 3486: use const &
- 3517: OK
- 3523: it is better to make check in 3308, making sure there is no deg edges in processing at all.
- 3532: you control building of wire yourself, so it's better to use low-level builder here.
- 3555: usage NCollection_IndexedMap as item in SV2CE is not justified. It's better to check for existence of added edge by iteration in UpdateSV2CEMap.
- 3568: use const &. Removal from CEM is not necessary, it's better just to iterate all its items.
- 3569: usage NCollection_IndexedMap as item in AllGE is not justified. It's better to use the list.
- 3613: too long line
- 3633-3634: use TopExp::Vertices to get both vertices of an edge.
- 3709: too long line
- 3721: use const &
- 3723: use BRep_Builder to make wire here.
- 3735: set the flag yourself.
- 3745,3747: too long line
- 3766: use BRep_Builder
- 3774: avoid creation of classifier for each wire. First select the most outer using only SurfaceProperties. And only for it create classifier. Then no smart pointer of BRepTopAdaptor_FClass2d will be needed.
- Use dedicated handle class BRepAdaptor_HCurve2d instead of NCollection_Handle<BRepAdaptor_Curve2d>.
- 3848-3854: you can use the method BRep_Tool::MaxTolerance instead.
- 3855: I think it is needed to limit it to be not greater than 1% of max extension (bounding box of all vertices).
- 3857: why oriented hasher is used here? Analyzing the further code does not prove its necessity. It's better to use TopTools_IndexedDataMapOfShapeListOfShape.
- 3894: iteration on data map with key depending on memory address can lead to unstable run of the algorithm. If you use indexed data map this pb will be gone.
- 3860: extra container. It is enough to use allV.
- 3870: it is needed to add vertex tolerance as gap to the box.
- 3905: If wire has no intersections but vertices were fused we should apply this fusion to output (change wire).
- 3947: use 'the' prefix only for function arguments.
- use TopTools_IndexedDataMapOfShapeShape instead of NCollection_IndexedDataMap<TopoDS_Edge, TopoDS_Edge>.
- use TopTools_IndexedMapOfShape instead of NCollection_IndexedMap<TopoDS_Vertex,TopTools_ShapeMapHasher>
- use TopTools_IndexedMapOfShape instead of NCollection_IndexedMap<TopoDS_Edge>
2017-05-24 11:18   
Ilya, what is the current state of this bug?