[merged] PR #5357 - add feature to create tapered Pads / Pockets

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.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
davidosterberg
Posts: 529
Joined: Fri Sep 18, 2020 5:40 pm

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by davidosterberg »

uwestoehr wrote: Sun Jan 09, 2022 4:46 pm
TheMarkster wrote: Sun Jan 09, 2022 4:39 pm By the way, if a student refuses to take an exam he still gets a failing grade
I think TheMarkster was refering to his own quote "Extrude is failing to cut", not that you are getting a failing grade on this PR :D
I think that would be a bit harsh.
TheMarkster
Veteran
Posts: 5505
Joined: Thu Apr 05, 2018 1:53 am

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by TheMarkster »

davidosterberg wrote: Sun Jan 09, 2022 4:52 pm
uwestoehr wrote: Sun Jan 09, 2022 4:46 pm
TheMarkster wrote: Sun Jan 09, 2022 4:39 pm By the way, if a student refuses to take an exam he still gets a failing grade
I think TheMarkster was refering to his own quote "Extrude is failing to cut", not that you are getting a failing grade on this PR :D
I think that would be a bit harsh.
Yes, the current Part Extrude implementation fails to handle inner wires properly, whether that's by design (student refusing to take the exam) or a bug (student took the exam, but made some errors), it still fails to handle them. I wasn't referring to the current PR at all. I'm all in favor of small steps in the right direction.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by uwestoehr »

TheMarkster wrote: Sun Jan 09, 2022 5:00 pm Yes, the current Part Extrude implementation fails to handle inner wires properly,

So let's act! I hope with a collaborative effort, we can achieve this.

I opened a new thread for this to not to hi-jack this one:
https://forum.freecadweb.org/viewtopic.php?f=10&t=65171
User avatar
jonasb
Posts: 162
Joined: Tue Dec 22, 2020 7:57 pm

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by jonasb »

TheMarkster wrote: Sun Jan 09, 2022 4:21 pm With Part workbench extrude, if you extrude a sketch with one circle inside another circle on a taper, and then use compound explode on the extrude object and cut the inner solid from the outer you can see that both wires have been tapered at the same angle.

Extrude is managing to taper both wires, it's just failing to boolean cut the inner from the outer.
That reminds me of a behaviour I observed while working on the helical extrusion of internal gears profiles for the FCGear WB... Maybe we have just an issue with the ordering of wires and their "Reversed" property?
User avatar
-alex-
Veteran
Posts: 1856
Joined: Wed Feb 13, 2019 9:42 pm
Location: France

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by -alex- »

Thanks at @uwestoehr for this great feature in PD FC master branch.
TheMarkster wrote: Sun Jan 09, 2022 3:47 pm I'm just tossing out ideas. How about an enumeration of different taper types? Some options could be:

Taper outer wire only, inner wires not extruded.
Taper all wires at taper angle.
Taper all wires, but inner wires are inverted. Example: outer wire is tapered 2 degrees, inner wires -2 degrees.
Taper outer wire only, inner wires extruded without taper.
Taper inner wires only, outer wire extruded without taper.
AFAIK such options already exist in Linkstage3 branch of @realthunder, very convenient BTW.
Unfortunately such taper options change the nature of edges and faces (planar/linear -> bsplines), and IMO that not so good.
I hope this PR dosen't modifiy the nature of elements when taper is enabled.
BTW @realthunder has started to improve this behavior in his branch, see here
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by adrianinsaval »

uwestoehr wrote: Sun Jan 09, 2022 4:14 pm For ages the Extrude feature behaves as it does. Now I just transferred tits functionality to Pad/Pocket. So I add a feature, all existing functionalities are preserved.
having an unsatisfactory behavior in Part for ages doesn't really justify putting it in as is in PD (your argument here sounds like this: it's been done poorly before so I can do it again). Note that I'm very grateful that you are taking the time to do this feature and I think it's a very good feature to have.
So I provide here a feature that would help people in many situations but you say, better not add this because it does in the first step not cover all possible cases?
Actually I would say yes (to a degree), better to get it into a better condition before merging it to master, if incomplete features gets into stable then changing the behavior in the next version can be problematic. IMO silently ignoring inner wires entirely is an awful UX so it shouldn't be merged until it does something with them. And please don't take feedback about the feature as a personal attack, I think most of us have nothing against you, it's just that the UX for this sounds terrible.
Also, inviting @realthunder to the discussion since he added a similar feature into his branch
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by uwestoehr »

adrianinsaval wrote: Sun Jan 09, 2022 11:08 pm having an unsatisfactory behavior in Part for ages
If it is that unsatisfactory, why was this never changed?

The PR here improved Pad/Pocket for many cases and does not have drawbacks to existing features. This is my understanding of progress. As it is you need for every drafted pad a loft, meaning two sketches, adjust them etc. Compared to the tapered feature this costs a lot of time. So because that tapered does not work yet for nested structures, you don't want to have it at all despite most of the uses cases don't have nested structures?
That does not make much sense because for use cases with nested structured, the PR does not change anything. You can keep your workflow at it already is, all others benefit from a much faster workflow.
And when you read this thread, you noticed that support for nested structures is in development but this will affect 2 workbenches and requires some deep code work. Please join the team to code this.

So because let's say 10% of the taper uses cases wont have a benefit by this PR, 90% does. For the 10% nothing changes. I think the benefit is clear.
As I stated already, support for nested structures won't become part of this PR. This should for good reasons done in 2 further steps, one for Part, one for PartDesign.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by adrianinsaval »

uwestoehr wrote: Sun Jan 09, 2022 11:25 pm If it is that unsatisfactory, why was this never changed?
do you really need to ask this? How many hundreds of unsatisfactory UX/UI or features we have that have been left there for ages? Reason is simple, nobody stepped up to change it. How often do we even see people working to improve Part wb? I personally would rather work on PD and even the stuff I found unsatisfactory there have turned out hard for me to successfully improve (I've been sitting for more than a year on incomplete and now conflicting code to get variable radius fillets on PD in a satisfactory state)
As it is you need for every drafted pad a loft, meaning two sketches, adjust them etc.
unrelated to the current discussion really but: wouldn't it be enough to make a pad an then a draft feature? not half as complicated I think.
And when you read this thread, you noticed that support for nested structures is in development but this will affect 2 workbenches and requires some deep code work.
and as I said, I'm very grateful for this, my deepest thanks to you for all the work you are doing as I'm seeing improvements all the time while lurking here.
Please join the team to code this.
Oh I would love to, Iv'e been dying to get coding for months now but I'm so chronically short on time right now (at least with codingworthy brain capacity if you understand me)
This should for good reasons done in 2 further steps, one for Part, one for PartDesign.
Wouldn't then a better approach be to improve in Part first then you can copy paste it as is into PD? I'm not an absolutist so I wouldn't really complain if you merge this into master but understand that silently ignoring wires is terrible UX, at the bare minimum there should be a warning of not supporting inner wires when the user tries to pad a profile that has them. (forgive me if this is already the case since right now I'm just running off of what has been commented here).
On the other hand isn't the purpose of making PRs to get feedback and polish a feature before merging? Why do the improvements that may come from the feedback need to come in a second PR? What happens if the version that doesn't support inner wires is released as stable and then we have a version that deals with inner wires but an unnecessary mode that ignores inner wires would have to be kept for backwards compatibility. (I know this is not critical but why go looking for that kind of trouble?)
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by uwestoehr »

For information, this is when inner wires are supported:
https://forum.freecadweb.org/viewtopic. ... 22#p560422

So when this comes, only positive angles will/can be supported.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: PR #5357 - add feature to create tapered Pads / Pockets

Post by realthunder »

uwestoehr wrote: Mon Jan 10, 2022 3:04 am For information, this is when inner wires are supported:
https://forum.freecadweb.org/viewtopic. ... 22#p560422

So when this comes, only positive angles will/can be supported.
I have actually implemented this taper feature with support of inner wires on both Part and PartDesign. The code is in Part::Extrusion::makeDraft(). I simply split the face wires into outer and inner ones, perform recursive draft on all of them, and then obtain the final shape through cutting. The wire split code is here. The user can set different taper angle on outer shape and inner holes.

Regarding the BSpline edge/face generated by the loft operation, there is an option to use pipe (i.e. sweep) operation instead, which seem to work better in some cases, but unfortuantely fail in some others. So I added it as an option for the user to decide. Another attempt is to explicitly 'linearize' those straight edges and planar faces. I got the edge part working, but not the face, although it seems to be enough in many use cases, such as the sheet workbench. The linearize code is here, which also calls Geometry::toLine() and Geometry::toPlane() to convert the underlying geometries. 'toPlane()' works, but TopoShape::linearize() failed to replace the bspline face with the converted planar one.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
Post Reply