MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0028026Open CASCADE[OCCT] OCCT:Modeling Datapublic2016-10-28 15:032020-12-04 11:58
Reporterkgv 
Assigned Toakaftasev 
PrioritynormalSeverityminor 
StatusassignedResolutionopen 
PlatformWindowsOSVC++ 2015OS Version64 bit
Product Version[OCCT] 6.9.0 
Target Version[OCCT] 7.6.0*Fixed in Version 
Summary0028026: Modeling Data - BRepTools::Clean() does not clean all triangulation from shape
DescriptionIt seems that in some cases BRepTools::Clean() does not clean all triangulation defined in the attached shape Ball.brep.

Ball.brep:
> PolygonOnTriangulations 53
> Triangulations 24

This is regression since OCCT 6.9.0 release.
Steps To Reproduce
pload MODELING
restore Ball.brep b
trinfo
# shows no triangulation, but it is there

tcopy b b2
save b2 b2.brep
# file does not contain triangulation (150 KB)

tclean b
save b b3.brep
# file still saved with triangulation (2 MB)
TagsNo tags attached.
Test case numberbugs moddata_3 bug28026
Attached Files? file icon Ball.brep (2,319,146 bytes) 2016-10-28 15:03
? file icon bug28026 (943 bytes) 2017-09-29 12:33
png file icon 25479_descr.PNG (20,027 bytes) 2020-12-01 12:59

- Relationships
related to 0027356closedbugmaster Open CASCADE BRepTools::Clean() does not clean free edges from Poly_Polygon3D 

-  Notes
(0069873)
apv (tester)
2017-08-25 13:59

Problem described in issue is reproduced on current state of OCCT.
(0070819)
msv (developer)
2017-09-25 12:41

The default behavior of BRepTools::Clean() cannot be changed back without regression. It is because the method does not have access to all shapes that may share the same edges with the input shape. So, in order not to destruct possible other shapes in the current session it must not clean polygons of triangulations not related to faces of the current shape.
However, in order to have possibility to get rid of redundant polygons we shall add the optional Boolean flag to the method that will force removal.
The command tclean will be also extended with this optional flag.
(0070974)
apv (tester)
2017-09-29 12:33

Test scenario has been added
(0097112)
git (administrator)
2020-11-27 18:08

Branch CR28026 has been created by akaftasev.

SHA-1: b63ca17625969689a692618121005e1d38b72263


Detailed log of new commits:

Author: akaftasev
Date: Fri Nov 27 18:07:39 2020 +0300

    0028026: Modeling Data - BRepTools::Clean() does not clean all triangulation from shape
    
    Added flag for force clean triangulation and treatment for it
(0097173)
git (administrator)
2020-11-30 16:05

Branch CR28026 has been updated forcibly by akaftasev.

SHA-1: cc9f55ee813430de0a2991dcc367c140e6eef826
(0097183)
emv (developer)
2020-12-01 10:32

src/BRepTools/BRepTools.hxx
 - Why two Clean methods instead of one with default parameter?
 - Use const Standard_Boolean for similarity
 - Please give a meaningful name to the flag
 - The description of the method says:
   //! In case polygonal representation is the only available representation
   //! for the shape (shape does not have geometry) it is not removed.
   Will the new flag forcibly remove the only available representation?
   If so, please make it clear in the description.
   
src/BRepTools/BRepTools.cxx
 - It seems in force mode the 3D polygons of edges are not removed, only polygon on triangulation.

tests/bugs/moddata_3/bug28026
 - remove # source c:/repKaf/CR28026/bug28026
 - save tmp files into $imagedir/${casename}_b*.brep
 - replace "ERROR: OCC28026 is reproduced. The command" with "Error - the command ..."
(0097187)
kgv (developer)
2020-12-01 11:13

Method description states:
 151   //! Removes all cached polygonal representation of the shape,
 152   //! i.e. the triangulations of the faces of <S> and polygons on
 153   //! triangulations and polygons 3d of the edges.

therefore polygon-on-triangulation are expected to be cleared without any extra flags.

I may expect that enabling added code snippet would cause a regression for #0025479 use case.
(0097190)
emv (developer)
2020-12-01 11:30
edited on: 2020-12-01 11:47

>I may expect that enabling added code snippet would cause a regression for
#0025479 use case.
Yes, I guess so. Without force flag there should not be any change in 25479.

>therefore polygon-on-triangulation are expected to be cleared without any extra flags.
What are you proposing? Always clearing all polygons on triangulation or just changing the method description?

(0097193)
kgv (developer)
2020-12-01 12:01
edited on: 2020-12-01 12:02

My proposal is to understand why #0025479 causes regression and try to propose a solution which would handle both cases as expected.

So that as first step it is necessary to understand what #0025479 is supposed to fix (e.g. - was it intended to never clean polygon-on-triangulation, which would sounds strange, or something else?) and why it causes misbehavior in some cases (I guess not in every case).

(0097194)
emv (developer)
2020-12-01 12:17

I guess the initial version cleaned all polygons on triangulation, and version from #25479 cleans only those polygons built on the face contained in the given shape.
In my opinion, we cannot guess the user's intention for mesh removal, and should provide meaningful options so the user should decide which data to remove.

Same goes for CleanGeometry method - I guess after geometry removal, some edges may still have CurveOnSurface representations.
(0097195)
kgv (developer)
2020-12-01 12:25
edited on: 2020-12-01 12:25

> ...version from 0025479 cleans only those polygons built
> on the face contained in the given shape.
I have the same understanding - but within the given scenario, the whole shape is passed to BRepTools::Clean(), not some sub-shape! Hence, it looks like a bug in the logic introduced by #0025479.

> In my opinion, we cannot guess the user's intention for mesh removal,
> and should provide meaningful options so the user should decide
> which data to remove.
I agree that BRepTools::Clean() should provide an option clearing all polygonal representations without skipping potentially shared instances, as it was done before #0025479. But from my understanding this could be another issue on Bugtracker.

(0097197)
emv (developer)
2020-12-01 12:51

I don't have access to #0025479, but in the test case I see only the following call:
tclean a_2
tricheck a_1

So, it means not the whole shape is cleaned.

Why creating another issue if it is already done in frames of this one?
(0097198)
kgv (developer)
2020-12-01 12:58

> ...but in the test case I see only the following call
This issue is for another use case clearing the whole shape
> restore Ball.brep b
> tclean b

By the way, I would expect first expanding trinfo output (with new arguments?) or finding another command that would print information about polygonal representation of curves without saving shape to file for making test case simpler.

> Why creating another issue if it is already done in frames of this one?
No problem implementing this in scope of this issue - but current patch only implements this new flag.
(0097199)
msv (developer)
2020-12-01 13:01

I have attached the screenshot of summary and description of 0025479 for clarity.
(0097200)
emv (developer)
2020-12-01 13:40

> This issue is for another use case clearing the whole shape.
How would you know in the algorithm if it was given the whole shape or just a small part of it?

> I would expect first expanding trinfo output
I agree.

> but current patch only implements this new flag.
It seems the other flags are not needed. Except, may be, for the case when there is no geometry in edge/face. May be it would be helpful to introduce an additional flag for this case, so the user may explicitly specify whether to clean triangulation or not. What do you think?
(0097201)
msv (developer)
2020-12-01 14:11

In my opinion, the method Clean() must not make the given shape invalid. In this regard, the current version of the patch is dangerous. With the force flag it removes all triangulations regardless of face has or not a geometry. My vision is that the flag 'force' must act only as a permission to clean all polygons of edges, except for polygons that are needed for representation of faces of the given shape having only triangulation.

In addition, it is needed to allow removing polygons of edges that belong to the faces without triangulation. In the master version there is such code:
    if (aTriangulation.IsNull())
      continue;

It is needed to change this so that to allow removing polygons anyway.

I think it is better to add a flag (and name it "theForce") with default value false instead of creating a new overloaded method. It is needed to clearly explain its meaning that it permits removing all polygons that are not relevant to triangulations of belonging to the given shape faces.
(0097202)
msv (developer)
2020-12-01 14:16

>Same goes for CleanGeometry method - I guess after geometry
>removal, some edges may still have CurveOnSurface representations.
I support the idea to make the same behavior in the method CleanGeometry.
(0097203)
msv (developer)
2020-12-01 14:21

The command tclean must be implemented so that the order of options would be meaningless.

I propose the following description of the option -force:
-force : force delete all representations not relevant to the given shape
(0097204)
msv (developer)
2020-12-01 14:23

catch {exec rm $Ver_1FileName}
catch {exec rm $Ver_2FileName}

Do not use external command to remove a file. Use the tcl command 'file ...'
(0097205)
msv (developer)
2020-12-01 14:26

Instead of saving and comparing file size you may compare the length of the output of the command 'dump'.
(0097206)
kgv (developer)
2020-12-01 14:28
edited on: 2020-12-01 14:30

OK, I see now that my previous comments were actually floated away from use case described in this bug.

- #0025479 has modified BRepTools::Clean() so that now it removes Polygon-on-Triangulation representations of the TopoDS_Edge's only from Triangulations bound to TopoDS_Face's that are cleared by the same method call. E.g., BRepTools::Clean() will NOT clear Polygon-on-Triangulation while passing only TopoDS_Edge (and not TopoDS_Face containing this Edge) and it will NOT clear Polygon-on-Triangulation for triangulations of other TopoDS_Face's sharing the same Edge.
- 0027356 fixes the regression after #0025479 for removing 3d-polygon representations of free TopoDS_Edge (e.g. which do not have Polygon-on-Triangulation).
- I don't recall particular scenario where attached Ball.brep has appeared, but it has no Triangulations in TopoDS_Face but still has Polygon-on-Triangulation in Edges. As result, there is no way to remove these dead triangulations using BRepTools::Clean() implementing current behavior. In normal scenario, triangulating Ball.brep and calling BRepTools::Clean() will indeed remove Polygon-on-Triangulation as expected.
- #0029964 describes a similar use case, when TopoDS_Edge shape has been detached from TopoDS_Face with preserved Polygon-on-Triangulation representation. As result, there is no way to remove these dead triangulations using BRepTools::Clean().

Therefore, indeed, it looks like what we need is an option that would allow to eliminate "dead" representations from TopoDS_Edge using BRepTools::Clean() interface. Theoretically, BRepTools::Clean() might attempt to determine dead Triangulations no more attached to any TopoDS_Face, but I suppose this will be undesired overcomplication of the logic of this utility method.

One question could be discussed - if BRepTools::Clean() should clear all polygonal representations by default (restoring behavior before #0025479 and providing an option for new behavior) or to add a flag forcing such removal (as in current patch).

(0097207)
akaftasev (developer)
2020-12-01 14:33

> Instead of saving and comparing file size you may compare the length of the output of the command 'dump'.
It is another length of these files. For this reason was added variable "Tol" in test case.
(0097208)
emv (developer)
2020-12-01 14:36

Let's follow KGV's idea on extending trinfo output to check if polygons are really removed.
(0097209)
msv (developer)
2020-12-01 14:41

>One question could be discussed - if BRepTools::Clean() should
> clear all polygonal representations by default
I believe the method should by default make preference to not impact other shapes that may be present in the current process.
(0097210)
msv (developer)
2020-12-01 14:42

+1 for trinfo extension.

>It is another length of these files. For this reason was added
> variable "Tol" in test case.
You can use the tolerance to compare lengths of dumps as well.
(0097212)
akaftasev (developer)
2020-12-01 14:52

In this test:
  in original shape - 54 Locations;
  in "tcopy" shape - 42 Locations;
  in "tclean" shape - 28 Locations,
and I think, that this tolerance will be too big.
(0097213)
kgv (developer)
2020-12-01 14:56

>>One question could be discussed - if BRepTools::Clean() should
>> clear all polygonal representations by default
> I believe the method should by default make preference to not
> impact other shapes that may be present in the current process.
OK, lets stick to the new behavior introduced by #0025479 and add an option clearing all polygonal representations for shapes having geometry representation.
(0097214)
msv (developer)
2020-12-01 15:29

>and I think, that this tolerance will be too big.
OK, extend trinfo and use it for checking if cleaning was OK.
(0097233)
akaftasev (developer)
2020-12-02 11:54
edited on: 2020-12-02 11:54

As I undarstand, in addition to displaying the number of triangles and nodes, need to display number of segments of polygons on triangulation and segments of polygons 3D?

(0097308)
git (administrator)
2020-12-04 11:58

Branch CR28026 has been updated forcibly by akaftasev.

SHA-1: f1063b6afa315f72e04c54ff8769160fb108672e

- Issue History
Date Modified Username Field Change
2016-10-28 15:03 kgv New Issue
2016-10-28 15:03 kgv Assigned To => msv
2016-10-28 15:03 kgv File Added: Ball.brep
2016-10-28 15:08 kgv Product Version 7.0.0 => 6.9.0
2016-10-28 15:08 kgv Description Updated View Revisions
2016-10-28 15:09 kgv Relationship added related to 0025479
2016-10-28 15:11 kgv Description Updated View Revisions
2016-10-28 16:01 msv Target Version 7.1.0 => 7.2.0
2017-07-20 15:30 msv Target Version 7.2.0 => 7.3.0
2017-08-25 13:59 apv Test case number => bugs moddata_3 bug28026
2017-08-25 13:59 apv Note Added: 0069873
2017-09-18 14:18 kgv Relationship added related to 0027356
2017-09-25 12:41 msv Note Added: 0070819
2017-09-29 12:33 apv File Added: bug28026
2017-09-29 12:33 apv Note Added: 0070974
2017-12-05 16:59 msv Target Version 7.3.0 => 7.4.0
2018-07-17 08:58 kgv Relationship added related to 0029964
2019-08-12 17:45 msv Target Version 7.4.0 => 7.5.0
2020-09-14 22:54 msv Target Version 7.5.0 => 7.6.0*
2020-11-19 17:07 szy Assigned To msv => akaftasev
2020-11-19 17:07 szy Status new => assigned
2020-11-27 18:08 git Note Added: 0097112
2020-11-30 16:05 git Note Added: 0097173
2020-11-30 16:05 akaftasev Assigned To akaftasev => emv
2020-11-30 16:05 akaftasev Status assigned => resolved
2020-12-01 10:32 emv Note Added: 0097183
2020-12-01 10:32 emv Assigned To emv => akaftasev
2020-12-01 10:32 emv Status resolved => assigned
2020-12-01 11:13 kgv Note Added: 0097187
2020-12-01 11:30 emv Note Added: 0097190
2020-12-01 11:47 emv Note Edited: 0097190 View Revisions
2020-12-01 12:01 kgv Note Added: 0097193
2020-12-01 12:02 kgv Note Edited: 0097193 View Revisions
2020-12-01 12:17 emv Note Added: 0097194
2020-12-01 12:25 kgv Note Added: 0097195
2020-12-01 12:25 kgv Note Edited: 0097195 View Revisions
2020-12-01 12:51 emv Note Added: 0097197
2020-12-01 12:58 kgv Note Added: 0097198
2020-12-01 12:59 msv File Added: 25479_descr.PNG
2020-12-01 13:01 msv Note Added: 0097199
2020-12-01 13:40 emv Note Added: 0097200
2020-12-01 14:11 msv Note Added: 0097201
2020-12-01 14:16 msv Note Added: 0097202
2020-12-01 14:21 msv Note Added: 0097203
2020-12-01 14:23 msv Note Added: 0097204
2020-12-01 14:26 msv Note Added: 0097205
2020-12-01 14:28 kgv Note Added: 0097206
2020-12-01 14:28 kgv Note Edited: 0097206 View Revisions
2020-12-01 14:30 kgv Note Edited: 0097206 View Revisions
2020-12-01 14:30 kgv Note Edited: 0097206 View Revisions
2020-12-01 14:30 kgv Note Edited: 0097206 View Revisions
2020-12-01 14:33 akaftasev Note Added: 0097207
2020-12-01 14:36 emv Note Added: 0097208
2020-12-01 14:41 msv Note Added: 0097209
2020-12-01 14:42 msv Note Added: 0097210
2020-12-01 14:52 akaftasev Note Added: 0097212
2020-12-01 14:56 kgv Note Added: 0097213
2020-12-01 15:29 msv Note Added: 0097214
2020-12-02 11:54 akaftasev Note Added: 0097233
2020-12-02 11:54 akaftasev Note Edited: 0097233 View Revisions
2020-12-04 11:58 git Note Added: 0097308


Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker