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

[davidosterberg]

This PR adds possibility to make true threaded holes with the Hole feature (possible to e.g 3D print).

Forum discussion:
https://forum.freecadweb.org/viewtopic.php?f=34&t=54240

It should work for all supported thread types

Todo

• Give the user possibility to tweak the clearance of the thread (important for 3D printing)
• Turn off auto-recompute when Model Thread is active. It is too slow if there are large hole patterns.
• Rename "Model Actual Thread" to simply "Model Thread" in the Ui
• Default threading depth to 3.5 x P from the hole bottom.
• Add UI support for specifying thread depth
• Add backend support for specifying thread depth
• Remove column 2 from hreadClass_ISOmetric_data
• Fix donovaly bug A
• Fix donovaly bug B
• Fix donovaly bug C
• Wait for PR [PD] improve blind hole depth handling #4134

[donovaly]

We just had the discussion few weeks ago if the hole dialog should have an option to get real threads. It is good to see it just to be done, however, we must be careful:

• the option should not be on by default since it consumes calculation bandwidth.
• many users say it is not important to have real threads but when we have it, then norm-conform.

A bit off-topic: What we need is that TechDraw recognizes threaded holes as such. However, I insist that we should now come to an end with FC 0.19 instead of continuing with new features so I won't work on that in TD for now.

[donovaly]

Some remarks after a first look

[donovaly]

A general note: before this PR would committed, the pending PR #4134 should go in as basis for the hole dialog that is further changed by this PR.

[donovaly]

Sorry that I insist on more documenting:

[davidosterberg]

I added a new feature:

1. Rename "Model Actual Thread" -> "Model Thread" (Thanks @hyarion for the suggestion)
2. Add a Update view button that becomes visible when Model Thread is active. Defaults to false. (this is because the UI becomes quite unresponsive when Model Thread is active.

[donovaly]

I have further remarks:

• the dialog is too wide for smaller screens:
  
  There is now a lot of whitespace (the red area)
• there are missing tooltips
• the additional clearance is far off the thread parameters (green arrow)
• the dialog does not follow our guidelines that elements should not be hidden but only disabled (blue area)
• the additional clearance is also enabled if the hole is not threaded

Except of the last point, this dialog would fix the issues:
TaskHoleParameters.zip
Here it its look:

[donovaly]

Another issue: what is the additional clearance about (what's the usecase)? I see that you allow negative value but what screw should then fit? So shouldn't it be restricted to positive values?

[donovaly]

I tested the feature and there are some bugs that needs to be fixed:

A:

1. model a thread
2. enable the Update view
   this takes e.g. 2 seconds
3. close the Hole dialog with OK
   -> FreeCAD becomes unresponsible for about 10 seconds.
   But since the thread was already modeled it is unclear why FC needs now 5 times as long and why it becomes unresponsive.

B:

1. model a thread
2. set update view
3. add additional clearance
4. close hole dialog with OK
5. reopen the hole dialog
   -> the status of the additional clearance and the Update view is lost, since there is now no additional clearance at all it is even a dataloss

C:

1. model a thread
2. close hole dialog with OK
3. reopen the hole dialog
4. uncheck to model the thread
5. close hole dialog with OK
   -> the thread is still modeled

[davidosterberg]

Another issue: what is the additional clearance about (what's the usecase)? I see that you allow negative value but what screw should then fit? So shouldn't it be restricted to positive values?

The main use case is 3D printing threads. 3D printers are not precise enough to respect the Standard thread class tolerance limits. Therefore additional clearance is needed. It could be restricted to positive values without loosing much, and it would perhaps relieve the user from having to worry about the sign convention used.

[davidosterberg]

Thanks for the Dialog, I have been working on something similar, with the Thread Model options grouped together. I did not know about the hiding guideline. I think your dialog looks pretty good. Also I will add the thread depth, as I think it is a necessary parameter. My thought was to add it just below the Hole depth, and change the label "Depth" to "Hole depth".

Also thanks for the bug reports. Very valuable. Some of them are fixed already in in my offline branch, and some I was unaware of. The performance is certainly an issue. One that I am not certain of how to attack. But I have not looked at how the feature is doing the pattern of the prototype negative hole. I have only added my code to the "prototype hole". I suspect that it is the pattern and the cut that takes time.

[donovaly]

Another issue: what is the additional clearance about (what's the usecase)?
The main use case is 3D printing threads.

OK, then the value must not be restricted. I have resins leading to smaller holes than designed while other resins lead to wider ones.

[donovaly]

Thanks for the Dialog, I have been working on something similar, with the Thread Model options grouped together.

I am known as the (annoying ;-) ) "dialog guy" because I made so many PRs for dialogs. The point is that FC gets a wider audience and things like touch-screen handling but also smaller screens become more popular. I use smaller screens myself and it annoyed me that the dialogs were often not as compact as they can be. Width was for most an issue and when several dialogs are stacked upon each other also height.
However, just a general: don't change things in the dialog that don't belong to your changes (except your PR is to change the dialog). I see that you moved the Reversed option despite this is independent on threads.

Also I will add the thread depth, as I think it is a necessary parameter. My thought was to add it just below the Hole depth, and change the label "Depth" to "Hole depth".

Then a groupbox would make sense. A groupbox can be made clickable and this would then made a hole threaded and activate all settings inside the groupbox.

So if you like my dialog, take it as basis and put in the thread depth setting. I would then volunteer :-) to make a proposal for a compacts dialog design using groupboxes.

[davidosterberg]

Having focus on the user experience is not being annoying. I agree fully, and I am well aware that the UX/UI is the weakest part of this PR so far. That is why the linked forum thread is posted in the UX/UI subforum. I will work on incorporating your requests.

I do think that the dialog could use a general overhaul. For me the central choice of the dialog should be Threaded hole or Clearance hole. I also think it could be more compact. I would welcome a PR from you on this.

[vosk]

I do think that the dialog could use a general overhaul. For me the central choice of the dialog should be Threaded hole or Clearance hole. I also think it could be more compact. I would welcome a PR from you on this.

Please make sure that the use of clearance hole does not violate the engineering fit nomenclature. Clearance hole, implies a hole with a clearance fit.
https://en.wikipedia.org/wiki/Engineering_fit

[vosk]

Bored hole and tapped hole are well established unofficial jargon

[davidosterberg]

@vosk, my point is that Threaded = unchecked, does not mean bored hole, with suitable clearance fit to a certain bolt size. But this is what the dialog currently assumes that the user understands.

[davidosterberg]

However, just a general: don't change things in the dialog that don't belong to your changes (except your PR is to change the dialog). I see that you moved the Reversed option despite this is independent on threads.

So if you like my dialog, take it as basis and put in the thread depth setting. I would then volunteer :-) to make a proposal for a compacts dialog design using groupboxes.

I have adapted your dialog. There are 3 issues:

Note three issues

1. There is a bug where the whole diameter is not visible. This is the same in the AppImage so it is not me who have messed up the dialog.
2. There is a lot of whitespace in the middle
3. The XML ordering has been randomized in your version. It used to be that the items were (almost) ordered by the 'row' attribute. Now the order is random. THis will make the diff unreadable.

For reference this is the dialog in the AppImage, that suffers from the same problems.

OS: Ubuntu 20.10 (KDE/plasma)
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.23463 (Git) AppImage
Build type: Release
Branch: master
Hash: adc6db8
Python version: 3.8.6
Qt version: 5.12.5
Coin version: 4.0.0
OCC version: 7.4.0
Locale: English/United States (en_US)

[donovaly]

I have adapted your dialog. There are 3 issues:

Thanks.
However, we drift a bit away. Let's focus in this PR to get the threads modeled properly without bugs. The dialog fine-tuning can be done later. I would for now also skip the additional clearance feature (that should maybe better names "additional thread diameter"
In a second step the thread length and additional clearance can be added. Then it makes sense to think about the dialog layout in detail.

1. There is a bug where the whole diameter is not visible. This is the same in the AppImage so it is not me who have messed up the dialog.
2. There is a lot of whitespace in the middle

2. causes 1. I would need to look how you managed to do this. As you can see in my screenshot, my proposal does not have these issues. However, as I wrote, let's better now only work on the actual new feature - modeled threads and skip extra features and UI tuning. When you now only add the 2 options to model and to update view, life becomes much easier for this PR.

3. The XML ordering has been randomized in your version.

Yes, this is done by Qt Designer and cannot be avoided. However, the order has no effect so it can be ignored.
What is however important is the tab order. This needs of course be adapted since you added new elements. Interestingly The tab order also affects how Qt Designer orders the XML code, despite as I said the order is irrelevant. But also then you will often not get a human-readable diff.
And I see that you use Qt 5.12 while I use the Qt Designer from version 5.15.1 this can also make a difference in the XML order.

[donovaly]

For reference this is the dialog in the AppImage, that suffers from the same problems.

What do you mean with "in the AppImage"?

For reference this is how the hole dialog looks in current master:

OS: Windows 10 Version 2009
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.23707 (Git)
Build type: Release
Branch: master
Hash: 919196f
Python version: 3.8.6+
Qt version: 5.15.1
Coin version: 4.0.1
OCC version: 7.5.0
Locale: German/Germany (de_DE)

[davidosterberg]

Ok, I have fixed @donovaly bugs 7 and 8. Personally I feel this PR can be merged now. For me everything works ok. The discussion in this PR is too long and is starting to deviate into issues with TechDraw etc Those issues are best fixed in separate PRs. @abdullahtahiriyo what do you think?

[donovaly]

Ok, I have fixed @donovaly bugs 7 and 8. Personally I feel this PR can be merged now. For me everything works ok.

Thank you. I would like to test your latest changes but can first do this the night to Monday.

[wwmayer]

Is there a relation between the two property sets (ThreadAngle, ThreadCutOffInner, ThreadCutOffOuter) and (ThreadDepth, CustomThreadClearance)? If yes then it would be good to implement a conversion from the old to the new properties.

And it should also handle ModelActualThread -> ModelThread

[davidosterberg]

@wwmayer , the discussion is very long so I do not expect that you have read all the previous comments. The question about the removed properties was discussed above. This was my rationale for removing them without any conversion:

The removed and renamed properties were not used before, they were clearly inteded for the model thread feature, that was not implemented. Therefore no existing model could reasonably rely on them. I have tested opening an older file that contained a hole feature, and FreeCAD happily opens it (ignoring the properties that it does not understand).

I therefore concluded that there is no legacy to consider, so I took the opportunity to remove these old properties, as they otherwise will be confusing for the user looking at the properties view.

By the way the mentioned properties are not related to the ThreadDepth. But they are related to the thread profile.
I you disagree with this reasoning, I will implement the conversion.

[wwmayer]

@wwmayer , the discussion is very long so I do not expect that you have read all the previous comments.

No, I didn't. More than 140 comments is a bit too much to go through :)

The removed and renamed properties were not used before, they were clearly intended for the model thread feature...

OK, this is fine for me.

I you disagree with this reasoning, I will implement the conversion.

No, not needed. I will have a look at this PR again later today and if works I will merge it.

Hint for the future: if you ever have to support older projects because of changed properties then have a look at the methods:

• handleChangedPropertyName
• handleChangedPropertyType

In the current code base you will find examples how to implement them.

[luzpaz]

Hint for the future: if you ever have to support older projects because of changed properties then have a look at the methods:

* handleChangedPropertyName
* handleChangedPropertyType

If we were to document this? Where would we do so?

[abdullahtahiriyo]

@davidosterberg

I did not check the new changes, but the old code was codewise fine for me.

As wmayer will review it, he will let you know if he requires any change.

Sorry, it took more time than you expected.

[abdullahtahiriyo]

@luzpaz

There are several examples in the codebase where that mechanism is used (it is not something "new"). Sure it could go somewhere in the wiki.

Basically it is used when a property gets renamed or the type of a property changes. In those cases, when an older file is read, the old property is read and the data migrated to the new property in those functions. This way, even if the property changed name or type, backwards compatibility is provided for those files. Of course, if afterwards the file is saved, it will be saved with the new property (name and type), so there is no forward compatibility with versions not knowing about the new property.

In this case, it is not necessary to do this because those "older" properties were present, but not used at all. There is no expectation from a user of opening an "older" file and getting a specific behaviour.

[donovaly]

No, I didn't. More than 140 comments is a bit too much to go through :)

What? And I thought that would be a lot of fun. And now I am sad we did not make it to 150 ;-)

[donovaly]

@davidosterberg I created now the Release note for the next version: https://wiki.freecadweb.org/Release_notes_0.20 so that you can add the new thread feature there

[lual]

Thanks a lot for implementing this! This is a must have for a CAD program.
I just installed Freecad Version: 0.20.26683 only for using this feature. I found a small problem. I'm not sure where to post that. (Sorry if I'm wrong here).

Please see the pictures...

But there is a Bug with Thread depth: Dimension

[adrianinsaval]

@lual better to post in the help forum following the guidelines in the red banner there, and better if you post screenshots of the software in english so it's easier to understand for everyone

[donovaly]

I'm not sure where to post that. (Sorry if I'm wrong here).

As already written, please report this to the forum. But please post the link to your forum thread here too.
In your forum post please attach your example file there.

I think we can fix the issue quickly.

[lual]

thx, here the link to the forum-post

[acominelli]

I tried the new tool to make threaded holes and it is a great addition!
I think it would be great to add a way to model custom threads (custom diameter and custom pitch, similarly to what Fastners Workbech does).
In alternative, it would be useful if you could create custom thread models to be added to standard ISO models predefined in the "size" selection

[donovaly]

Thanks. Can you please open a new issue for this feature request. We will try to implement this for FreeCAD 1.0.