DiscussionsIssue archiveOCCT:Modeling Algorithms

Archived issue #0028456

BRepBuilderAPI_MakeFace modifies the input shape

Open CASCADEOCCT:Modeling Algorithmsclosed27 public notes

Search issues

Description

BRepBuilderAPI_MakeFace modifies the input wire: updates the tolerances of wire's edges/vertices or/and updates flags/ranges.
It's proposed to add mutable flag for BRepBuilderAPI_MakeShape which defines if input shape can be modified during make-face operation or some of subshapes from input shape should be empty-copied to avoid modifying.

Steps to reproduce

box a 1 1 1
explode a w
setflags a_1 locked
mkplane result a_1

...
also test cases: mkface mkplane A1 A2 (with additional setflags <inp_sh> locked before mkplane operation)

Public activity

27 archived notes

Participants are labeled by their role within this record.

01Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Tue Feb 14 18:42:54 2017 +0300

    0028456: BRepBuilderAPI_MakeFace modifies the input shape
    
    (draft version)
    1) New 'MutableInput' flag have been added
    2) BRepLib::UpdateTolerances() now supports non-mutable input (finished, but seems to be unoptimal)
    3) BRepLib::SameParameter() also supports non-mutable, but this code unfinished yet
02Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Tue Feb 14 18:42:54 2017 +0300

    0028456: BRepBuilderAPI_MakeFace modifies the input shape
    
    1) New 'MutableInput' flag have been added
    2) BRepLib::UpdateTolerances() & BRepLib::SameParameter() now supports non-mutable input
    3) new "-mi" option have been added to mkplane command
03Commenter 3
Remarks:

Commit message:
1) have => has
2) supports => support
3) have => has
Point that you added new flag in the ancestor algorithm, root of all others. Point the default value.

src\BRepBuilderAPI\BRepBuilderAPI_MakeShape.hxx
- Point the default value of the flag.
- 69: use const for argument.
- It is better to make myMutableInput private, and methods IsMutableInput and SetMutableInput non-virtual (so that to safely call from constructor).

src\BRepBuilderAPI\BRepBuilderAPI_MakeFace.hxx
- 120: wrap the long line.

src\BRepBuilderAPI\BRepBuilderAPI_MakeFace.cxx
- 269: use the method SetMutableInput.

src\BRepLib\BRepLib_MakeFace.cxx
- 269: move into the scope where it is used. Follow coding rules when naming new variables (prefix 'a', see https://dev.opencascade.org/doc/overview/html/occt_dev_guides__coding_rules.html#occt_coding_rules_2_3). What is ECSH? Can you drop a line in comments?
- 273: what is wrong about forced?
- 277: not clear question

src\BRepLib\BRepLib.hxx
- Follow coding rules for naming of arguments (prefix 'the').
- 141, 149: add description
- 142,165,174: use const for non-modifiable arguments.
- 156-159: absence of punctuation makes hard to understand the text. Also, use
tags to mark the end of line.
"and the new shape will be returned." => "In this case the new shape will be returned."
- 161: "This" => "These".

src\BRepLib\BRepLib.cxx
- use BRepTools_ReShape instead of data map for mapping old->new shape.
- 1201: no such method in the class.
- Please add standard comment header before each method.
- 757: mismatch name
- 834: what kind of checks do you mean?
- 897-899: use BRep_Builder::UpdateFace
- 949-950, 1618: I don't see why do you separate vertices, edges and faces.
- 1784: not clear question

src\BRepTest\BRepTest_SurfaceCommands.cxx
- 185: yes, add usage
- 196: historical reasons, let's leave here as it is.
- Change option name to "-nmi". Character '-' here has no meaning of negation, and "-mi" can be confusing.

04Commenter 2
Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Fri Mar 10 15:19:23 2017 +0300

    corrections

05Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
06Commenter 6
Remarks:

- As we agreed, changes in MakeShape, MakeFace and BRepTest_SurfaceCommands.cxx are to be dropped.

src\BRepTools\BRepTools_ReShape.hxx
- Add description of the method IsNewShape.
- What is the meaning of the name 'mySMap'? May be to rename it something like myNewShapes?

src\BRepLib\BRepLib.hxx
- 140: why 'AnEdge'? Please read coding rules. 'theEdge' is correct!
- add description of SameParameter at 142
- 149: remove the empty line
- 158: extra '.' at the end.
- update description of the method SameParameter at 164.
- add description of UpdateTolerances at 174
- remove argument isMutableInput from public methods. Methods with reshaper must write all changes in reshaper only. In implementation, make the current methods static and call them from API methods. It is better in implementation accept pointer to reshaper, in order to not create an instance when it is not needed. Null pointer means 'mutable input'. In method SameParameter make reshaper last argument.

src\BRepLib\BRepLib.cxx
- 912: missing header
- 1178: again, no such method in the class. How is it compiled?
- 1590: missing header
07Commenter 2
Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Fri Mar 31 14:27:35 2017 +0300

    corrections

08Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
09Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
10Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
11Commenter 11
src/BRepTools/BRepTools_ReShape.hxx
- 142: 'have' -> 'has', and put point at end.

src/BRepLib/BRepLib.hxx
- 29-30: remove unused includes
- 146-150: use punctuation and capital letters to distinct sentences (otherwise doxygen will generate a mess).

src\BRepLib\BRepLib.cxx
- 996: use InternalUpdateTolerances here.

- Squash into one commit and put in the new branch.
12Commenter 12
And please rebase on last master.
13Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Tue Feb 14 18:42:54 2017 +0300

    0028456: BRepBuilderAPI_MakeFace modifies the input shape
    
    1) BRepLib::UpdateTolerances(..) & BRepLib::SameParameter(..) functions now support non-mutable input feature. reshaper is used to store modified copies of subshapes of original (input) shape(s) as substitutions.
    2) IsNewShape(..) method has been added to BRepTools_ReShape to check if the given shape has been recorded as a value
14Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
15Commenter 15
- Add possibility in the commands sameparameter/fsameparameter to use the new safe API.
- Create several test cases for the new API of sameparameter in the category heal/same_parameter.
- Add information about new method of reshaper IsNewShape() in its documentation here: https://dev.opencascade.org/doc/overview/html/occt_user_guides__shape_healing.html#occt_shg_5_1
16Commenter 2
Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 10 17:51:02 2017 +0300

    update of tests/docs

17Commenter 17
src\BRepTest\BRepTest_BasicCommands.cxx
- 330,367: there will be warning on other platforms; include sub-expressions in parentheses.
18Commenter 2
Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Tue Apr 11 15:38:16 2017 +0300

    remark

19Commenter 19
Reviewed.
20Commenter 20
Dear Commenter 1,

Branch CR28456_2 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: 6 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 1 (0 on master)
products component:
   Linux: 64
   Windows: 0
   MacOS: 1210
New warnings have been detected during OCCT component building
on Linux:
http://jenkins-test-05.nnov.opencascade.com/view/CR28456_2-master/job/CR28456_2-master-OCCT-Debian70-64-opt-compile/1/warnings17Result/new/
on MacOS:
http://jenkins-test-05.nnov.opencascade.com/view/CR28456_2-master/job/CR28456_2-master-OCCT-MacOS-opt-compile/1/warnings7Result/

Regressions/Differences:
http://occt-tests/CR28456_2-master-OCCT/Debian70-64/summary.html
http://occt-tests/CR28456_2-master-OCCT/Windows-64-VC10/summary.html
bugs modalg_1 buc60896

Testing cases:
heal same_parameter_locked - OK
http://occt-tests/CR28456_2-master-OCCT/Debian70-64/summary.html#heal-same_parameter_locked
http://occt-tests/CR28456_2-master-OCCT/Windows-64-VC10/summary.html#heal-same_parameter_locked
heal update_tolerance_locked - OK
http://occt-tests/CR28456_2-master-OCCT/Debian70-64/summary.html#heal-update_tolerance_locked
http://occt-tests/CR28456_2-master-OCCT/Windows-64-VC10/summary.html#heal-update_tolerance_locked

Testing on Linux:
Total MEMORY difference: 91551894 / 91388139 [+0.18%]
Total CPU difference: 19598.810000000238 / 19613.190000000242 [-0.07%]

Testing on Windows:
Total MEMORY difference: 57863693 / 57855806 [+0.01%]
Total CPU difference: 18351.318035898534 / 18063.40259029849 [+1.59%]
21Commenter 21
Dear Ilya,

Branch [archived branch] has been rejected due to:
- additional warnings
- regressions/difference/improvements
22Commenter 2
Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Wed Apr 12 14:38:26 2017 +0300

    elimination of warnings; corrections of test

23Commenter 23
Dear Alexey,

>>New warnings have been detected during OCCT component building
Seems to be fixed now.

>>Test bugs vis bug5682
the new changes should not affect this test case...

>>Test bugs modalg_1 buc60896
I guess this script looks somewhat incorrect.
This line:
sameparameter result tol 1.e-2
uses additional (i.e. optional) tolerance value. However, 'tol' is not a tcl value in this context. Before the patch, 'sameparamter' command ignores this 'tol' word along with the next value (1.e-2, for ex.). Thus this draw-command uses a default tolerance value (1.e-7). So I think that the 'tol' word should be dropped. I correct the test case according to this.

Please compile&test the new commit.

24Commenter 24
Dear Commenter 1,

Branch CR28456_2 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: 64
    Windows: 0
    MacOS: 1188

Regressions/Differences:
Not detected

Testing cases:
heal same_parameter_locked - OK
http://occt-tests/CR28456_2-master-OCCT/Debian70-64/summary.html#heal-same_parameter_locked
http://occt-tests/CR28456_2-master-OCCT/Windows-64-VC10/summary.html#heal-same_parameter_locked
heal update_tolerance_locked - OK
http://occt-tests/CR28456_2-master-OCCT/Debian70-64/summary.html#heal-update_tolerance_locked
http://occt-tests/CR28456_2-master-OCCT/Windows-64-VC10/summary.html#heal-update_tolerance_locked

Testing on Linux:
Total MEMORY difference: 91345181 / 91384662 [-0.04%]
Total CPU difference: 19708.93000000035 / 19613.25000000024 [+0.49%]

Testing on Windows:
Total MEMORY difference: 57862638 / 57855806 [+0.01%]
Total CPU difference: 18352.659644498486 / 18063.40259029849 [+1.60%]
25Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
26Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
27Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]

Related records