MantisBT - Community
View Issue Details
0026682Community[OCCT] OCCT:Modeling Datapublic2015-09-13 19:052017-09-29 16:24
Vico Liang 
mkv 
normalminor 
closedfixed 
 
[OCCT] 7.2.0[OCCT] 7.2.0 
Not needed
0026682: TopExp::MapShapesAndAncestors() will build map with duplicated ancestors.
Code 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.
Not needed
No tags attached.
Issue History
2015-09-13 19:05Vico LiangNew Issue
2015-09-13 19:05Vico LiangAssigned To => msv
2015-09-17 12:38gitNote Added: 0045768
2015-09-17 12:40msvStatusnew => assigned
2015-09-17 12:41msvAssigned Tomsv => abv
2015-09-17 12:41msvStatusassigned => resolved
2015-09-17 12:41msvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=11634#r11634
2015-09-17 12:52Vico LiangNote Added: 0045771
2015-09-17 12:52Vico LiangStatusresolved => assigned
2015-09-17 13:43abvNote Added: 0045781
2015-09-17 13:43abvTarget Version6.9.1 => 7.1.0
2015-09-17 13:43abvAssigned Toabv => Vico Liang
2015-09-17 13:43abvStatusassigned => feedback
2015-09-17 13:50abvNote Edited: 0045781bug_revision_view_page.php?bugnote_id=45781#r11641
2015-09-17 14:29msvNote Added: 0045789
2015-09-17 14:31msvNote Added: 0045791
2015-09-17 14:31msvNote Added: 0045793
2015-09-17 14:36Vico LiangNote Added: 0045797
2015-09-17 14:36Vico LiangAssigned ToVico Liang =>
2015-09-17 14:37Vico LiangAssigned To => msv
2015-09-17 14:37Vico LiangStatusfeedback => acknowledged
2015-09-17 14:38abvRelationship addedhas duplicate 0005337
2015-09-17 14:45abvNote Added: 0045799
2015-09-17 14:45abvStatusacknowledged => assigned
2015-09-17 15:01Vico LiangNote Added: 0045804
2015-09-17 15:14msvNote Added: 0045806
2016-10-25 16:13msvNote Added: 0059098
2016-10-25 16:14msvTarget Version7.1.0 => 7.2.0
2016-10-25 17:58msvSeveritymajor => minor
2017-01-10 09:17msvAssigned Tomsv => mpa
2017-02-27 15:58gitNote Added: 0064009
2017-02-27 16:16mpaAssigned Tompa => Vico Liang
2017-02-27 16:18mpaAssigned ToVico Liang => mpa
2017-02-28 09:33mpaNote Added: 0064017
2017-03-01 19:26msvAssigned Tompa => msv
2017-03-01 19:26msvStatusassigned => resolved
2017-03-01 20:52msvNote Added: 0064065
2017-03-01 20:52msvAssigned Tomsv => mpa
2017-03-01 20:52msvStatusresolved => assigned
2017-03-01 20:56msvNote Added: 0064066
2017-03-02 17:20gitNote Added: 0064089
2017-03-02 17:20mpaAssigned Tompa => msv
2017-03-02 17:20mpaStatusassigned => resolved
2017-03-03 12:11msvNote Added: 0064101
2017-03-03 12:11msvAssigned Tomsv => mpa
2017-03-03 12:11msvStatusresolved => assigned
2017-03-03 16:07gitNote Added: 0064106
2017-03-16 11:21gitNote Added: 0064362
2017-03-16 11:22msvAssigned Tompa => msv
2017-03-16 11:22msvStatusassigned => resolved
2017-03-16 11:22msvNote Added: 0064363
2017-03-16 11:22msvAssigned Tomsv => bugmaster
2017-03-16 11:22msvStatusresolved => reviewed
2017-03-16 13:25mkvAssigned Tobugmaster => mkv
2017-03-16 18:03gitNote Added: 0064387
2017-03-17 16:34mkvNote Added: 0064402
2017-03-17 16:34mkvNote Added: 0064403
2017-03-17 16:34mkvNote Added: 0064404
2017-03-17 16:34mkvAssigned Tomkv => bugmaster
2017-03-17 16:34mkvStatusreviewed => tested
2017-03-17 16:35mkvTest case number => Not needed
2017-03-24 15:53mkvChangeset attached => occt master f1191d30
2017-03-24 15:53mkvAssigned Tobugmaster => mkv
2017-03-24 15:53mkvStatustested => verified
2017-03-24 15:53mkvResolutionopen => fixed
2017-05-12 11:35gitNote Added: 0065880
2017-05-12 11:35gitNote Added: 0065881
2017-09-29 16:20aivFixed in Version => 7.2.0
2017-09-29 16:24aivStatusverified => closed

Notes
(0045768)
git   
2015-09-17 12:38   
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
(0045771)
Vico Liang   
2015-09-17 12:52   
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;
(0045781)
abv   
2015-09-17 13:43   
(edited on: 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.

(0045789)
msv   
2015-09-17 14:29   
We can add an optional flag isUniqueAncestor to this method, with default value false.
(0045791)
msv   
2015-09-17 14:31   
About IsSame and IsEqual, I think in different scenarios each of them can have sense.
(0045793)
msv   
2015-09-17 14:31   
So, one more flag we can add for checking orientation.
(0045797)
Vico Liang   
2015-09-17 14:36   
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.
(0045799)
abv   
2015-09-17 14:45   
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.
(0045804)
Vico Liang   
2015-09-17 15:01   
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.
(0045806)
msv   
2015-09-17 15:14   
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.
(0059098)
msv   
2016-10-25 16:13   
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.
(0064009)
git   
2017-02-27 15:58   
Branch CR26682 has been updated forcibly by mpa.

SHA-1: 1e352209baf566a9ee05f445af4a90745e058d39
(0064017)
mpa   
2017-02-28 09:33   
Branch CR26682 is ready for review.
(0064065)
msv   
2017-03-01 20:52   
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'.
(0064066)
msv   
2017-03-01 20:56   
Also, please add short description of what is done in commit message (in the form good for release notes).
(0064089)
git   
2017-03-02 17:20   
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.

(0064101)
msv   
2017-03-03 12:11   
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.
(0064106)
git   
2017-03-03 16:07   
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.
(0064362)
git   
2017-03-16 11:21   
Branch CR26682_1 has been updated forcibly by msv.

SHA-1: fb6cd7e2cdd6900f3d163a7918239c94cfd46c07
(0064363)
msv   
2017-03-16 11:22   
Reviewed.
(0064387)
git   
2017-03-16 18:03   
Branch CR26682_1 has been updated forcibly by mkv.

SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a
(0064402)
mkv   
2017-03-17 16:34   
Dear BugMaster,
Branch CR26682_1 was rebased on IR-2017-03-16 of occt git-repository.
SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a
(0064403)
mkv   
2017-03-17 16:34   
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.
(0064404)
mkv   
2017-03-17 16:34   
Dear BugMaster,
Branch CR26682_1 is TESTED.
(0065880)
git   
2017-05-12 11:35   
Branch CR26682 has been deleted by kgv.

SHA-1: b63e1b0d30ca3713d06df359853afff35c98beae
(0065881)
git   
2017-05-12 11:35   
Branch CR26682_1 has been deleted by kgv.

SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a