View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0026682CommunityOCCT:Modeling Datapublic2015-09-13 19:052017-09-29 16:24
ReporterVico Liang Assigned Tomkv 
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Target Version7.2.0Fixed in Version7.2.0 
Summary0026682: TopExp::MapShapesAndAncestors() will build map with duplicated ancestors.
DescriptionCode snippets:

TopTools_IndexedDataMapOfShapeListOfShape aMapVFs;
BRepPrimAPI_MakeBox aBox(10,10,10);
TopExp::MapShapesAndAncestors(aBox, TopAbs_VERTEX, TopAbs_FACE, aMapVFs);

// The expected result should be that each vertex should has three ancestor faces, the actual result is six faces per vertex. There are three duplicated faces which should not occur in the map ancestor.
Steps To ReproduceNot needed
TagsNo tags attached.
Test case numberNot needed

Activities

git

2015-09-17 12:38

administrator   ~0045768

Branch CR26682 has been created by msv.

SHA-1: af0e069a6b6d4a66223416f10e4ca0d7ae5d27ca


Detailed log of new commits:

Author: msv
Date: Thu Sep 17 12:37:44 2015 +0300

    0026682: TopExp::MapShapesAndAncestors() will build map with duplicated ancestors

Vico Liang

2015-09-17 12:52

developer   ~0045771

Dear msv,

I have a look at the fix, it willl cause new issue. Please check it again of below, it should be IsEqual rather than IsSame. The oritation should be considered.

for (; it.More(); it.Next())
  if (anc.IsSame(it.Value()))
    break;

abv

2015-09-17 13:43

manager   ~0045781

Last edited: 2015-09-17 13:50

 The proposed fix causes multiple regressions in blend tests (I stopped testing after that): 165 in IsSame() variant, 85 in IsEqual() variant. This means that duplication of ancestors is essential for this (and likely others) algorithm.

My guess (by looking at failed cases) is that one of situations where it is important is when faces are collected for an edge in order to find free boundaries of the shape (one face means free boundary). For a seam edge on cylinder or similar object, one edge is shared twice by the same face, hence edge will be considered as free boundary if we remove duplication of face in ancestors.

Please share your thoughts on this.

msv

2015-09-17 14:29

developer   ~0045789

We can add an optional flag isUniqueAncestor to this method, with default value false.

msv

2015-09-17 14:31

developer   ~0045791

About IsSame and IsEqual, I think in different scenarios each of them can have sense.

msv

2015-09-17 14:31

developer   ~0045793

So, one more flag we can add for checking orientation.

Vico Liang

2015-09-17 14:36

developer   ~0045797

Dear abv,

I understand that there are many regressions relying on this behaviou. If changing this will cause many test update, it might be not worth to do so. It's acceptable from your description. There are no much significant benefit even if this is fixed without duplication ancestors. I agree to close this.

abv

2015-09-17 14:45

manager   ~0045799

I propose we should revise how MapShapesAndAncestors() is used in OCCT code to see whether non-duplicating variant would be useful. If it is not important in practical cases, we will keep current variant, just adding appropriate description in the method documentation.

Vico Liang

2015-09-17 15:01

developer   ~0045804

I think most of developer will need to process such duplication cases. It might be boring to do such checking every time to call this method.

msv

2015-09-17 15:14

developer   ~0045806

OK to revise the code.
For now, I know one such place in the class ShapeUpgrade_UnifySameDomain.cxx, and there the developer explicitly performed actions to exclude duplicate edges from ancestors.

msv

2016-10-25 16:13

developer   ~0059098

I think we must not overload the method by additional flags. Their processing will slow down execution. It is better to add a new method and name it MapShapesAndUniqueAncestors (to use IsEqual in discussed above place). Then it is worth to analyze OCCT code and find places where MapShapesAndAncestors is to be replaced with MapShapesAndUniqueAncestors.

git

2017-02-27 15:58

administrator   ~0064009

Branch CR26682 has been updated forcibly by mpa.

SHA-1: 1e352209baf566a9ee05f445af4a90745e058d39

mpa

2017-02-28 09:33

developer   ~0064017

Branch CR26682 is ready for review.

msv

2017-03-01 20:52

developer   ~0064065

Remarks:

dox\dev_guides\upgrade\upgrade.md
- Not needed: we report here only changes that may cause problems during porting from old versions. Modification of private members is safe.

src\TopExp\TopExp.hxx
- Logical error in description of new method: uniqueness of subshapes <TS> is maintained even in previous method as they are keys in the map. It is needed to say here about uniqueness of ancestors:
"for each one append to the list all unique ancestors of type <TA>"
- Please add the last optional argument "useOrientation" with default value false (in all cases you updated the code the orientation is ignored). It will define if the same ancestors with different orientation are considered different shapes. Describe it, too.

src\TopExp\TopExp.cxx
- implement option "useOrientation": use IsSame or IsEqual in line 131.

src\QANewBRepNaming\QANewBRepNaming_ImportShape.cxx
- 211: indexed map keeps edge order, so here it is better to use direct iteration on map 'edgeNaborFaces' instead of TopExp_Explorer.
- 215, 237-245: removal of items from indexed map is not cheap operation, so here is order to exclude treatment of the same edges twice it is better to put edges to remove in a special map with outer scope. In this case you will not need to put edges in the list 'aEdgesToRemove'.

msv

2017-03-01 20:56

developer   ~0064066

Also, please add short description of what is done in commit message (in the form good for release notes).

git

2017-03-02 17:20

administrator   ~0064089

Branch CR26682 has been updated by mpa.

SHA-1: b63e1b0d30ca3713d06df359853afff35c98beae


Detailed log of new commits:

Author: mpa
Date: Thu Mar 2 17:19:59 2017 +0300

    0026682: TopExp::MapShapesAndAncestors() will build map with duplicated ancestors.
    
    - New 'useOrientation' flag have been added to TopExp::MapShapesAndUniqueAncestors() method.
    - Avoid usage of items removal from indexed map.

msv

2017-03-03 12:11

developer   ~0064101

src\QANewBRepNaming\QANewBRepNaming_ImportShape.cxx
- 213: Improper iteration of indexed map: you use iterator of simple data map. It is needed to iterate by indices from 1 to map extent, as in the line 218.
- 218: here we can start iteration from index of edge1 plus 1, and line 220 can be removed.

Please create a new branch and put there combined one commit with proper message.

git

2017-03-03 16:07

administrator   ~0064106

Branch CR26682_1 has been created by mpa.

SHA-1: 292de1a22c3c0dad3b93e7223ea48ceb8022549c


Detailed log of new commits:

Author: mpa
Date: Fri Mar 3 13:57:36 2017 +0300

    0026682: TopExp::MapShapesAndAncestors() will build map with duplicated ancestors.
    
    - Added a new method TopExp::MapShapesAndUniqueAncestors, which excludes duplication of ancestors.
    - New 'useOrientation' flag have been added to TopExp::MapShapesAndUniqueAncestors() method.
    - Replaced MapShapesAndAncestors with MapShapesAndUniqueAncestors in all necessary places.
    - Removed deprecated code identical with new method.

git

2017-03-16 11:21

administrator   ~0064362

Branch CR26682_1 has been updated forcibly by msv.

SHA-1: fb6cd7e2cdd6900f3d163a7918239c94cfd46c07

msv

2017-03-16 11:22

developer   ~0064363

Reviewed.

git

2017-03-16 18:03

administrator   ~0064387

Branch CR26682_1 has been updated forcibly by mkv.

SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a

mkv

2017-03-17 16:34

tester   ~0064402

Dear BugMaster,
Branch CR26682_1 was rebased on IR-2017-03-16 of occt git-repository.
SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a

mkv

2017-03-17 16:34

tester   ~0064403

Dear BugMaster,
Branch CR26682_1 from occt git-repository (and IR-2016-03-16 from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a

Number of compiler warnings:

occt component :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

products component :
Linux: 64 (64 on master)
Windows: 0 (0 on master)
MacOS : 1198

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 92786725 / 92510389 [+0.30%]
Total CPU difference: 20132.52000000012 / 20187.16000000011 [-0.27%]
products component :
Total MEMORY difference: 31124322 / 31142549 [-0.06%]
Total CPU difference: 5424.3499999999785 / 5411.689999999976 [+0.23%]

Testing on Windows:
occt component :
Total MEMORY difference: 57783761 / 57782265 [+0.00%]
Total CPU difference: 18786.046022598508 / 18782.551600198512 [+0.02%]
products component :
Total MEMORY difference: 22290823 / 22254740 [+0.16%]
Total CPU difference: 5446.3381121999755 / 5421.409152399973 [+0.46%]

There are no differences in images found by testdiff.

mkv

2017-03-17 16:34

tester   ~0064404

Dear BugMaster,
Branch CR26682_1 is TESTED.

git

2017-05-12 11:35

administrator   ~0065880

Branch CR26682 has been deleted by kgv.

SHA-1: b63e1b0d30ca3713d06df359853afff35c98beae

git

2017-05-12 11:35

administrator   ~0065881

Branch CR26682_1 has been deleted by kgv.

SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a

Related Changesets

occt: master f1191d30

2017-03-03 10:57:36

mpa


Committer: mkv Details Diff		 0026682: TopExp::MapShapesAndAncestors() will build map with duplicated ancestors.

The new method TopExp::MapShapesAndUniqueAncestors has been implemented, which excludes duplication of ancestors in the list items. The optional Boolean argument 'useOrientation' of this method points whether two same shapes with different orientation will be considered equal.
OCCT code has been inspected and MapShapesAndAncestors has been replaced with MapShapesAndUniqueAncestors where it is necessary.		 Affected Issues
0026682
mod - src/BOPTest/BOPTest_TolerCommands.cxx Diff File
mod - src/BRepLib/BRepLib.cxx Diff File
mod - src/BRepLib/BRepLib_FuseEdges.cxx Diff File
mod - src/BRepLib/BRepLib_FuseEdges.hxx Diff File
mod - src/BRepOffset/BRepOffset_Analyse.cxx Diff File
mod - src/BRepOffset/BRepOffset_Inter3d.cxx Diff File
mod - src/QANewBRepNaming/QANewBRepNaming_ImportShape.cxx Diff File
mod - src/ShapeUpgrade/ShapeUpgrade_UnifySameDomain.cxx Diff File
mod - src/TopExp/TopExp.cxx Diff File
mod - src/TopExp/TopExp.hxx Diff File
mod - src/TopOpeBRepBuild/TopOpeBRepBuild_End.cxx Diff File
mod - src/TopOpeBRepTool/TopOpeBRepTool_FuseEdges.cxx Diff File
mod - src/TopOpeBRepTool/TopOpeBRepTool_FuseEdges.hxx Diff File

Issue History

Date Modified Username Field Change
2015-09-13 19:05 Vico Liang New Issue
2015-09-13 19:05 Vico Liang Assigned To => msv
2015-09-17 12:38 git Note Added: 0045768
2015-09-17 12:40 msv Status new => assigned
2015-09-17 12:41 msv Assigned To msv => abv
2015-09-17 12:41 msv Status assigned => resolved
2015-09-17 12:41 msv Steps to Reproduce Updated
2015-09-17 12:52 Vico Liang Note Added: 0045771
2015-09-17 12:52 Vico Liang Status resolved => assigned
2015-09-17 13:43 abv Note Added: 0045781
2015-09-17 13:43 abv Target Version 6.9.1 => 7.1.0
2015-09-17 13:43 abv Assigned To abv => Vico Liang
2015-09-17 13:43 abv Status assigned => feedback
2015-09-17 13:50 abv Note Edited: 0045781
2015-09-17 14:29 msv Note Added: 0045789
2015-09-17 14:31 msv Note Added: 0045791
2015-09-17 14:31 msv Note Added: 0045793
2015-09-17 14:36 Vico Liang Note Added: 0045797
2015-09-17 14:36 Vico Liang Assigned To Vico Liang =>
2015-09-17 14:37 Vico Liang Assigned To => msv
2015-09-17 14:37 Vico Liang Status feedback => acknowledged
2015-09-17 14:45 abv Note Added: 0045799
2015-09-17 14:45 abv Status acknowledged => assigned
2015-09-17 15:01 Vico Liang Note Added: 0045804
2015-09-17 15:14 msv Note Added: 0045806
2016-10-25 16:13 msv Note Added: 0059098
2016-10-25 16:14 msv Target Version 7.1.0 => 7.2.0
2016-10-25 17:58 msv Severity major => minor
2017-01-10 09:17 msv Assigned To msv => mpa
2017-02-27 15:58 git Note Added: 0064009
2017-02-27 16:16 mpa Assigned To mpa => Vico Liang
2017-02-27 16:18 mpa Assigned To Vico Liang => mpa
2017-02-28 09:33 mpa Note Added: 0064017
2017-03-01 19:26 msv Assigned To mpa => msv
2017-03-01 19:26 msv Status assigned => resolved
2017-03-01 20:52 msv Note Added: 0064065
2017-03-01 20:52 msv Assigned To msv => mpa
2017-03-01 20:52 msv Status resolved => assigned
2017-03-01 20:56 msv Note Added: 0064066
2017-03-02 17:20 git Note Added: 0064089
2017-03-02 17:20 mpa Assigned To mpa => msv
2017-03-02 17:20 mpa Status assigned => resolved
2017-03-03 12:11 msv Note Added: 0064101
2017-03-03 12:11 msv Assigned To msv => mpa
2017-03-03 12:11 msv Status resolved => assigned
2017-03-03 16:07 git Note Added: 0064106
2017-03-16 11:21 git Note Added: 0064362
2017-03-16 11:22 msv Assigned To mpa => msv
2017-03-16 11:22 msv Status assigned => resolved
2017-03-16 11:22 msv Note Added: 0064363
2017-03-16 11:22 msv Assigned To msv => bugmaster
2017-03-16 11:22 msv Status resolved => reviewed
2017-03-16 13:25 mkv Assigned To bugmaster => mkv
2017-03-16 18:03 git Note Added: 0064387
2017-03-17 16:34 mkv Note Added: 0064402
2017-03-17 16:34 mkv Note Added: 0064403
2017-03-17 16:34 mkv Note Added: 0064404
2017-03-17 16:34 mkv Assigned To mkv => bugmaster
2017-03-17 16:34 mkv Status reviewed => tested
2017-03-17 16:35 mkv Test case number => Not needed
2017-03-24 15:53 mkv Changeset attached => occt master f1191d30
2017-03-24 15:53 mkv Assigned To bugmaster => mkv
2017-03-24 15:53 mkv Status tested => verified
2017-03-24 15:53 mkv Resolution open => fixed
2017-05-12 11:35 git Note Added: 0065880
2017-05-12 11:35 git Note Added: 0065881
2017-09-29 16:20 aiv Fixed in Version => 7.2.0
2017-09-29 16:24 aiv Status verified => closed