[PD] improve blind hole depth handling #4134

donovaly · 2020-12-13T05:21:02Z

as reported in https://tracker.freecadweb.org/view.php?id=3818
we treat the blind hole depth not according to the conventions. The size of the drill point due to the angle is normally not taken into account but FC does this in every case.

There are of course cases where the total depth, including the drill point size matters.

This PR adds therefore an option, that is by default off, to take the drill point size into account.
Without the option, (the new default) the depth is nor calculated according to the conventions.

I purposely made the new option by default off, because all literature about technical drawings don't take the drill point size into account and the hole dialog should by default offer the normed / conventioned behavior.

As side effect that means that existing blind holes change, but the same applied for my last hole dialog fixed (diameters of existing holes changed). So I think it is OK. If you disagree, please tell. However, I don't see how this can be avoided.

Forum thread: https://forum.freecadweb.org/viewtopic.php?t=51491&view=unread#p457068

chennes · 2020-12-14T04:30:33Z

When I tried to test this I got a crash due to a debug assertion failure in OCC. The steps I followed were:

Create a rectangle in a sketch
Pad the rectangle 50mm
Sketch a circle on that pad's upper surface
Activate the hole task
Check the new option to "Take into account for depth" (without changing anything else).

Error message is:
"Error: a duplicated modification of a shape. (Condition: "!aModifications->Contains(theModified)")"

Backtrace:

 	TKBRepd.dll!00007ffbbe5fa660()	Unknown
 	TKBOd.dll!00007ffbbf98f2a1()	Unknown
 	TKBOd.dll!00007ffbbf965374()	Unknown
 	TKBOd.dll!00007ffbbf9723b3()	Unknown
 	TKBOd.dll!00007ffbbf970751()	Unknown
 	TKBOd.dll!00007ffbbf910d42()	Unknown
 	TKBOd.dll!00007ffbbf90cc1e()	Unknown
 	TKBOd.dll!00007ffbbf914edc()	Unknown
>	_PartDesign_d.pyd!PartDesign::Hole::execute() Line 1518	C++
 	FreeCADApp_d.dll!App::DocumentObject::recompute() Line 111	C++
 	Part_d.pyd!Part::Feature::recompute() Line 101	C++
 	FreeCADApp_d.dll!App::Document::_recomputeFeature(App::DocumentObject * Feat) Line 3744	C++
 	FreeCADApp_d.dll!App::Document::recomputeFeature(App::DocumentObject * Feat, bool recursive) Line 3804	C++
 	PartDesignGui_d.pyd!PartDesignGui::TaskFeatureParameters::recomputeFeature() Line 74	C++
 	PartDesignGui_d.pyd!PartDesignGui::TaskHoleParameters::drillForDepthChanged() Line 387	C++
 	PartDesignGui_d.pyd!PartDesignGui::TaskHoleParameters::qt_static_metacall(QObject * _o, QMetaObject::Call _c, int _id, void * * _a) Line 176	C++
 	[External Code]	
 	FreeCADGui_d.dll!Gui::GUIApplication::notify(QObject * receiver, QEvent * event) Line 92	C++
 	[External Code]	
 	FreeCADGui_d.dll!Gui::GUIApplication::notify(QObject * receiver, QEvent * event) Line 92	C++
 	[External Code]	
 	FreeCADGui_d.dll!Gui::Application::runApplication() Line 2278	C++
 	FreeCAD_d.exe!main(int argc, char * * argv) Line 302	C++
 	FreeCAD_d.exe!WinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, char * __formal, int __formal) Line 97	C++
 	[External Code]

wwmayer · 2020-12-14T11:38:14Z

I cannot confirm the behaviour you observed but I realized that there are several locations where the doubles x and z are declared and used for further calculations but they are never correctly initialized so behaviour is undefined.

These are the lines 1269, 1298 and 1349.

src/Mod/PartDesign/App/FeatureHole.cpp

donovaly · 2020-12-14T23:27:12Z

there are several locations where the doubles x and z are declared and used for further calculations but they are never correctly initialized so behaviour is undefined.

Yes, and since their meaningless name it is even hard to follow what the code does. I now renamed the variables and initialized them.

I also improved the code a little bit here. But the whole hole code could benefit from some love. However, not for now.

donovaly · 2020-12-14T23:41:43Z

When I tried to test this I got a crash due to a debug assertion failure in OCC. The steps I followed were:

I could not reproduce this with a debug build. But maybe it was because of the uninitialized variables. Can you please check again?

chennes · 2020-12-15T00:19:33Z

When I tried to test this I got a crash due to a debug assertion failure in OCC. The steps I followed were:

I could not reproduce this with a debug build. But maybe it was because of the uninitialized variables. Can you please check again?

It seems that it was being caused by the weird geometry that was generated when the tip doubled back on itself (due to those duplicated lines 1389 through 1398) -- a3bcf88 should do the trick, but I will re-test shortly.

donovaly · 2021-01-02T20:37:47Z

Can this PR go in? I fixed the 2 issues spotted by @chennes . I see that the PR is necessary since it popped up again in the forum: https://forum.freecadweb.org/viewtopic.php?t=51491&view=unread#p462138

chennes

I just re-tested and the bug I reported is resolved. It works well and is a good addition, thanks!

donovaly · 2021-01-15T22:22:13Z

Can this PR go in?

Ping.

@chennes tested it and we have another PR (#4274) that waits for it. (Waits for this PR because it will change the Hole dialog further.)

src/Mod/PartDesign/Gui/TaskHoleParameters.ui

davidosterberg

I have not tested it, but the code looks good. I request that the UI file is sorted, and the PR rebased. Otherwise this looks ready (considering @chennes approval).

as reported in https://tracker.freecadweb.org/view.php?id=3818 we treat the blind hole depth not according to the conventions. The size of the drill point due to the angle is normally not taken into account but FC does this in any case. This PR adds therefore an option, that is by default off, to take the drill point size into account. Without the option, (the new default) the depth is calculated according to the conventions. The PR also removes unused widgets and restored the tab order in the .ui file. The thread parameters were never used. In case we made in future the decision to carve into holes real (modeled) threads, we need a special UI for that solution anyway so having the dead code in is not helpful at all.

donovaly · 2021-01-25T00:15:57Z

I request that the UI file is sorted, and the PR rebased. Otherwise this looks ready (considering @chennes approval).

Done, thanks for having a look.

davidosterberg

Looks good to me

donovaly · 2021-01-30T18:49:57Z

Many thanks for merging. We can now proceed to finish the other 2 pending hole PRs.

chennes reviewed Dec 14, 2020

View reviewed changes

src/Mod/PartDesign/App/FeatureHole.cpp Outdated Show resolved Hide resolved

chennes reviewed Dec 14, 2020

View reviewed changes

src/Mod/PartDesign/App/FeatureHole.cpp Outdated Show resolved Hide resolved

chennes reviewed Dec 14, 2020

View reviewed changes

src/Mod/PartDesign/App/FeatureHole.cpp Outdated Show resolved Hide resolved

donovaly force-pushed the PD-Hole-DrillPoint branch from fa8703f to a3bcf88 Compare December 14, 2020 23:17

donovaly force-pushed the PD-Hole-DrillPoint branch 3 times, most recently from 20e67ee to 1ffc7f5 Compare January 2, 2021 21:39

chennes approved these changes Jan 2, 2021

View reviewed changes

donovaly force-pushed the PD-Hole-DrillPoint branch from 8d91c14 to 6216951 Compare January 2, 2021 22:16

donovaly mentioned this pull request Jan 12, 2021

[0.20] PartDesign: Add true threads to the Hole feature #4274

Merged

11 tasks

davidosterberg reviewed Jan 24, 2021

View reviewed changes

src/Mod/PartDesign/Gui/TaskHoleParameters.ui Outdated Show resolved Hide resolved

davidosterberg suggested changes Jan 24, 2021

View reviewed changes

donovaly force-pushed the PD-Hole-DrillPoint branch from a3276df to 5535ded Compare January 25, 2021 00:08

donovaly force-pushed the PD-Hole-DrillPoint branch from 5535ded to 7306ed2 Compare January 25, 2021 00:10

donovaly mentioned this pull request Jan 25, 2021

[PD] fix several hole bugs #4281

Closed

davidosterberg approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into PD-Hole-DrillPoint

046aa8a

kkremitzki merged commit 497ecbe into FreeCAD:master Jan 30, 2021

donovaly deleted the PD-Hole-DrillPoint branch January 31, 2021 00:42

FreeCAD-Bug-Importer mentioned this pull request Feb 7, 2022

PartDesign Hole Depth Dimension should provide the option not to take the angled drill point type into account #5879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PD] improve blind hole depth handling #4134

[PD] improve blind hole depth handling #4134

donovaly commented Dec 13, 2020 •

edited

Loading

chennes commented Dec 14, 2020 •

edited

Loading

wwmayer commented Dec 14, 2020

donovaly commented Dec 14, 2020

donovaly commented Dec 14, 2020

chennes commented Dec 15, 2020

donovaly commented Jan 2, 2021

chennes left a comment

donovaly commented Jan 15, 2021

davidosterberg left a comment •

edited

Loading

donovaly commented Jan 25, 2021

davidosterberg left a comment

donovaly commented Jan 30, 2021

[PD] improve blind hole depth handling #4134

[PD] improve blind hole depth handling #4134

Conversation

donovaly commented Dec 13, 2020 • edited Loading

chennes commented Dec 14, 2020 • edited Loading

wwmayer commented Dec 14, 2020

donovaly commented Dec 14, 2020

donovaly commented Dec 14, 2020

chennes commented Dec 15, 2020

donovaly commented Jan 2, 2021

chennes left a comment

Choose a reason for hiding this comment

donovaly commented Jan 15, 2021

davidosterberg left a comment • edited Loading

Choose a reason for hiding this comment

donovaly commented Jan 25, 2021

davidosterberg left a comment

Choose a reason for hiding this comment

donovaly commented Jan 30, 2021

donovaly commented Dec 13, 2020 •

edited

Loading

chennes commented Dec 14, 2020 •

edited

Loading

davidosterberg left a comment •

edited

Loading