View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0028026 | Open CASCADE | OCCT:Modeling Data | public | 2016-10-28 15:03 | 2022-07-04 09:42 |
Reporter | kgv | Assigned To | bugmaster | ||
Priority | normal | Severity | minor | ||
Status | closed | Resolution | fixed | ||
Platform | Windows | OS | VC++ 2015 | ||
Product Version | 6.9.0 | ||||
Target Version | 7.6.0 | Fixed in Version | 7.6.0 | ||
Summary | 0028026: Modeling Data - BRepTools::Clean() does not clean all triangulation from shape | ||||
Description | It 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) | ||||
Tags | No tags attached. | ||||
Test case number | bugs moddata_3 bug28026 | ||||
|
Ball.brep (2,319,146 bytes) |
|
Problem described in issue is reproduced on current state of OCCT. |
|
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. |
2017-09-29 12:33 tester |
bug28026 (943 bytes) |
|
Test scenario has been added |
|
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 |
|
Branch CR28026 has been updated forcibly by akaftasev. SHA-1: cc9f55ee813430de0a2991dcc367c140e6eef826 |
|
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 ..." |
|
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. |
|
>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? |
|
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). |
|
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. |
|
> ...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. |
|
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? |
|
> ...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. |
2020-12-01 12:59 developer |
25479_descr.PNG (20,027 bytes) |
|
I have attached the screenshot of summary and description of 0025479 for clarity. |
|
> 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? |
|
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. |
|
>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. |
|
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 |
|
catch {exec rm $Ver_1FileName} catch {exec rm $Ver_2FileName} Do not use external command to remove a file. Use the tcl command 'file ...' |
|
Instead of saving and comparing file size you may compare the length of the output of the command 'dump'. |
|
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). |
|
> 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. |
|
Let's follow KGV's idea on extending trinfo output to check if polygons are really removed. |
|
>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. |
|
+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. |
|
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. |
|
>>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. |
|
>and I think, that this tolerance will be too big. OK, extend trinfo and use it for checking if cleaning was OK. |
|
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? |
|
Branch CR28026 has been updated forcibly by akaftasev. SHA-1: f1063b6afa315f72e04c54ff8769160fb108672e |
|
tests/bugs/moddata_3/bug28026 after clean please check that polygonal representation is cleaned (tcopy may change its behavior one day) src/MeshTest/MeshTest.cxx shape contains nb representations of what? Make it clear, e.g. shape contains nb polygons on triangulations. src/DBRep/DBRep.cxx print help information if no enough arguments (di.PrintHelp(a[0]);) src/BRepTools/BRepTools.hxx change comment to: //! theForce - allows removing all polygonal representations from the shape, //! including polygons on triangulations irrelevant for the faces of the given shape. //! //! In case polygonal representation is the only available representation //! for the shape (shape does not have geometry) it is not removed (even in the force mode). src/BRepTools/BRepTools.cxx Why aShapeMap is only used in non-force mode? Polygon3d is still not removed in force mode. The point is that in force mode the block 842-859 should be applied additionally to removing polygons, not instead. Revert changes CleanGeometry method. The idea was to introduce similar force flag for this method, to force removal of irrelevant curves on surfaces (this should be done in a separate issue) |
|
Branch CR28026 has been updated forcibly by akaftasev. SHA-1: 39b9b2cc53a88be4d526b04a5b0b0c80f3b10fb5 |
|
Branch CR28026 has been updated forcibly by akaftasev. SHA-1: 6b8529f94525b0bacf076713e013f147fb572c7c |
|
Andrey, a new squashed branch (CR28026_1) is expected to be pushed for review. |
|
Branch CR28026_1 has been created by akaftasev. SHA-1: b371e2ebe1f4318b9f4d3bdd198723a1c92268f4 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 Command "trinfo" now show number of polygons on triangulation |
|
+ const Handle(BRep_TEdge)& aTE = *((Handle(BRep_TEdge)*) &anEdge.TShape()); It would be better using a static_cast<> instead of less safe C-cast. + //! theForce - allows removing all polygonal representations from the shape, Please use @param [in] Doxygen tags. + di << "Syntax error: wrong usage of arguments: do not use 'force' and 'geom' flags together! "; Unexpected trailing space and exclamation sign. |
|
Branch CR28026_1 has been updated forcibly by akaftasev. SHA-1: 8d864ee9cfef76a747b57eb9d90080e9324fe6ea |
|
- + . Please discard unrelated empty spaces. + //! @param theForce allows removing all polygonal representations from the shape, This will generate a Doxygen warning that not all parameters are documented - please also add simple description @param theShape. 406 for (Standard_Integer i = aStart; i < n; i++) { 407 TopoDS_Shape S = DBRep::Get(a[i]); 408 if (toRemoveGeometry) While modifying the command, please improve handling of parameters and add NULL-checks: TopoDS_Compound aCompound; BRep_Builder().MakeCompound (aCompound); for (Standard_Integer anArgIter = 1; anArgIter < n; ++anArgIter) { if (strcmp (a[anArgIter], "-geom") == 0) { ... } else { TopoDS_Shape aShape = DBRep::Get (a[anArgIter]); if (aShape.IsNull{}) { syntax error: NULL shape } BRep_Builder().Add (aCompound, aShape); } } if (aCompound.NbChildren() == 0) { syntax error: wrong number of arguments } Clean (aCompound); |
|
Branch CR28026_1 has been updated forcibly by akaftasev. SHA-1: 37d88f563f54c74ddef1bc973e87dfa93c94dd03 |
|
Branch CR28026 has been updated forcibly by akaftasev. SHA-1: 9eaa63a78825ba65273c69c9b1dcd7992c1ae0cc |
|
Branch CR28026_1 has been updated forcibly by akaftasev. SHA-1: 222ec2bf15f50bee1edce4f99a0e3d2e3b13af8b |
|
src/BRepTools/BRepTools.hxx Duplication of the string "In case polygonal representation ..." in the description src/BRepTools/BRepTools.cxx Add braces for the else statement: if (aCR->IsPolygonOnTriangulation()) { aLCR.Remove(anIterCR); } else anIterCR.Next(); Put lines 861-863 before the force block: TopLoc_Location aLoc; Handle(Poly_Polygon3D) aPoly3d = BRep_Tool::Polygon3D(anEdge, aLoc); if (!aPoly3d.IsNull()) { aBuilder.UpdateEdge(anEdge, aNullPoly3d); } |
|
Branch CR28026 has been updated forcibly by akaftasev. SHA-1: 981fdf9c3134ebdbb975374b8fcc57f9ad749aad |
|
CPU perf mesh bug30511: 14.328125 / 5.09375 [+181.29%] Force option is not used in this case. What is the reason of such performance downgrade? |
|
The reason of performance downgrade is a usage of additional loop in "trinfo" command. |
|
Patch is ready for review - OCCT: branch CR28026_2; - OCC Products: branch NONE. |
|
Reviewed. For integration to OCCT - CR28026_2. |
|
Combination - OCCT branch : IR-2021-01-22 master SHA - 8948e18df815e356c59a750bd2711cb4a7914cec a206de37fbfa0bf71bd534ae47192bbec23b8522 Products branch : IR-2021-01-22 SHA - 7cb485e51cb439852e1e1e0726af89f51d6d6db0 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: 17720.020000000055 / 17721.200000000135 [-0.01%] Products Total CPU difference: 12428.380000000123 / 12412.630000000105 [+0.13%] Windows-64-VC14: OCCT Total CPU difference: 19367.234375 / 19300.265625 [+0.35%] Products Total CPU difference: 13821.875 / 13740.03125 [+0.60%] Image differences : No differences that require special attention Memory differences : No differences that require special attention |
occt: master 5bae0beb 2020-11-27 15:07:39 Committer: bugmaster Details Diff |
0028026: Modeling Data - BRepTools::Clean() does not clean all triangulation from shape Added flag for force clean triangulation and treatment for it Command "trinfo" now show number of polygons on triangulation |
Affected Issues 0028026 |
|
mod - src/BRepTools/BRepTools.cxx | Diff File | ||
mod - src/BRepTools/BRepTools.hxx | Diff File | ||
mod - src/DBRep/DBRep.cxx | Diff File | ||
mod - src/MeshTest/MeshTest.cxx | Diff File | ||
add - tests/bugs/moddata_3/bug28026 | Diff File |
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 | |
2016-10-28 15:11 | kgv | Description Updated | |
2016-10-28 16:01 |
|
Target Version | 7.1.0 => 7.2.0 |
2017-07-20 15:30 |
|
Target Version | 7.2.0 => 7.3.0 |
2017-08-25 13:59 |
|
Test case number | => bugs moddata_3 bug28026 |
2017-08-25 13:59 |
|
Note Added: 0069873 | |
2017-09-18 14:18 | kgv | Relationship added | related to 0027356 |
2017-09-25 12:41 |
|
Note Added: 0070819 | |
2017-09-29 12:33 |
|
File Added: bug28026 | |
2017-09-29 12:33 |
|
Note Added: 0070974 | |
2017-12-05 16:59 |
|
Target Version | 7.3.0 => 7.4.0 |
2019-08-12 17:45 |
|
Target Version | 7.4.0 => 7.5.0 |
2020-09-14 22:54 |
|
Target Version | 7.5.0 => 7.6.0 |
2020-11-19 17:07 |
|
Assigned To | msv => akaftasev |
2020-11-19 17:07 |
|
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 |
|
Note Added: 0097183 | |
2020-12-01 10:32 |
|
Assigned To | emv => akaftasev |
2020-12-01 10:32 |
|
Status | resolved => assigned |
2020-12-01 11:13 | kgv | Note Added: 0097187 | |
2020-12-01 11:30 |
|
Note Added: 0097190 | |
2020-12-01 11:47 |
|
Note Edited: 0097190 | |
2020-12-01 12:01 | kgv | Note Added: 0097193 | |
2020-12-01 12:02 | kgv | Note Edited: 0097193 | |
2020-12-01 12:17 |
|
Note Added: 0097194 | |
2020-12-01 12:25 | kgv | Note Added: 0097195 | |
2020-12-01 12:25 | kgv | Note Edited: 0097195 | |
2020-12-01 12:51 |
|
Note Added: 0097197 | |
2020-12-01 12:58 | kgv | Note Added: 0097198 | |
2020-12-01 12:59 |
|
File Added: 25479_descr.PNG | |
2020-12-01 13:01 |
|
Note Added: 0097199 | |
2020-12-01 13:40 |
|
Note Added: 0097200 | |
2020-12-01 14:11 |
|
Note Added: 0097201 | |
2020-12-01 14:16 |
|
Note Added: 0097202 | |
2020-12-01 14:21 |
|
Note Added: 0097203 | |
2020-12-01 14:23 |
|
Note Added: 0097204 | |
2020-12-01 14:26 |
|
Note Added: 0097205 | |
2020-12-01 14:28 | kgv | Note Added: 0097206 | |
2020-12-01 14:28 | kgv | Note Edited: 0097206 | |
2020-12-01 14:30 | kgv | Note Edited: 0097206 | |
2020-12-01 14:30 | kgv | Note Edited: 0097206 | |
2020-12-01 14:30 | kgv | Note Edited: 0097206 | |
2020-12-01 14:33 | akaftasev | Note Added: 0097207 | |
2020-12-01 14:36 |
|
Note Added: 0097208 | |
2020-12-01 14:41 |
|
Note Added: 0097209 | |
2020-12-01 14:42 |
|
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 |
|
Note Added: 0097214 | |
2020-12-02 11:54 | akaftasev | Note Added: 0097233 | |
2020-12-02 11:54 | akaftasev | Note Edited: 0097233 | |
2020-12-04 11:58 | git | Note Added: 0097308 | |
2020-12-04 17:52 | akaftasev | Assigned To | akaftasev => emv |
2020-12-04 17:52 | akaftasev | Status | assigned => resolved |
2020-12-14 17:59 |
|
Note Added: 0097588 | |
2020-12-14 18:00 |
|
Assigned To | emv => akaftasev |
2020-12-14 18:00 |
|
Status | resolved => assigned |
2020-12-22 17:00 | git | Note Added: 0097865 | |
2020-12-22 17:47 | git | Note Added: 0097866 | |
2020-12-23 09:38 | akaftasev | Assigned To | akaftasev => emv |
2020-12-23 09:38 | akaftasev | Status | assigned => resolved |
2020-12-23 11:22 | kgv | Note Added: 0097878 | |
2020-12-23 11:22 | kgv | Assigned To | emv => akaftasev |
2020-12-23 11:22 | kgv | Status | resolved => assigned |
2020-12-23 11:22 | kgv | Note Edited: 0097878 | |
2020-12-23 13:39 | git | Note Added: 0097881 | |
2020-12-23 13:40 | akaftasev | Assigned To | akaftasev => emv |
2020-12-23 13:40 | akaftasev | Status | assigned => resolved |
2020-12-23 14:11 | kgv | Note Added: 0097882 | |
2020-12-23 16:20 | git | Note Added: 0097883 | |
2020-12-23 16:48 | kgv | Note Added: 0097884 | |
2020-12-24 11:12 | git | Note Added: 0097887 | |
2020-12-24 12:07 | git | Note Added: 0097889 | |
2020-12-24 17:00 | git | Note Added: 0097893 | |
2020-12-29 08:13 |
|
Note Added: 0097967 | |
2020-12-29 08:13 |
|
Assigned To | emv => akaftasev |
2020-12-29 08:13 |
|
Status | resolved => assigned |
2021-01-11 13:15 | git | Note Added: 0098056 | |
2021-01-11 16:35 | akaftasev | Assigned To | akaftasev => emv |
2021-01-11 16:35 | akaftasev | Status | assigned => resolved |
2021-01-15 08:27 |
|
Note Added: 0098127 | |
2021-01-15 08:27 |
|
Assigned To | emv => akaftasev |
2021-01-15 08:27 |
|
Status | resolved => assigned |
2021-01-18 12:40 | akaftasev | Note Added: 0098180 | |
2021-01-18 16:39 | akaftasev | Note Added: 0098188 | |
2021-01-18 16:39 | akaftasev | Assigned To | akaftasev => emv |
2021-01-18 16:39 | akaftasev | Status | assigned => resolved |
2021-01-19 08:58 |
|
Note Added: 0098189 | |
2021-01-19 08:58 |
|
Assigned To | emv => bugmaster |
2021-01-19 08:58 |
|
Status | resolved => reviewed |
2021-01-23 12:49 | bugmaster | Note Added: 0098369 | |
2021-01-23 12:49 | bugmaster | Status | reviewed => tested |
2021-01-23 14:14 | bugmaster | Changeset attached | => occt master 5bae0beb |
2021-01-23 14:14 | bugmaster | Status | tested => verified |
2021-01-23 14:14 | bugmaster | Resolution | open => fixed |
2022-07-04 09:42 | kgv | Relationship added | related to 0033049 |