View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0026265 | Open CASCADE | OCCT:Modeling Algorithms | public | 2015-05-25 19:44 | 2023-08-01 15:08 |
| Reporter | Assigned To | ||||
| Priority | normal | Severity | minor | ||
| Status | assigned | Resolution | open | ||
| Product Version | 6.9.0 | ||||
| Target Version | Unscheduled | ||||
| Summary | 0026265: Incorrect result of 2d offset | ||||
| Description | Incorrect result of 2d offset operation. It is reproduced in the test case "offset wire_unclosed_outside_0_025 B4". | ||||
| Steps To Reproduce | offset wire_unclosed_outside_0_025 B4 | ||||
| Tags | No tags attached. | ||||
| Test case number | |||||
|
2015-05-25 19:44 developer |
offset_KO.PNG (9,997 bytes) |
|
2015-05-25 19:45 developer |
offset_OK.PNG (9,341 bytes) |
|
2015-05-25 19:46 developer |
offset_OK.brep (87,761 bytes) |
|
|
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. |
|
|
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 |
|
|
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 |
|
|
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 |
|
|
Branch CR26265 has been updated forcibly by isn. SHA-1: 62bfe55abac418895a07053618a25a7a027423c6 |
|
|
Branch CR26265 has been updated forcibly by isn. SHA-1: 69e05a4acbf0d9631ca64f76c1edebaca4a8375b |
|
|
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.. |
|
|
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 |
|
|
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 comments |
|
|
Branch CR26265 has been updated forcibly by isn. SHA-1: 293d0f6ddebc674a17d6b7d2349d2071116ddfd8 |
|
|
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() |
|
|
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 |
|
|
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 |
|
|
Branch CR26265 has been updated forcibly by isn. SHA-1: a02c7b3cd23472f0db71708f79a1ffe3309705f1 |
|
|
Branch CR26265 has been updated forcibly by isn. SHA-1: dc494f8429d5880b8d4722cad3353536fa1205b9 |
|
|
Branch CR26265 has been updated forcibly by isn. SHA-1: fc5d3cab9fd7d0613bb0c2977074b391f14e16f8 |
|
|
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. |
|
|
The fix has been re-based on current master and combined in one commit. |
|
|
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. |
|
|
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) |
|
|
this new branch is on top of cr27890, just because the changes from 27890 are not yet rebased on current master |
|
|
Dear Ilya, please explain here what is relation with 27890. |
|
|
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... |
|
|
- 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. |
|
|
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 |
|
|
Remarks: - 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> |
|
|
Ilya, what is the current state of this bug? |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2015-05-25 19:44 |
|
New Issue | |
| 2015-05-25 19:44 |
|
Assigned To | => msv |
| 2015-05-25 19:44 |
|
File Added: offset_KO.PNG | |
| 2015-05-25 19:45 |
|
File Added: offset_OK.PNG | |
| 2015-05-25 19:46 |
|
File Added: offset_OK.brep | |
| 2015-05-25 19:48 |
|
Note Added: 0041530 | |
| 2015-05-26 10:45 |
|
Assigned To | msv => isn |
| 2015-05-26 10:45 |
|
Status | new => assigned |
| 2015-07-24 15:48 | git | Note Added: 0043418 | |
| 2015-07-24 17:24 | git | Note Added: 0043431 | |
| 2015-07-27 11:32 | git | Note Added: 0043446 | |
| 2015-07-27 11:35 | git | Note Added: 0043447 | |
| 2015-07-27 14:08 | git | Note Added: 0043558 | |
| 2015-07-27 20:25 | git | Note Added: 0043606 | |
| 2015-07-28 15:02 | git | Note Added: 0043636 | |
| 2015-08-05 13:27 | git | Note Added: 0043843 | |
| 2015-08-05 14:08 | git | Note Added: 0043847 | |
| 2015-08-06 13:05 | git | Note Added: 0043876 | |
| 2015-08-07 17:23 | git | Note Added: 0043927 | |
| 2015-08-11 17:26 | git | Note Added: 0043984 | |
| 2015-08-11 19:05 | git | Note Added: 0043999 | |
| 2015-08-12 10:48 | git | Note Added: 0044019 | |
| 2015-08-12 11:03 | git | Note Added: 0044020 | |
| 2015-09-24 17:10 | git | Note Added: 0046098 | |
| 2015-09-24 17:11 |
|
Note Added: 0046099 | |
| 2016-09-13 16:04 | git | Note Added: 0057746 | |
| 2016-09-28 11:22 | git | Note Added: 0058237 | |
| 2016-09-28 11:23 |
|
Assigned To | isn => msv |
| 2016-09-28 11:23 |
|
Status | assigned => resolved |
| 2016-09-28 11:37 |
|
Note Added: 0058239 | |
| 2016-09-28 11:38 |
|
Note Edited: 0058239 | |
| 2016-09-28 14:45 |
|
Relationship added | related to 0027890 |
| 2016-09-28 14:46 |
|
Note Added: 0058262 | |
| 2016-09-28 15:09 |
|
Note Added: 0058264 | |
| 2016-09-28 15:10 |
|
Note Edited: 0058264 | |
| 2016-09-28 21:12 |
|
Note Added: 0058289 | |
| 2016-09-28 21:12 |
|
Assigned To | msv => isn |
| 2016-09-28 21:12 |
|
Status | resolved => assigned |
| 2016-10-28 16:28 |
|
Target Version | 7.1.0 => 7.2.0 |
| 2017-02-14 12:21 | git | Note Added: 0063793 | |
| 2017-02-14 12:33 |
|
Assigned To | isn => msv |
| 2017-02-14 12:33 |
|
Status | assigned => feedback |
| 2017-03-20 09:53 |
|
Relationship added | related to 0028571 |
| 2017-03-21 17:51 |
|
Note Added: 0064587 | |
| 2017-03-21 17:51 |
|
Assigned To | msv => isn |
| 2017-03-21 17:51 |
|
Status | feedback => assigned |
| 2017-05-24 11:18 |
|
Note Added: 0066503 | |
| 2017-07-24 09:22 |
|
Target Version | 7.2.0 => 7.3.0 |
| 2017-12-05 17:09 |
|
Target Version | 7.3.0 => 7.4.0 |
| 2019-08-12 16:44 |
|
Target Version | 7.4.0 => 7.5.0 |
| 2020-09-14 22:55 |
|
Target Version | 7.5.0 => 7.6.0 |
| 2021-08-29 18:52 |
|
Target Version | 7.6.0 => 7.7.0 |
| 2022-10-24 10:42 |
|
Target Version | 7.7.0 => 7.8.0 |
| 2023-08-01 15:08 | dpasukhi | Target Version | 7.8.0 => Unscheduled |
