MantisBT - Open CASCADE
View Issue Details
0028259Open CASCADE[OCCT] OCCT:Modeling Algorithmspublic2016-12-22 10:352017-09-29 16:25
msv 
bugmaster 
normaltrivial 
closedfixed 
[OCCT] 7.1.0 
[OCCT] 7.2.0[OCCT] 7.2.0 
Not needed
0028259: Method MakeBlocksCnx is duplicated in two different places in BOPAlgo
There are two methods with the name MakeBlocksCnx in the package BOPAlgo:

- public static method of the class BOPAlgo_Tools;
- local static method in the file BOPAlgo_Builder_2.cxx

Implementation of these methods looks like copy-paste of each other, with small modification regarding the type of items in the input containers. In one method they are shapes, in the other they are indices of shapes.

The same is for the methods FillMap with the following parameters:

  void FillMap(const TopoDS_Shape& aS1,
               const TopoDS_Shape& aS2,
               BOPCol_IndexedDataMapOfShapeListOfShape& aDMSLS,
               Handle(NCollection_BaseAllocator)& aAllocator);
  void FillMap(const Standard_Integer tneN1,
               const Standard_Integer tneN2,
               BOPCol_IndexedDataMapOfIntegerListOfInteger& theMILI,
               const BOPCol_BaseAllocator& theAllocator);

They are implemented also in these two files.

It is needed to remove local static methods and to use methods of the class BOPAlgo_Tools. Some modification of calling code will be necessary to switch to new structures.
Not needed.
No tags attached.
Issue History
2016-12-22 10:35msvNew Issue
2016-12-22 10:35msvAssigned To => msv
2016-12-22 10:36msvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=15661#r15661
2017-05-03 08:49emvNote Added: 0065696
2017-05-23 15:15emvAssigned Tomsv => emv
2017-05-23 15:15emvStatusnew => assigned
2017-05-25 13:35gitNote Added: 0066580
2017-05-25 13:37emvNote Added: 0066581
2017-05-25 13:37emvAssigned Toemv => msv
2017-05-25 13:37emvStatusassigned => resolved
2017-05-25 18:08gitNote Added: 0066608
2017-05-25 18:12gitNote Added: 0066610
2017-05-25 18:16emvNote Added: 0066612
2017-05-26 09:18msvNote Added: 0066620
2017-05-26 09:18msvAssigned Tomsv => emv
2017-05-26 09:18msvStatusresolved => assigned
2017-05-26 10:36gitNote Added: 0066621
2017-05-26 10:42gitNote Added: 0066622
2017-05-26 10:43emvNote Added: 0066623
2017-05-26 10:43emvAssigned Toemv => msv
2017-05-26 10:43emvStatusassigned => resolved
2017-05-26 11:11gitNote Added: 0066627
2017-05-26 11:18gitNote Added: 0066630
2017-05-26 11:20msvNote Added: 0066631
2017-05-26 11:20msvAssigned Tomsv => bugmaster
2017-05-26 11:20msvStatusresolved => reviewed
2017-05-26 12:40mkvAssigned Tobugmaster => mkv
2017-05-26 15:26gitNote Added: 0066647
2017-05-26 16:35mkvNote Added: 0066659
2017-05-26 16:35mkvAssigned Tomkv => emv
2017-05-26 16:35mkvStatusreviewed => assigned
2017-05-26 17:02gitNote Added: 0066662
2017-05-26 17:03emvNote Added: 0066663
2017-05-26 17:03emvAssigned Toemv => msv
2017-05-26 17:03emvStatusassigned => resolved
2017-05-26 17:38msvNote Added: 0066671
2017-05-26 17:38msvAssigned Tomsv => bugmaster
2017-05-26 17:38msvStatusresolved => reviewed
2017-05-26 17:45mkvAssigned Tobugmaster => mkv
2017-05-29 17:59mkvNote Added: 0066824
2017-05-29 18:00mkvNote Added: 0066825
2017-05-29 18:00mkvAssigned Tomkv => emv
2017-05-29 18:00mkvStatusreviewed => feedback
2017-05-29 18:00mkvTest case number => Not needed
2017-05-30 07:34emvNote Added: 0066842
2017-05-30 07:34emvAssigned Toemv => mkv
2017-05-30 07:35emvNote Edited: 0066842bug_revision_view_page.php?bugnote_id=66842#r16827
2017-05-30 16:34mkvNote Added: 0066890
2017-05-30 16:34mkvAssigned Tomkv => bugmaster
2017-05-30 16:34mkvStatusfeedback => tested
2017-06-02 10:28bugmasterChangeset attached => occt master edfa30de
2017-06-02 10:28bugmasterStatustested => verified
2017-06-02 10:28bugmasterResolutionopen => fixed
2017-06-02 10:45gitNote Added: 0066998
2017-06-02 10:46gitNote Added: 0066999
2017-09-29 16:18aivFixed in Version => 7.2.0
2017-09-29 16:25aivStatusverified => closed

Notes
(0065696)
emv   
2017-05-03 08:49   
The similar methods:
void BOPAlgo_Tools::FillMap(const Handle(BOPDS_PaveBlock)& aPB1,
                            const Handle(BOPDS_PaveBlock)& aPB2,
                            BOPDS_IndexedDataMapOfPaveBlockListOfPaveBlock& aMPBLPB,
                            const Handle(NCollection_BaseAllocator)& aAllocator)

void BOPAlgo_Tools::MakeBlocks(const BOPDS_IndexedDataMapOfPaveBlockListOfPaveBlock& aMILI,
                               BOPDS_DataMapOfIntegerListOfPaveBlock& aMBlocks,
                               const Handle(NCollection_BaseAllocator)& aAllocator)

It is necessary to unify implementation of all these methods using two template functions - one for FillMap and the other for MakeBlocks.
(0066580)
git   
2017-05-25 13:35   
Branch CR28259 has been created by emv.

SHA-1: 5202aa3f373858c2c38eab8d0b7a8ba9a2c7b74a


Detailed log of new commits:

Author: emv
Date: Sun Apr 30 01:09:36 2017 +0300

    0028259: Method MakeBlocksCnx is duplicated in two different places in BOPAlgo
    
    The implementation of the methods BOPAlgo_Tools::MakeBlocksCnx(), BOPAlgo_Tools::MakeBlocks() and static method MakeBlocksCnx in BOPAlgo_Builder_2.cxx have been replaced with the usage of the new template method MakeBlocksT().
    The implementation of all methods BOPAlgo_Tools::FillMap() have been replaced with the usage of the new template method FillMapT().
    
    Making the Pave Block with the smallest index of original edge to be the first in the Common Block (i.e. the representing Pave Block).
    
    The following improvements have been made in Boolean Operations algorithm to avoid regressions:
    - When updating the existing common block update its pave blocks in a way that the parameters of the paves should be valid for the original edge (bugs/modalg_5/bug24809);
    - When trying to reduce the tolerance of the section edge check the tolerance of all Face/Face interferences created this edge (boolean/volumemaker/C4,D2);
    - Avoid producing the different Pave Blocks for the same section edge (boolean/volumemaker/D6);
    
    Adjustment of the test cases.
(0066581)
emv   
2017-05-25 13:37   
Dear Mikhail, could you please review the branch CR28259?
(0066608)
git   
2017-05-25 18:08   
Branch CR28259 has been updated by emv.

SHA-1: a19f720a0bb48efb88873d69465ed09b39b1fec8


Detailed log of new commits:

Author: emv
Date: Thu May 25 18:07:37 2017 +0300

    Keep only template functions BOPAlgo_Tools::MakeBlocks() and BOPAlgo_Tools::FillMap().

(0066610)
git   
2017-05-25 18:12   
Branch CR28259_1 has been created by emv.

SHA-1: b183b2c053823cd04c70e1a7295ccd5c0cb16f50


Detailed log of new commits:

Author: emv
Date: Sun Apr 30 01:09:36 2017 +0300

    0028259: Method MakeBlocksCnx is duplicated in two different places in BOPAlgo
    
    The methods BOPAlgo_Tools::MakeBlocksCnx(), BOPAlgo_Tools::MakeBlocks() and static method MakeBlocksCnx in BOPAlgo_Builder_2.cxx have been replaced with the new template method BOPAlgo_Tools::MakeBlocks().
    All methods BOPAlgo_Tools::FillMap() have been replaced with the new template method BOPAlgo_Tools::FillMap().
    
    Making the Pave Block with the smallest index of original edge to be the first in the Common Block (i.e. the representing Pave Block).
    
    The following improvements have been made in Boolean Operations algorithm to avoid regressions:
    - When updating the existing common block update its pave blocks in a way that the parameters of the paves should be valid for the original edge (bugs/modalg_5/bug24809);
    - When trying to reduce the tolerance of the section edge check the tolerance of all Face/Face interferences created this edge (boolean/volumemaker/C4,D2);
    - Avoid producing the different Pave Blocks for the same section edge (boolean/volumemaker/D6);
    
    Adjustment of the test cases.
(0066612)
emv   
2017-05-25 18:16   
The corrections have been made according to verbal remarks. These corrections are localized in last commit to CR28259 branch. The branch CR28259_1 contains all changes and the correct commit message. Please review again.
(0066620)
msv   
2017-05-26 09:18   
In commit massage:
"check the tolerance of all Face/Face interferences created this edge"
=>
"check the tolerance of all Face/Face interferences that created this edge"

src\BOPAlgo\BOPAlgo_Tools.hxx
- useless include of BOPDS_DataMapOfIntegerListOfPaveBlock
- it seems the argument theMBlocks of the method MakeBlocks has too complex type. According to the context where it is used it is enough to have list of list.
(0066621)
git   
2017-05-26 10:36   
Branch CR28259 has been updated by emv.

SHA-1: dff7c8e329c5fffba25d6a28fa2450b58bbac75f


Detailed log of new commits:

Author: emv
Date: Fri May 26 10:33:49 2017 +0300

    In the method BOPAlgo_Tools::MakeBlocks() store the connected elements into the list of list instead of data map.

(0066622)
git   
2017-05-26 10:42   
Branch CR28259_1 has been updated forcibly by emv.

SHA-1: dddfd809101aa16c18062e872854491d156d106c
(0066623)
emv   
2017-05-26 10:43   
Done, please review.
(0066627)
git   
2017-05-26 11:11   
Branch CR28259_1 has been updated forcibly by emv.

SHA-1: 14035d1d673e84f52d4afa471f4f8dad8dac9a97
(0066630)
git   
2017-05-26 11:18   
Branch CR28259_1 has been updated forcibly by emv.

SHA-1: 648fab3b6b396e55a2f4b402d024685cb39f6142
(0066631)
msv   
2017-05-26 11:20   
Reviewed.
(0066647)
git   
2017-05-26 15:26   
Branch CR28259_1 has been updated forcibly by mkv.

SHA-1: 899020d3ba52d1973f5103dbee2b12ace9eb425b
(0066659)
mkv   
2017-05-26 16:35   
Dear BugMaster,
Branch CR28259_1 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms on Release mode.
SHA-1: 899020d3ba52d1973f5103dbee2b12ace9eb425b

There are compilation errors:

http://jenkins-test-07.nnov.opencascade.com:8080/view/CR28259_1-master/job/CR28259_1-master-OCCT-Debian70-64-opt-compile/1/parsed_console/ [^]

/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:7: error: need 'typename' before 'NCollection_List<theType>::Iterator' because 'NCollection_List<theType>' is a dependent scope
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:43: error: expected ';' before 'aItLChain'
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:55:14: error: 'aItLChain' was not declared in this scope
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:59:14: error: need 'typename' before 'NCollection_List<theType>::Iterator' because 'NCollection_List<theType>' is a dependent scope
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:59:50: error: expected ';' before 'aItLI'
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:59:62: error: 'aItLI' was not declared in this scope

/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:43:65: error: no matching function for call to 'NCollection_Map<TopoDS_Shape, TopTools_ShapeMapHasher>::NCollection_Map(const BOPCol_BaseAllocator&)'

/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:7: error: dependent-name 'NCollection_List<theType>::Iterator' is parsed as a non-type, but instantiation yields a type
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:7: note: say 'typename NCollection_List<theType>::Iterator' if a type is meant
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:59:60: error: dependent-name 'NCollection_List<theType>::Iterator' is parsed as a non-type, but instantiation yields a type
/dn54/builds/CR28259_1-master/Debian70-64-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:59:60: note: say 'typename NCollection_List<theType>::Iterator' if a type is meant

http://jenkins-test-07.nnov.opencascade.com:8080/view/CR28259_1-master/job/CR28259_1-master-OCCT-MacOS-opt-compile/1/parsed_console/ [^]

/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:23: error: expected ';' after expression
      NCollection_List<theType>::Iterator aItLChain(aChain);
                      ^
                      ;
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:55:14: error: use of undeclared identifier 'aItLChain'; did you mean 'aChain'?
      for (; aItLChain.More(); aItLChain.Next()) {
             ^~~~~~~~~
             aChain
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:51:34: note: 'aChain' declared here
      NCollection_List<theType>& aChain = theMBlocks.Append(NCollection_List<theType>(theAllocator));
                                 ^
[ 39%] /Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:55:32: error: use of undeclared identifier 'aItLChain'; did you mean 'aChain'?
      for (; aItLChain.More(); aItLChain.Next()) {
                               ^~~~~~~~~
                               aChain
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:51:34: note: 'aChain' declared here
      NCollection_List<theType>& aChain = theMBlocks.Append(NCollection_List<theType>(theAllocator));
                                 ^
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:56:29: error: use of undeclared identifier 'aItLChain'; did you mean 'aChain'?
        const theType& n1 = aItLChain.Value();
                            ^~~~~~~~~
                            aChain
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:51:34: note: 'aChain' declared here
      NCollection_List<theType>& aChain = theMBlocks.Append(NCollection_List<theType>(theAllocator));
                                 ^
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:59:14: error: missing 'typename' prior to dependent type name 'NCollection_List<theType>::Iterator'
        for (NCollection_List<theType>::Iterator aItLI(aLI); aItLI.More(); aItLI.Next()) {
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             typename
Building CXX object src/TKDraw/CMakeFiles/TKDraw.dir/__/DBRep/DBRep_HideData.cxx.o
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:43:45: error: no matching constructor for initialization of 'NCollection_Map<TopoDS_Shape, TopTools_ShapeMapHasher>'
    NCollection_Map<theType, theTypeHasher> aMFence(theAllocator);
                                            ^ ~~~~~~~~~~~~

/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:7: error: missing 'typename' prior to dependent type name 'NCollection_List<TopoDS_Shape>::Iterator'
      NCollection_List<theType>::Iterator aItLChain(aChain);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:54:43: error: use of undeclared identifier 'aItLChain'
      NCollection_List<theType>::Iterator aItLChain(aChain);
                                          ^
/Users/mnt/builds/CR28259_1-master/MacOS-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx:55:24: error: no member named 'More' in 'NCollection_List<TopoDS_Shape>'
      for (; aItLChain.More(); aItLChain.Next()) {
             ~~~~~~~~~ ^
9 errors generated.

http://jenkins-test-07.nnov.opencascade.com:8080/view/CR28259_1-master/job/CR28259_1-master-OCCT-Windows-64-VC10-opt-compile/1/parsed_console/ [^]

23>D:/builds/CR28259_1-master/Windows-64-VC10-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx(67): error C4519: default template arguments are only allowed on a class template
23>D:/builds/CR28259_1-master/Windows-64-VC10-opt/OCCT/src/BOPAlgo/BOPAlgo_Tools.hxx(87): error C4519: default template arguments are only allowed on a class template

(0066662)
git   
2017-05-26 17:02   
Branch CR28259_1 has been updated by emv.

SHA-1: 38e226d91f54d4140574e1e693e089c4b456e9e5


Detailed log of new commits:

Author: emv
Date: Fri May 26 16:58:16 2017 +0300

    // Corrections of the compilation errors.

(0066663)
emv   
2017-05-26 17:03   
The compilation errors should be eliminated now. Please review.
(0066671)
msv   
2017-05-26 17:38   
Reviewed.
(0066824)
mkv   
2017-05-29 17:59   
Dear BugMaster,
Branch CR28259_1 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
SHA-1: 38e226d91f54d4140574e1e693e089c4b456e9e5

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 : 1208

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 91929501 / 91797911 [+0.14%]
Total CPU difference: 18906.610000000233 / 19346.9700000001 [-2.28%]
products component :
Total MEMORY difference: 31270768 / 31352485 [-0.26%]
Total CPU difference: 5425.829999999985 / 5393.529999999966 [+0.60%]
Testing on Windows:
occt component :
Total MEMORY difference: 58567549 / 58497831 [+0.12%]
Total CPU difference: 17342.178767098525 / 17520.28510879866 [-1.02%]
products component :
Total MEMORY difference: 22726695 / 22688363 [+0.17%]
Total CPU difference: 5360.927564699972 / 5357.885545199975 [+0.06%]

There are following differences in images found by testdiff.
http://occt-tests/CR28259_1-master-OCCT/Debian70-64/diff-Debian70-64.html [^]
http://occt-tests/CR28259_1-master-OCCT/Windows-64-VC10/diff-Windows-64-VC10-image.html [^]
(0066825)
mkv   
2017-05-29 18:00   
Dear emv,
Branch CR28259_1 has been rejected due to:
- differences in images
(0066842)
emv   
2017-05-30 07:34   
(edited on: 2017-05-30 07:35)
The differences in images are cased by changing the way of choosing the representing edge in the block of coinciding edges in Boolean Operations algorithm. As I see, all differences are minor, I did not find any difference which looks like a regression.
Dear Mikhail, can you point out the particular cases which look suspicious to you?

(0066890)
mkv   
2017-05-30 16:34   
Dear BugMaster,
Branch CR28259_1 is TESTED.
(0066998)
git   
2017-06-02 10:45   
Branch CR28259_1 has been deleted by inv.

SHA-1: 38e226d91f54d4140574e1e693e089c4b456e9e5
(0066999)
git   
2017-06-02 10:46   
Branch CR28259 has been deleted by inv.

SHA-1: dff7c8e329c5fffba25d6a28fa2450b58bbac75f