Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PD] add feature to create tapered Pad / Pocket #5357

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

donovaly
Copy link
Member

@donovaly donovaly commented Jan 8, 2022

This PR adds support for tapered extrudes.

This was from time to time requested in the forum since other CAD programs and also Pad's Extrusion provides this feature.

Forum thread to discuss it: https://forum.freecadweb.org/viewtopic.php?f=17&t=65124

Here is it in action:
FreeCAD_phD8DANHBv

The PR treats Pad and Pocket the same way, so Pocket has the same feature:
FreeCAD_fzqdB1cbDr

@github-actions github-actions bot added the Mod: Part Design Related to the Part Design Workbench label Jan 8, 2022
@donovaly donovaly changed the title [PD] add option to create tapered Pad / Pocket [PD] add feature to create tapered Pad / Pocket Jan 8, 2022
@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 443915890 was triggered at 9148d80. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 443984655 was triggered at 8a85e15. All CI branches and pipelines.

@donovaly
Copy link
Member Author

donovaly commented Jan 8, 2022

@berndhahnebach, I made another commit to fix the CI build failure, can you therefore please trigger another CI run on this PR?

@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 444014761 was triggered at c59a1ba. All CI branches and pipelines.

Copy link
Contributor

@jbaehr jbaehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be some missing headers when not using _PreComp_.

src/Mod/PartDesign/App/FeatureSketchBased.cpp Outdated Show resolved Hide resolved
src/Mod/PartDesign/App/FeatureSketchBased.cpp Outdated Show resolved Hide resolved
src/Mod/PartDesign/App/FeatureSketchBased.cpp Outdated Show resolved Hide resolved
@donovaly
Copy link
Member Author

donovaly commented Jan 9, 2022

@berndhahnebach I need another CI run please
What compiler is run be the CI? Does it use precompiled headers?

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 2 times, most recently from 72f3865 to 3413c5e Compare January 9, 2022 00:59
@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 444168620 was triggered at 3413c5e. All CI branches and pipelines.

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 2 times, most recently from 7de678d to 3e8adc1 Compare January 9, 2022 14:03
@donovaly
Copy link
Member Author

donovaly commented Jan 9, 2022

@berndhahnebach give me another CI run please

I realized now that the CI does not use precompiled headers. Since I do, I don't see the compilation failures due to missing headers.

@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 444253172 was triggered at 3e8adc1. All CI branches and pipelines.

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 2 times, most recently from fdc463f to 6bcefcf Compare January 9, 2022 20:53
@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 444427600 was triggered at 6bcefcf. All CI branches and pipelines.

@wwmayer
Copy link
Contributor

wwmayer commented Jan 10, 2022

The main question to me is do we really need this feature directly for pad/pockets? To me this makes FeatureExtrude to be a so called god class that provides functions that should be better kept in a separate class.

When looking at some online available docs about commercial systems (http://catiadoc.free.fr/online/prtug_C2/prtugbt0501.htm) then there is no such option either.

This function is already part of the draft feature (http://catiadoc.free.fr/online/prtug_C2/prtugbt0612.htm) and you can achieve the very same effect.

@donovaly
Copy link
Member Author

The main question to me is do we really need this feature directly for pad/pockets?

Sure, otherwise I would not have spent so much time on this ;-).
Since I am active in FC people ask why FC lacks features so-called "CAD standard software" provides. They refer to Fusion 360:
https://help.autodesk.com/view/fusion360/ENU/?guid=SLD-EXTRUDE-SOLID
and Fusion appears indeed to be a standard in the maker scene. As I implemented it, it works almost the same as in Fusion.

In my working field Solidworks is used by most companies around and this also provides the feature:
https://help.solidworks.com/2019/english/SolidWorks/sldworks/r_extrude_propertymanager.htm
its workflow is close to the of Fusion.

You spoke about a "god" class, but I only implemented what part Extrude for years. And I could understand the users who asked why Part has a basic feature that PD does not provide.
(As you can see in Solidworks, there it is a real "god" feature because it also includes the Thickness feature.
Writing this after a full day of real-life construction work, I understand that speed matters and when you can setup a feature in one dialog and later change it in the tree in one place, saves time. I guess SW designed the UI as they did to speedup the workflow.)

Therefore implementing this was for a long time on my Todo-list. While working on it during the whole last week, I realized its power. One can save a lot of time in constructing because one saves the work to make lofts.
(Drafts are no option for me for real-life document since all the time the face numbers change and then the drafts go to the wrong faces.)
The last hour I took the freedom to use this PR on a real-life document and play around. So despite I was skeptic about such a feature for a long time (you can e.g ask chrisB who tried for a while to change my mind ;-)), I see its power now. Any having the same functionality in Part and PD also helps.

Here is the thread that eventually triggered my action: https://forum.freecadweb.org/viewtopic.php?f=3&t=64795&start=30

p.s. no fear. I am not planning to include the thickness feature to Pad/Pocket.

@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 459527778 was triggered at 7a2bb51. All CI branches and pipelines.

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 2 times, most recently from 841a38a to b671d2d Compare January 30, 2022 21:50
@donovaly donovaly removed the ✋ On hold This PR must not be merged before some condition is met label Jan 30, 2022
@donovaly
Copy link
Member Author

OK, this PR is now adapted to current master. The functionality nothing has changes, the PR inherits Part's Extrude code for tapered extrusion. However, as wished in the forum, it does now support inner structures as Part does since today.

Anyone, please give it a try.
@mwganson

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 3 times, most recently from 9e571db to c7519c4 Compare January 31, 2022 01:31
@mwganson
Copy link
Contributor

Looks good. We even get cones and planes for the faces.

Only thing I got an error message opening ExtrusionHelper.cpp:
Snip macro screenshot-5dfe96

@mwganson
Copy link
Contributor

That was opening in QtCreator.

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 2 times, most recently from fd203f3 to 7919050 Compare January 31, 2022 01:38
@donovaly
Copy link
Member Author

Only thing I got an error message opening ExtrusionHelper.cpp: Snip macro screenshot-5dfe96

Thanks. I hope this encoding issue is now fixed. Can you please try again?

@mwganson
Copy link
Contributor

That seems to have fixed it.

@donovaly
Copy link
Member Author

That seems to have fixed it.

Thanks.

I found meanwhile a minor issue, fixed this and rebased to trigger the CI.

@donovaly
Copy link
Member Author

Here is a Windows build with the PR on top of the current master to try out the PR without the need to compile:

https://github.com/donovaly/FreeCADInstProj/releases/tag/0.20r27327%2B1

@jbaehr
Copy link
Contributor

jbaehr commented Jan 31, 2022

@donovaly Thanks for your work here!

I have a general question though: From a brief look at the diffs I get the impression that the Extrude (and its private generate...Prism) methods of FeatureSketchBased are only used in Pad and Pocket. And those have their own base in between FeatureSketchBased: FeatureExtrude. Wouldn't it be cleaner to move the code there?

@donovaly
Copy link
Member Author

I have a general question though: From a brief look at the diffs I get the impression that the Extrude (and its private generate...Prism) methods of FeatureSketchBased are only used in Pad and Pocket.

It seems so. However, any further code shifting should be done in a separate PR. This PR here is already huge and the important thing is the new functionality of tapered extrudes. Therefore the focus is on that code part.

Later I plan to shift more code around, for example Part and PD have the same features to pad along a certain direction and this code can later be put into the new helper file I introduced with this PR.

@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 460324278 was triggered at 730b112. All CI branches and pipelines.

@donovaly donovaly force-pushed the PD-tapered-PadPocket branch 2 times, most recently from 2275676 to afed429 Compare February 1, 2022 02:03
This PR adds the same functionality as provided by Part Extrude.
The used code parts are sorted out to a new helper function that is used by Part and PartDesign.
@freecadci
Copy link

pipeline status for feature branch PR_5357. Pipeline 460535351 was triggered at 10375db. All CI branches and pipelines.

@donovaly
Copy link
Member Author

donovaly commented Feb 2, 2022

OK, I think it is time to make the move to merge.

@donovaly donovaly merged commit cfdf334 into FreeCAD:master Feb 2, 2022
@donovaly donovaly deleted the PD-tapered-PadPocket branch February 5, 2022 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mod: Part Design Related to the Part Design Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants