PR #6761 -- Part workbench Link support

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Post Reply
TheMarkster
Veteran
Posts: 4511
Joined: Thu Apr 05, 2018 1:53 am

PR #6761 -- Part workbench Link support

Post by TheMarkster »

https://github.com/FreeCAD/FreeCAD/pull/6761

This file crashes with this PR:

To reproduce, open the file and refresh.
sweep_test_crash.FCStd
(11.85 KiB) Downloaded 63 times
How to prevent the crash?


This file results in a ruled surface that is incorrectly placed (except because of a new property I added it doesn't happen unless the new property "Reset Placement" is set to True).
ruled_surface.FCStd
(6.78 KiB) Downloaded 20 times
Would it be better to remove the workaround code entirely? I think it's better to have the property in case it is still needed in some situations. Is there a better way to handle this situation?

Note: setting the property back to false does not reset the placement. I didn't want to change the placement in case the user has moved it.


This PR is in response to this post:

https://forum.freecadweb.org/viewtopic.php?f=3&t=67960

Basically, it adds support for App::Link sublinks by replacing calls to Part::TopoShape::getSubShape() with Part::Feature::getTopoShape(docobj, subelementname, true /*need subelement*/).getShape(). Both functions return the TopoDS_Shape of the subshape, but getTopoShape() handles the case where docobj is App::Link sublink.
openBrain
Veteran
Posts: 7988
Joined: Fri Nov 09, 2018 5:38 pm

Re: PR #6761 -- Part workbench Link support

Post by openBrain »

TheMarkster wrote: Sun Apr 17, 2022 8:05 pm This file crashes with this PR:
According my tests, the file also crashes without the PR. It crashes in OCC actually.
There is no reason why this PR should be delayed of the crash because it's not due to it. ;)
Would it be better to remove the workaround code entirely? I think it's better to have the property in case it is still needed in some situations. Is there a better way to handle this situation?
I looked at the PR but lack information.
But yes, better avoid a workaround if possible. Especially here it looks like a workaround on another one, which doesn't smell good generally. ;)
Could you give some details about the problem ? Implied code lines, and what they do or are intended to (functionally) do ?
TheMarkster
Veteran
Posts: 4511
Joined: Thu Apr 05, 2018 1:53 am

Re: PR #6761 -- Part workbench Link support

Post by TheMarkster »

openBrain wrote: Mon May 09, 2022 4:54 pm
TheMarkster wrote: Sun Apr 17, 2022 8:05 pm This file crashes with this PR:
According my tests, the file also crashes without the PR. It crashes in OCC actually.
There is no reason why this PR should be delayed of the crash because it's not due to it. ;)
Would it be better to remove the workaround code entirely? I think it's better to have the property in case it is still needed in some situations. Is there a better way to handle this situation?
I looked at the PR but lack information.
But yes, better avoid a workaround if possible. Especially here it looks like a workaround on another one, which doesn't smell good generally. ;)
Could you give some details about the problem ? Implied code lines, and what they do or are intended to (functionally) do ?
https://github.com/FreeCAD/FreeCAD/blob ... s.cpp#L244

This is where the placement is getting reset. The new property will default to false, so this workaround does not get applied. But it can be set to True for those cases where it might still be needed. But if the workaround is no longer needed then it could be better to just remove it.
Post Reply