View Issue Details

IDProjectCategoryView StatusLast Update
0033791Open CASCADEOCCT:Shape Healingpublic2024-10-01 13:45
Reporteroan Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status verifiedResolutionfixed 
Target Version7.8.1Fixed in Version7.9.0 
Summary0033791: Shape Healing - ShapeCustom not take location of source shape for the cached context and misses root one
DescriptionContext of ShapeCustom stores objects without location while the source shape comes with location.
ShapeCustom does not take that fact trying to find the shape that is already processed and cached in the context.
Additionally, ShapeCustom checks only subshapes of the source shape for possible modifications, but not the shape itself thus missing some history information.
Steps To ReproduceNone
TagsNo tags attached.
Test case numbernone

Relationships

related to 0033792 newika Open CASCADE Shape Healing - Add API to BRepTools_Modifier to check if modification exists instead of handling exception from ModifiedShape 

Activities

git

2024-08-06 14:15

administrator   ~0116380

Branch CR33791 has been created by oan.

SHA-1: a25361e54b51985bc0b7c9f56f4fa758c0abfc7a


Detailed log of new commits:

Author: oan
Date: Tue Aug 6 12:15:22 2024 +0100

    0033791: Shape Healing - ShapeCustom does not take location of the source shape when searching for the cached context and misses root one
    
    Check the context for a cached shape using a reference shape without location.
    Update history of changes by the source shape (if changed), not only by its subshapes.

oan

2024-08-06 20:39

developer   ~0116400

Test reports:
http://jenkins-test-10.nnov.opencascade.com/view/CR33791-CR33710-oan/view/COMPARE/

To integrate:
REQUIRES: CR33710
OCCT: CR33791
PROD: None

ika

2024-08-07 17:20

manager   ~0116410

Dear oan,

I do not have any remarks on this issue, so maybe it will be better to leave OCCT changes fully here and remove them from #33710? It looks more natural if the Product issue requires an OCCT one.

dpasukhi

2024-08-07 17:42

administrator   ~0116411

The issue has complicated relation with CR33791. And according new workflow we have no separate PROD and OCCT ticket as much as possible.
Especially on current task.
I ask for move anything from 33710 related with OCCT to 33791.

Remarks:

Need to update BRepTools_Modifier to have new method without exception.
Exception is a devil way. Need to avoid them as much as possible.
    try
    {
      OCC_CATCH_SIGNALS
      aResult = theModifier.ModifiedShape (theShape);
    }
    catch (Standard_NoSuchObject const&)
    {
      // the sub shape isn't in the map
      aResult.Nullify();
    }

Please avoid so complicated update, split at least on a few lines.
    return C.Oriented ( S.Orientation() ).Located ( S.Location(), Standard_False );

dpasukhi

2024-08-07 18:05

administrator   ~0116412

@oan please additionally update the summary with a little shorter description.
for both issue if possible.
The description will be not visible in web git. Especially GitHub.

oan

2024-08-08 13:40

developer   ~0116422

Last edited: 2024-08-08 13:40

@dpasukhi, I will not change description, because it has no relation to the patch and provides the general human-readable description of the problem.
Do it yourself in the way your want if you see the necessity. For both issues.

oan

2024-08-08 14:09

developer   ~0116424

Remark related to the exception handling has no relation to the current task and utilizes the only possible way provided by BRepTools_Modifier.

New issue has been registered 0033792.

oan

2024-08-08 18:03

developer   ~0116431

@dpasukhi

According to the new practice of separation of issues, base changes required by this patch is moved to #0033736 issue.

It is not recommended to move those changes here, because the nature of the problem is different.

git

2024-08-08 18:07

administrator   ~0116433

Branch CR33791 has been updated forcibly by oan.

SHA-1: fc951ac4ee64891112389a29d162b53afa9d0b3f

oan

2024-08-08 18:09

developer   ~0116436

@dpasukhi

Please review the changes then, if OK, I will start non-regression testing again.

dpasukhi

2024-08-08 18:34

administrator   ~0116437

@oan I recommend to combine changes into single branch (single ticket)

my previous remarks still actual

dpasukhi

2024-08-08 18:35

administrator   ~0116438

Last edited: 2024-08-08 18:37

It difficult to review a not combined changes on one functionality.
There will be some remarks with naming and using search mechanism (after combining)

oan

2024-08-08 21:01

developer   ~0116445

@dpasukhi

there is nothing to combine, there are two separate and independent issues, this one, and 0033791 and there is a single commit for each of them.

0033791 requires changes made in context of this issue, given that current issue should have been integrated into master by the end of June. Now it is August.

dpasukhi

2024-08-08 21:04

administrator   ~0116447

33791 must be combined with 33736 and process together.
There are no needs or reason to separate them
33736 rework almost each line of 33791.

git

2024-08-08 21:59

administrator   ~0116455

Branch CR33791 has been updated forcibly by oan.

SHA-1: 16b2f1a6d0c98e2bf5716da6cd60a14b010fab39

oan

2024-08-08 22:06

developer   ~0116458

@dpasukhi

Please review the changes then, if OK, I will start non-regression testing again.

dpasukhi

2024-08-08 22:30

administrator   ~0116462

Last edited: 2024-08-08 22:32

I would ask to rework now or create a small ticket to rework "Exception" isBound checking. But probably I will do by myself, because better to rework a multiple places with the same problem inside single commit (with common goal). It is just my though, nothing more. Using exception is a devil way :)
+    {
+      OCC_CATCH_SIGNALS
+      aResult = theModifier.ModifiedShape (theShape);
+    }

Additionally, there nullifying the variable instead of return false. But it can be fixed later. Because now it is just copy-past, No issue for now.
+      // the sub shape isn't in the map
+      aResult.Nullify();


Not necessary space. Just recommendation to remove. Not required.
-      shape.Location ( nullLoc );
-      TopoDS_Shape res;
+    for ( TopoDS_Iterator it(SF); it.More() && aPS.More(); it.Next())

New variable with incorrect style and with not clear name
+      TopoDS_Shape shared = it.Value();

Instead of Seek what about use Find(key, result)? It returs bool if was found.
+      TopoDS_Shape res;
+      if (const TopoDS_Shape* found = context.Seek (shared))
+      {
+        res = *found;
+        res.Orientation (shape.Orientation());
+        res.Location    (shape.Location(), Standard_False);
+      }
->
+      TopoDS_Shape res;
+      if (context.Find(shared, res))
+      {
+        res.Orientation (shape.Orientation());
+        res.Location    (shape.Location(), Standard_False);
+      }

dpasukhi

2024-08-08 22:34

administrator   ~0116463

Thank you for your understanding and combining :)

There are few remarks, but the main is about search logic and new variable.

As for a PROD branch, I will update Francisco profile manually during merging.

git

2024-08-09 12:10

administrator   ~0116470

Branch CR33791 has been updated forcibly by oan.

SHA-1: 2ce68bf73ec13f4377951dd53529f5d9ea539c40

oan

2024-08-09 12:13

developer   ~0116471

I would ask to rework now or create a small ticket to rework "Exception" isBound checking.

I have already created a new issue 0033792.

Additionally, there nullifying the variable instead of return false. 

To be fixed during implementation of IsBound check.

Other remarks are fixed. Please check the final result.

dpasukhi

2024-08-09 12:27

administrator   ~0116474

No more remarks

dpasukhi

2024-08-09 12:28

administrator   ~0116476

Last edited: 2024-08-09 12:34

Waiting for test result with #33710

dpasukhi

2024-08-09 17:45

administrator   ~0116479

Will be integrated into IR and tested together with other issues.
OCCT - CR33791

oan

2024-08-09 21:18

developer   ~0116487

Test reports:
http://jenkins-test-10.nnov.opencascade.com/view/CR33791-CR33710-oan-2/view/COMPARE/

git

2024-10-01 13:45

administrator   ~0116783

Branch CR33791 has been deleted by vglukhik.

SHA-1: 2ce68bf73ec13f4377951dd53529f5d9ea539c40

Issue History

Date Modified Username Field Change
2024-08-06 14:09 oan New Issue
2024-08-06 14:09 oan Assigned To => ika
2024-08-06 14:10 oan Assigned To ika => oan
2024-08-06 14:15 git Note Added: 0116380
2024-08-06 19:02 oan Status new => assigned
2024-08-06 20:39 oan Assigned To oan => ika
2024-08-06 20:39 oan Status assigned => resolved
2024-08-06 20:39 oan Steps to Reproduce Updated
2024-08-06 20:39 oan Note Added: 0116400
2024-08-07 17:20 ika Assigned To ika => oan
2024-08-07 17:20 ika Status resolved => feedback
2024-08-07 17:20 ika Note Added: 0116410
2024-08-07 17:42 dpasukhi Note Added: 0116411
2024-08-07 17:42 dpasukhi Status feedback => assigned
2024-08-07 17:43 dpasukhi Project Internal => Open CASCADE
2024-08-07 18:05 dpasukhi Note Added: 0116412
2024-08-08 13:40 oan Note Added: 0116422
2024-08-08 13:40 oan Note Edited: 0116422
2024-08-08 13:40 oan Note Edited: 0116422
2024-08-08 13:47 dpasukhi Summary Shape Healing - ShapeCustom does not take location of the source shape when searching for the cached context and misses root one => Shape Healing - ShapeCustom not take location of source shape for the cached context and misses root one
2024-08-08 14:07 oan Relationship added parent of 0033792
2024-08-08 14:09 oan Note Added: 0116424
2024-08-08 18:03 oan Note Added: 0116431
2024-08-08 18:07 git Note Added: 0116433
2024-08-08 18:09 oan Assigned To oan => dpasukhi
2024-08-08 18:09 oan Status assigned => feedback
2024-08-08 18:09 oan Note Added: 0116436
2024-08-08 18:34 dpasukhi Note Added: 0116437
2024-08-08 18:35 dpasukhi Note Added: 0116438
2024-08-08 18:37 dpasukhi Note Edited: 0116438
2024-08-08 18:38 dpasukhi Assigned To dpasukhi => oan
2024-08-08 21:01 oan Note Added: 0116445
2024-08-08 21:01 oan Assigned To oan => dpasukhi
2024-08-08 21:04 dpasukhi Note Added: 0116447
2024-08-08 21:04 dpasukhi Assigned To dpasukhi => oan
2024-08-08 21:04 dpasukhi Status feedback => assigned
2024-08-08 21:59 git Note Added: 0116455
2024-08-08 21:59 oan Assigned To oan => dpasukhi
2024-08-08 21:59 oan Status assigned => resolved
2024-08-08 22:06 oan Note Added: 0116458
2024-08-08 22:30 dpasukhi Note Added: 0116462
2024-08-08 22:32 dpasukhi Note Edited: 0116462
2024-08-08 22:34 dpasukhi Assigned To dpasukhi => oan
2024-08-08 22:34 dpasukhi Status resolved => assigned
2024-08-08 22:34 dpasukhi Note Added: 0116463
2024-08-09 12:10 git Note Added: 0116470
2024-08-09 12:13 oan Assigned To oan => dpasukhi
2024-08-09 12:13 oan Status assigned => feedback
2024-08-09 12:13 oan Note Added: 0116471
2024-08-09 12:27 dpasukhi Note Added: 0116474
2024-08-09 12:27 dpasukhi Status feedback => resolved
2024-08-09 12:28 dpasukhi Note Added: 0116476
2024-08-09 12:34 dpasukhi Note Edited: 0116476
2024-08-09 17:45 dpasukhi Assigned To dpasukhi => bugmaster
2024-08-09 17:45 dpasukhi Status resolved => reviewed
2024-08-09 17:45 dpasukhi Note Added: 0116479
2024-08-09 21:18 oan Note Added: 0116487
2024-08-10 13:53 dpasukhi Relationship replaced related to 0033792
2024-08-10 13:53 dpasukhi Status reviewed => verified
2024-08-10 13:53 dpasukhi Resolution open => fixed
2024-08-10 13:53 dpasukhi Fixed in Version => 7.9.0
2024-08-10 13:53 dpasukhi Test case number => none
2024-10-01 13:45 git Note Added: 0116783