Crash on changing fillet radius

Post here for help on using FreeCAD's graphical user interface (GUI).
Forum rules
and Helpful information
IMPORTANT: Please click here and read this first, before asking for help

Also, be nice to others! Read the FreeCAD code of conduct!
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Crash on changing fillet radius

Post by chennes »

uwestoehr wrote: Wed Jan 27, 2021 10:28 pm Yes, I am sure. My testcase with the crash issues now "Could not create fillet" and doesn't crash anymore. however, I worked now for 4 hours with this and got more than 10 crashed with chamfers and/or fillets. :cry:
This will not fix chamfers, that's a different DLL file, but probably the same sort of problem. If you are still getting crashes with fillets, I will keep trying to trace backwards to find the real problem instead of just using this stopgap solution.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Crash on changing fillet radius

Post by chennes »

I should have guessed based on the filenames. Your original crash was from code in "ChFi3d_Builder_C1.cxx" so that's where I added the extra check. But that directory actually contains:

Code: Select all

src/ChFi3d/ChFi3d_Builder.cxx
src/ChFi3d/ChFi3d_Builder.hxx
src/ChFi3d/ChFi3d_Builder_0.cxx
src/ChFi3d/ChFi3d_Builder_0.hxx
src/ChFi3d/ChFi3d_Builder_1.cxx
src/ChFi3d/ChFi3d_Builder_2.cxx
src/ChFi3d/ChFi3d_Builder_6.cxx
src/ChFi3d/ChFi3d_Builder_C1.cxx
src/ChFi3d/ChFi3d_Builder_C2.cxx
src/ChFi3d/ChFi3d_Builder_CnCrn.cxx
src/ChFi3d/ChFi3d_Builder_NotImp.cxx
src/ChFi3d/ChFi3d_Builder_SpKP.cxx
And not surprisingly, some of those other files have the exact same function defined in them, with the exact same lack of check. So if you are willing to try it, I'd like to simply add the same check to all of the matching functions, and see if it at least mitigates the crashing behavior.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Crash on changing fillet radius

Post by uwestoehr »

chennes wrote: Thu Jan 28, 2021 12:19 am And not surprisingly, some of those other files have the exact same function defined in them, with the exact same lack of check. So if you are willing to try it, I'd like to simply add the same check to all of the matching functions, and see if it at least mitigates the crashing behavior.
I am definitely willing since these many crashes are a productivity killer for me and today I found myself even getting aggressive - everything was working well, but just the filleting/chamfering cost me more than hour.

-------------
Just for info, about half of the crashes did not directly occur just by creating a fillet but due to existing ones:
- I have about 10 different fillets/chamfers at the part and due to the toponaming these are the last objects in my model tree.
- I change something in the main construction and thus get e.g. 2 new edges -> all fillets and chamfers become incorrect because of toponaming (some are now bound to the wrong edge, some even lost its edge)
- so I set the tip to the last object before the first fillet and tried to correct the fillets/chamfers one after another as they appear in the model tree. So I open the first fillet and set the correct edge. This often crashes and I found out that sometimes I previously have to delete certain fillets that are below the first fillet in the model tree. First then I could set the first fillet to the desired edge without a crash

So as strange as it is OCC also calculates fillets that occur later in the model tree and behind the tip. My new strategy is now to delete all fillets and recreate them all again. This is time consuming but not as time consuming than to get a dozen crashes until existing fillets/chamfers are reset to the desired edges.
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Crash on changing fillet radius

Post by chennes »

OK, here is a second attempt. Again, this only affects fillets right now, if it works I'll port the same idea to chamfers.

Here was my strategy: I searched for void functions that took a non-const reference as a parameter, and whose job appeared to be to set that reference to something. Then I looked at the rest of the parameters: if they were all const, then there was no way for that function to report an error, and the following code always simply assumed it worked. In that case, and only that case, I added an exception to the end of the function: if it did not set the value it was supposed to set, it throws. This really only amounts to three functions, but those three functions are repeated in several different code files, so the same change is made many times.

Here is the DLL, I will make a patch tomorrow for review, if this seems to work. I only tested for a few minutes, but did not experience any crashes.
Attachments
TKFillet.broaderCleanup.7z
7-zip file will uncompress to give TKFillet.dll
(557.33 KiB) Downloaded 39 times
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
kisolre
Veteran
Posts: 4163
Joined: Wed Nov 21, 2018 1:13 pm

Re: Crash on changing fillet radius

Post by kisolre »

In my Bin folder I got 2 files - TKFillet.dll - 2Mb and TKFilletd.dll - 4Mb. The file in that archive is 4 Mb which makes me wander which file should it replace?
chrisb
Veteran
Posts: 53922
Joined: Tue Mar 17, 2015 9:14 am

Re: Crash on changing fillet radius

Post by chrisb »

I am deeply impressed! Thanks for working on this.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Crash on changing fillet radius

Post by chennes »

kisolre wrote: Thu Jan 28, 2021 10:08 am In my Bin folder I got 2 files - TKFillet.dll - 2Mb and TKFilletd.dll - 4Mb. The file in that archive is 4 Mb which makes me wander which file should it replace?
Well, I compiled mine with Debug on, which is why it is bigger, but actually I used it to overwrite both, since the interface is the same. For testing purposes it should not matter as long as you get the one your copy of FC is loading.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Crash on changing fillet radius

Post by uwestoehr »

chennes wrote: Thu Jan 28, 2021 4:25 am OK, here is a second attempt.
You are a genius! I am totally amazed - no crash with fillets anymore since now an hour working on a real-life part. Only when a chamfer comes into play I get crashes, so your strategy seems to work. :D :D :D
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Crash on changing fillet radius

Post by chennes »

uwestoehr wrote: Thu Jan 28, 2021 3:50 pm no crash with fillets anymore since now an hour working on a real-life part.
I'm glad -- I think I missed one function, but it must not be a major problem. I will take a look at the chamfers, I bet it's the exact same problem.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Crash on changing fillet radius

Post by chennes »

chennes wrote: Thu Jan 28, 2021 4:00 pm I will take a look at the chamfers, I bet it's the exact same problem.
Nope, I think chamfers are really just a special case of fillets. So the bug there is something else.

I'm wrong, it really is a totally different code path.
Last edited by chennes on Thu Jan 28, 2021 9:09 pm, edited 1 time in total.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
Post Reply