MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0026682Community[OCCT] OCCT:Modeling Datapublic2015-09-13 19:052017-09-29 16:24
ReporterVico Liang 
Assigned Tomkv 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.2.0Fixed in Version[OCCT] 7.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
Attached Files

- Relationships

-  Notes
(0045768)
git (administrator)
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 (developer)
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 (manager)
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 (developer)
2015-09-17 14:29

We can add an optional flag isUniqueAncestor to this method, with default value false.
(0045791)
msv (developer)
2015-09-17 14:31

About IsSame and IsEqual, I think in different scenarios each of them can have sense.
(0045793)
msv (developer)
2015-09-17 14:31

So, one more flag we can add for checking orientation.
(0045797)
Vico Liang (developer)
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 (manager)
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 (developer)
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 (developer)
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 (developer)
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 (administrator)
2017-02-27 15:58

Branch CR26682 has been updated forcibly by mpa.

SHA-1: 1e352209baf566a9ee05f445af4a90745e058d39
(0064017)
mpa (developer)
2017-02-28 09:33

Branch CR26682 is ready for review.
(0064065)
msv (developer)
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 (developer)
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 (administrator)
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 (developer)
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 (administrator)
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 (administrator)
2017-03-16 11:21

Branch CR26682_1 has been updated forcibly by msv.

SHA-1: fb6cd7e2cdd6900f3d163a7918239c94cfd46c07
(0064363)
msv (developer)
2017-03-16 11:22

Reviewed.
(0064387)
git (administrator)
2017-03-16 18:03

Branch CR26682_1 has been updated forcibly by mkv.

SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a
(0064402)
mkv (tester)
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 (tester)
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 (tester)
2017-03-17 16:34

Dear BugMaster,
Branch CR26682_1 is TESTED.
(0065880)
git (administrator)
2017-05-12 11:35

Branch CR26682 has been deleted by kgv.

SHA-1: b63e1b0d30ca3713d06df359853afff35c98beae
(0065881)
git (administrator)
2017-05-12 11:35

Branch CR26682_1 has been deleted by kgv.

SHA-1: f1191d309911ff1f023c668f2d53f988a2f7d87a

- Related Changesets
occt: master f1191d30
Timestamp: 2017-03-03 10:57:36
Author: 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.
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 View Revisions
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 View Revisions
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:38 abv Relationship added has duplicate 0005337
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 user533 Fixed in Version => 7.2.0
2017-09-29 16:24 user533 Status verified => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker