Archived issue #0027870

Modeling - refactoring of HLR algorithms

Open CASCADEOCCT:Modeling Algorithmsclosed19 public notes

Description

Current implementation of HLR algorithms is ugly and undebuggable, due to extensive usage of low-level types (Standard_Address) and macros involving low-level casts to access particular data elements, instead of using typed data structures and accessing them using names. As result, no static type checking is performed, and debugger cannot be used normally to view content of "variables" (actually macros) used throughout the code.

See e.g. HLRAlgo_PolyData.cxx. For instance, method HLRAlgo_PolyData::UpdateGlobalMinMax() should take Bnd_Box or equivalent structure, or at least double[2][3] as its argument, and not Standard_Address!

Steps to reproduce

N/A

Public activity

19 archived notes

Participants are labeled by their role within this record.

01Commenter 12016-09-26 12:31

Dear Julia, please take care of this issue.

02Commenter 32016-10-07 18:31

Branch [archived branch] has been created by Participant.

[revision removed]

Detailed log of new commits:

Author: abk
Date: Wed Oct 5 13:49:44 2016 +0300

    HLRAlgo: Standard_Real[]

    HLRAlgo_EdgesBlock::MinMaxIndices

    PINod

    PISeg

    4

    3

    2

    1

03Commenter 32016-10-07 18:42

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

04Commenter 32016-10-07 19:14

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

05Commenter 52016-10-10 07:41

Some remarks on first changes:

1. In HLRAlgo_BiPoint::HLRAlgo_BiPoint(), when initializing myPoints:

> myPoints.Pnt1 = gp_XYZ(X1, Y1, Z1);

can be better replaced by

> myPoints.Pnt1.SetCoord (X1, Y1, Z1);

2. In declaration of HLRAlgo::UpdateMinMax(), it is better to specify dimension of arrays: Standard_Real Min[16]. Casts of Min to Standard_Real* are not needed.

3. In HLRAlgo_BiPoint.hxx: (a) when defining a structure, it is better to define each field on its own line, (b) when defining single-line inline method, keep "{" and "}" on separate lines (unless whole method fits single line).

4. HLRAlgo_PolyData.hxx: why enum HLR_PolyMask is defined here? It is not used in this header at all! Please define it where it is used, as locally as possible.

06Commenter 32016-10-17 21:18

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

07Commenter 72016-10-25 12:04

Dear Julia, please review the fix.

08Commenter 82016-10-27 12:48

My remarks:

1. HLRAlgo_PolyInternalData.cxx
line 323: why
Nod3RValues.Normal *= 1 / aNorm;
but not
Nod3RValues.Normal = aXYZ / aNorm; ?

2. HLRAlgo_PolyInternalData.cxx lines 698, 739;
HLRBRep_PolyAlgo.cxx lines 2471, 2942, 3125, 3135 :
there is a risk of usage of null pointer, it is better to avoid it like it is done in fix to OCC25214.

09Commenter 92016-10-27 15:22

I propose to rebase the branch on CR25214, as it has already been tested.

10Commenter 32016-10-28 16:46

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

11Commenter 112016-10-28 16:52

The remarks were considered, the branch was rebased to the master.
Please review ASAP.

12Commenter 122016-10-28 16:57

Please test.

13Commenter 32016-10-28 18:41

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

14Commenter 142016-10-28 18:42

Branch [archived branch] has been rebased on the current master

15Commenter 152016-11-01 15:03

Dear Commenter 2,

Branch CR27870 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
[revision removed]

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 0 (0 on master)
products component:
   Linux: 63
   Windows: 0
   MacOS: 1149

Regressions/Differences:
Not detected

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 91024361 / 90749168 [+0.30%]
Total CPU difference: 19467.219999999903 / 19313.639999999898 [+0.80%]

Testing on Windows:
Total MEMORY difference: 57306156 / 57321116 [-0.03%]
Total CPU difference: 17749.575378598707 / 18220.18359529866 [-2.58%]

16Commenter 12016-11-02 18:32

There are compilation errors in debug mode (build with preprocessor definition OCCT_DEBUG):

Building CXX object src/TKHLR/CMakeFiles/TKHLR.dir/__/HLRBRep/HLRBRep_ShapeBounds.cxx.o
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:906:12: error: use of undeclared identifier 'Nod11NrmX'
              if (Nod11NrmX*Nod12NrmX +
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:906:22: error: use of undeclared identifier 'Nod12NrmX'
              if (Nod11NrmX*Nod12NrmX +
                            ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:907:5: error: use of undeclared identifier 'Nod11NrmY'
                  Nod11NrmY*Nod12NrmY +
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:907:15: error: use of undeclared identifier 'Nod12NrmY'
                  Nod11NrmY*Nod12NrmY +
                            ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:908:15: error: use of undeclared identifier 'Nod12NrmZ'
                  Nod11NrmZ*Nod12NrmZ < 0) {
                            ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:908:5: error: use of undeclared identifier 'Nod11NrmZ'
                  Nod11NrmZ*Nod12NrmZ < 0) {
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1093:12: error: use of undeclared identifier 'Nod21NrmX'
              if (Nod21NrmX*Nod22NrmX +
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1093:22: error: use of undeclared identifier 'Nod22NrmX'
              if (Nod21NrmX*Nod22NrmX +
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1094:5: error: use of undeclared identifier 'Nod21NrmY'
                  Nod21NrmY*Nod22NrmY +
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1094:15: error: use of undeclared identifier 'Nod22NrmY'
                  Nod21NrmY*Nod22NrmY +
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1095:15: error: use of undeclared identifier 'Nod22NrmZ'
                  Nod21NrmZ*Nod22NrmZ < 0) {
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1095:5: error: use of undeclared identifier 'Nod21NrmZ'
                  Nod21NrmZ*Nod22NrmZ < 0) {
                  ^
[user path removed]/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:2313:11: error: use of undeclared identifier 'Tri1Node1'
                cout << Tri1Node1 << " modifies : DX,DY ";

17Commenter 32016-11-03 13:09

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

18Commenter 182016-11-03 13:16

The cause of the compilation errors in the debug mode was fixed.
Please test again.

19Commenter 32016-12-07 11:30

Branch [archived branch] has been deleted by Commenter 4.

[revision removed]

Related records

#0032292 · parent of · closedCoding Rules - improve HLRBRep_PolyAlgo readability part 1