View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0033791 | Open CASCADE | OCCT:Shape Healing | public | 2024-08-06 14:09 | 2024-08-26 20:54 |
| Reporter | oan | Assigned To | bugmaster | ||
| Priority | normal | Severity | minor | ||
| Status | verified | Resolution | fixed | ||
| Target Version | 7.8.1 | Fixed in Version | 7.9.0 | ||
| Summary | 0033791: Shape Healing - ShapeCustom not take location of source shape for the cached context and misses root one | ||||
| Description | Context 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 Reproduce | None | ||||
| Tags | No tags attached. | ||||
| Test case number | none | ||||
|
|
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. |
|
|
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 );
|
|
|
@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. |
|
|
@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. |
|
|
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. |
|
|
@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. |
|
|
Branch CR33791 has been updated forcibly by oan. SHA-1: fc951ac4ee64891112389a29d162b53afa9d0b3f |
|
|
@dpasukhi Please review the changes then, if OK, I will start non-regression testing again. |
|
|
@oan 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) |
|
|
@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. |
|
|
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 CR33791 has been updated forcibly by oan. SHA-1: 16b2f1a6d0c98e2bf5716da6cd60a14b010fab39 |
|
|
@dpasukhi 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 CR33791 has been updated forcibly by oan. SHA-1: 2ce68bf73ec13f4377951dd53529f5d9ea539c40 |
|
|
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/ |
| 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 |
