Shape Healing - ShapeCustom not take location of source shape for the cached context and misses root one

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
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.

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

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

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.

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 );

Commenter 5 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.

Commenter 6, 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.

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.

Commenter 6

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.

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

[revision removed]

Commenter 6

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

Commenter 5 I recommend to combine changes into single branch (single ticket)

my previous remarks still actual

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

Commenter 6

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.

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.

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

[revision removed]

Commenter 6

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

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);
+      }

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.

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

[revision removed]

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.

No more remarks

Waiting for test result with #33710

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

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

Branch [archived branch] has been deleted by Participant.

[revision removed]