DiscussionsIssue archiveOCCT:Modeling Data

Archived issue #0031336

Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation

Open CASCADEOCCT:Modeling Dataclosed53 public notes

Search issues

Description

It's proposed to allow creating box base on two gp_Pnt that will create thin box or point.

Public activity

53 archived notes

Participants are labeled by their role within this record.

01Commenter 1
This will be invalid shape. What for will we allow our user level building tools creating invalid shapes?
02Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Fri Jan 31 16:33:02 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
03Commenter 3
Branch [archived branch] has been updated by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Tue Feb 4 12:23:08 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation

04Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Tue Feb 4 12:41:14 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
05Commenter 2
Dear Mikhail

could you please review CR31336_1.

Thank you a lot, Svetlana
06Commenter 6
Why on the picture creation of the box b outputs the error?
It seems the box 15 0 0 20 5 5 should be valid.
07Commenter 7
Please make more detailed explanation of the new feature in the commit message. The bug subject does not explain creation of the new package.
08Commenter 4
+private:
+
+  //! Create a vertex if thin box in all directions is a point.
+  void MakeVertex();

Private and protected methods are expected to start lower-cased.

+  BRepPreviewAPI_MakeBox (const gp_Pnt& theFirstPoint, const gp_Pnt& theSecondPoint) :
+    BRepPrimAPI_MakeBox(),

BRepPrimAPI_MakeBox() is redundant here.

+
+class BRepPreviewAPI_MakeBox : public BRepPrimAPI_MakeBox
+{

Class description is missing.

+  gp_Pnt myFirstPoint;  //!< first point.
+  gp_Pnt mySecondPoint; //!< second point.

dot is redundant at the end of comment

+};
+#endif

Please put empty line after class definition.

+  //! Default constructor
+  Standard_EXPORT BRepPrim_GWedge() { std::cout << "Not valid box" << std::endl; } //TODO: make this case
...
+  //! Default constructor
+  Standard_EXPORT BRepPrim_Wedge() {}
...
+  //! Default constructor
+  Standard_EXPORT BRepPrimAPI_MakeBox() {}

Unexpected TODO and unexpected Standard_EXPORT for inline methods.

+//=======================================================================
+//function : Init
+//purpose  : .

Please avoid trailing spaces.

+void BRepPrimAPI_MakeBox::Init (const gp_Pnt& P, 
+                                const Standard_Real dx,
+                                const Standard_Real dy, 
+                                const Standard_Real dz)

Please follow OCCT Coding Rules recommendations in new code (thePnt, theDX, ...).

+  if (n > 8 && n < 11)
+  {
+    TopoDS_Shape S;
+    Standard_Real x1 = 0.0, y1 = 0.0, z1 = 0.0, x2 = 0.0, y2 = 0.0, z2 = 0.0;
+    for

Please implement general for-loop instead of two different loops.

+      anArgCase.LowerCase();
+        if (anArgCase == "-pnt1")

Broken indentation.

-//function : PrimitiveCommands
+//function : PrimitiveCommands                                                           C:\shutina\occt\CR29451_7\src\BRepTest\BRepTest_PrimitiveCommands.cxx

Artifact.

-  theCommands.Add("box","box name [x1 y1 z1] dx dy dz",__FILE__,box,g);
+  theCommands.Add("box","box name [x1 y1 z1] dx dy dz -pnt1 x y z -pnt2 x y z ",__FILE__,box,g);

Command description is missing.
You may take "bounding" command description as a sample:
  theCommands.Add ("bounding",
                   "bounding {shape | xmin ymin zmin xmax ymax zmax}"
         "\n\t\t:            [-obb] [-noTriangulation] [-optimal] [-extToler]"
         "\n\t\t:            [-dump] [-print] [-dumpJson] [-shape name] [-nodraw] [-finitePart]"
         "\n\t\t:            [-save xmin ymin zmin xmax ymax zmax]"
         "\n\t\t:"
         "\n\t\t: Computes a bounding box. Two types of the source data are supported:"
         "\n\t\t: a shape or AABB corners (xmin, ymin, zmin, xmax, ymax, zmax)."
         "\n\t\t:"
         "\n\t\t: Calculation options (applicable only if input is a shape):"
         "\n\t\t:  -obb     Compute Oriented Bounding Box (OBB) instead of AABB."


09Commenter 9
src/BRepPrim/BRepPrim_GWedge.hxx
- Please do not use std::cout in the code. OCCT must not put to cout without any conditions. If it is needed for debug then surround printout by #ifdef _DEBUG statement.
- All field members must be initialized in the constructor (make it in .cxx file).

src/BRepPrim/BRepPrim_Wedge.hxx
- Do not use Standard_EXPORT for inline method.

src/BRepPrimAPI/BRepPrimAPI_MakeBox.hxx
- Do not use Standard_EXPORT for inline method.
- Please do not copy the (incorrect) description of the last version of the constructor. I propose to correct description to match it with the method purpose. This description is more appropriate to be description of the class itself (move it there please).

src/BRepPreviewAPI/BRepPreviewAPI_MakeBox.hxx
- Please add the class description.
- The comment to the method Build() is incorrect
- line 47: misprint "compuund", and sentence is weird. The better:
Create a rectangle (compound of edges) if the box is thin in one direction only.

src/BRepPreviewAPI/BRepPreviewAPI_MakeBox.cxx
- lines 36,42: compute this expression once only.
10Commenter 10
Also, please create several test cases to cover all the new code.
11Commenter 4
                   "box name [dx dy dz] [x y z dx dy dz]"
         "\n\t\t:            [-min x y z] [-size dx dy dz] [-max x y z]"
         "\n\t\t:            [-dir x y z -xdir x y z] [-solid]"
         "\n\t\t: Construct axes-aligned box and put result into 'name' variable"
         "\n\t\t:  -min   box lower corner, origin; (0,0,0) by default"
         "\n\t\t:  -size  box dimensions   (alternative to -max)"
         "\n\t\t:  -max   box upper corner (alternative to -size)"
         "\n\t\t:  -dir   main direction of coordinate system (DZ by default)"
         "\n\t\t:  -xdir  x    direction of coordinate system (DX by default)"
         "\n\t\t:  -solid return NULL shape in case of zero box dimension;"
         "\n\t\t:         otherwise, non-solid shape will be created (vertex, edge, rectangle or box)."
12Author
Dear Kirill,

are there new desired parameters for the 'box' command?

13Commenter 4
> are there new desired parameters for the 'box' command?
BRepPrimAPI_MakeBox has a constructor taking gp_Ax2, so that it makes sense supporting this argument in Draw command as well.
14Commenter 3
Branch [archived branch] has been updated by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Wed Feb 5 17:39:01 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    #fix remarks

15Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Wed Feb 5 18:01:28 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRrepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the execution does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRrepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/preview (vertex, edge, rectangle, box)
16Commenter 2
Dear Mikhail

could you please review CR31336_2.

Thank you a lot, Svetlana
17Commenter 17
Remarks to commit message:
- misprint 'BRrepPreviewAPI'
- >BRepPrim_GWedge: in the case of non-valid data, the execution does not happen ...
  execution => exception
18Commenter 18
src/TKPrim/PACKAGES
- extra spaces after BRepPrimAPI
19Commenter 19
Move tests/preview to tests/tools/preview, and make sure they are running using 'testgrid tools preview' command.
20Commenter 20
src/BRepPrim/BRepPrim_GWedge.cxx
- lines 138-149: remove initialization of objects that have default constructor (myBuilder and myAxes).
- Not all fields are initialized in default constructor.

src/BRepPrimAPI/BRepPrimAPI_MakeBox.hxx
- 76: Init => Make

src/BRepPrimAPI/BRepPrimAPI_MakeBox.cxx
- 124: extra indent

src/BRepPreviewAPI/BRepPreviewAPI_MakeBox.hxx
- 26: reformulate:
//! 3 - preview can be a rectangle (compound of edges) if thin box in only one direction is a point;
- Description of default constructor must not contain params.

BRepPreviewAPI_MakeBox inherits BRepPrimAPI_MakeBox. The latter has methods Init(). Why do you define the same methods in BRepPreviewAPI_MakeBox?

src/BRepPreviewAPI/BRepPreviewAPI_MakeBox.cxx
- 159: checking condition is useless in the last case.
- 161-162: wht do you construct a new MakeBox object? This object's parent instance has been already initialized. Use it.
21Commenter 21
Did you tried to run your tests? They don't work.

- vclear must not be run before vinit.
- vtop must not be run if no view exists.
- all snapshots are empty, because you did not display any shape.

Please don't use v* commands. Use 'smallview +X+Y', donly, fit, xwd.
22Commenter 4
> - vclear must not be run before vinit.
Well, this one is awkward, but it is a common practice - the command doesn't raise error in case of no viewer,
and calling it before 'vinit' avoids issue in some rare scenarios (within interactive execution of multiple test cases in scope of the same session).

23Commenter 23
src/BRepTest/BRepTest_PrimitiveCommands.cxx
- In case of '-solid' you set isPreview to true. It is incorrect. To preserve the old command behavior, it is better to rename this option to '-preview'.
24Commenter 24
>Well, this one is awkward, but it is a common practice - the command doesn't raise error in case of no viewer,
I agree, but in this test we do not need shaded view at all.
25Commenter 3
Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 05:49:28 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    # compilation correction

26Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Thu Feb 6 12:50:19 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/tools/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
27Commenter 3
Branch [archived branch] has been updated by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Thu Feb 6 13:00:27 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    #correct a name of parameter

28Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Thu Feb 6 13:03:11 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/tools/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
29Commenter 2
Dear Mikhail

could you please review CR31336_4.

Jenkins job: http://jenkins-test-12.nnov.opencascade.com/view/CR31336-master-Commenter 2/

Thank you a lot, Svetlana
30Commenter 30
Well, the code is OK now.
But there is the problem with the tests. They are missing in the Jenkins test report. The category 'tools' is not included in the testing because of dependence on QT.
So, I propose to move the tests in other category. The best is 'geometry'.
Svetlana, please do it and retest.
31Commenter 3
Branch [archived branch] has been updated by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Thu Feb 6 18:05:29 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    #tests from tools to geometry

32Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Thu Feb 6 18:07:33 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/geometry/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
33Author
Dear Mikhail,

Svetlana moved tests, please check.

Jenkins job: http://jenkins-test-12.nnov.opencascade.com/view/CR31336-master-Commenter 2/ [^]

[signature removed]Natalia
34Commenter 34
Why did not you create a rectangular face in case of degeneration in one direction?
I think it would be more logical to degenerate from solid to surface instead of a wire model.
35Author
Dear Svetlana,

could you please follow Mikhail's recommendation:

Correct fix by using BRepBuilderAPI_MakeEdge, BRepBuilderAPI_MakeWire and finaly BRepBuilderAPI_MakeFace.

Thank you, Natalia
36Commenter 36
Indeed, there are two possibilities:

1. Do what Natalia proposed, and finally use the following constructor of MakeFace:
BRepBuilderAPI_MakeFace(const TopoDS_Wire& W, const Standard_Boolean OnlyPlane = Standard_False);

2. Skip edges and wire creation, and call only MakeFace:
BRepBuilderAPI_MakeFace(const gp_Pln& P, const Standard_Real UMin, const Standard_Real UMax, const Standard_Real VMin, const Standard_Real VMax);
In this case you need only to build the correct plane object.

Choose the simplest one.
37Commenter 3
Branch [archived branch] has been updated by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Mon Feb 10 10:50:22 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/geometry/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
    
    #rectangle creation fix

38Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Mon Feb 10 11:02:43 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/geometry/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
39Commenter 2
Dear Mikhail

could you please review CR31336_6.

Thank you a lot, Svetlana
40Commenter 40
You should update all comments where rectangle is described as a compound of edges. Now it is a rectangular face.
41Commenter 4
+  #ifdef _DEBUG
+    std::cout << "Not valid box: vertex" << std::endl;
+  #endif
...
+  #ifdef _DEBUG
+    std::cout << "Not valid box: edge" << std::endl;
+  #endif
...
+  #ifdef _DEBUG
+    std::cout << "Not valid box: rectangle" << std::endl;
+  #endif

Please remove all temporary debug stuff.
Debug builds should not generate unexpected output to std::cout.

+private:
+
+};

Redundant empty section.

+      aXDir.SetX (Draw::Atof(a[anArgIter + 1]));
+      aXDir.SetY (Draw::Atof(a[anArgIter + 2]));
+      aXDir.SetZ (Draw::Atof(a[anArgIter + 3]));

Per-component assignment will not work for gp_Dir like this.
Check documentation of SetX() for this class.
42Commenter 3
Branch [archived branch] has been updated by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Mon Feb 10 16:27:37 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/geometry/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
    
    #fix remarks

43Commenter 3
Branch [archived branch] has been created by Commenter 2.

[revision removed]


Detailed log of new commits:

Author: Commenter 2
Date: Mon Feb 10 16:32:56 2020 +0300

    0031336: Modeling data - extend BRepPrimAPI_MakeBox with planar shape creation
    
    -Create a new package BRepPreviewAPI, inherited from BRepPrimAPI to create both, valid and degenerative shapes
    -Create a new class BRepPreviewAPI_MakeBox for working with a box
        Preview can be vertex, edge, rectangle or box
    -BRepPrim_GWedge: in the case of non-valid data, the exception does not happen in the constructor, but at the moment of access to the shape
    -BRepPrimAPI_MakeBox: myWedge is now not private, but protected, because used in BRepPreviewAPI_MakeBox which inherits from BRepPrimAPI_MakeBox
    -Add tests for checking of a creation a preview in tests/geometry/preview (vertex, edge, rectangle, box)
    -Update a command "box": add new parameters
44Commenter 2
Dear Mikhail

could you please review CR31336_7.

Thank you a lot, Svetlana
45Commenter 1
Tested in framework of WEEK-7

Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16972.490000000067 / 16995.740000000096 [-0.14%]
Products
Total CPU difference: 11439.249999999975 / 11455.569999999962 [-0.14%]
Windows-64-VC14:
OCCT
Total CPU difference: 18436.9375 / 18439.6875 [-0.01%]
Products
Total CPU difference: 13329.09375 / 13351.46875 [-0.17%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention
46Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
47Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
48Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
49Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
50Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
51Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
52Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]
53Commenter 3
Branch [archived branch] has been deleted by Participant.

[revision removed]

Related records