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

PartDesign: Taper angle in Pad #5379

Closed
wants to merge 1 commit into from

Conversation

davidosterberg
Copy link
Contributor

@davidosterberg davidosterberg commented Jan 13, 2022

Make Pad support tapered pads with the help of BRepOffsetAPI_DraftAngle

This works for pads that are created from line segments and arcs.
It does not work for profiles that contain bspline segments. It also cannot pad profiles with arcs in other
directions than the normal direction.

  • The advantage is that the implementation is very simple.
  • TNP friendly since the face numbering is not dependent on if taper angle is 0 or not 0.
  • Produces simple faces (Planar faces and cones).
  • Inner and outer wires are seamlessly supported and behaves consistently

This PR should be seen as a complement to #5357 and #5367.
In particular this PR does not contain any UI code.

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

image

Behavior is a bit different than the implementation in #5357. In this PR there is only one Taper angle and it will apply to both directions
image

Make Pad support tapered pads with the help of BRepOffsetAPI_DraftAngle
This works for pads that are created from line segments and arcs.
It does not work for bspline segments. It also cannot pad arcs in other
directions than the normal direction.
@github-actions github-actions bot added the WB Part Design Related to the Part Design Workbench label Jan 13, 2022
@AjinkyaDahale
Copy link
Contributor

Thanks for the PR!

I wonder why we would like to do it this way rather than making a pad and then make a draft. Draft itself already uses BRepOffsetAPI_DraftAngle internally and should have the same capabilities and limitations, apart from being able to only draft some of the faces.

If it is still convenient to have a taper while making the pad, I would say it should use the pre-existing draft and auto-add all the faces in the command.

@davidosterberg
Copy link
Contributor Author

davidosterberg commented Jan 15, 2022

I wonder why we would like to do it this way rather than making a pad and then make a draft. Draft itself already uses BRepOffsetAPI_DraftAngle internally and should have the same capabilities and limitations, apart from being able to only draft some of the faces.

FeatureDraft is operating with FreeCAD face numbering. Topological naming problem makes FeatureDraft unusable for anything that will be parametric.

If it is still convenient to have a taper while making the pad, I would say it should use the pre-existing draft and auto-add all the faces in the command.

You still need the logic to identify the neutral plane, and identify which are the new faces. This is not part of FeatureDraft. This logic is most of this PR.

Edit: The discussion of the usefulness of the feature has been discussed in the forum threads for the other PRs. I created this PR because I wanted to give a concrete proposal for how the functionality could be achieved with a method that generates simple geometrical shapes.

@freecadci
Copy link

pipeline status for feature branch PR_5379. Pipeline 447916854 was triggered at f430b92. All CI branches and pipelines.

@donovaly donovaly self-assigned this Jan 16, 2022
@donovaly donovaly added the ✋ On hold This PR must not be merged before some condition is met label Jan 16, 2022
@donovaly
Copy link
Member

Thanks for the PR. However, we need to act in a planned matter. I will point this out again in the forum.
Here in short:

  • I agreed with wmayer that Part Extrude and PD Pad/Pocket should share the same code.
  • What you propose seems to work and should therefore first go into the Loft feature because there we don't have angles but have the issue of the shells being always splines.

@donovaly
Copy link
Member

Hi @davidosterberg , as I wrote here:
https://forum.freecadweb.org/viewtopic.php?p=562438#p562438

The plan is the following:

  1. finish [Part] handle inner wires for Extrude #5367

This way we have code that can handle inner wires.

  1. using the solution of this PR for Part Extrude. If an Extrude cannot be made with your drafting solution, we take the lofting existing method as fallback

Would this be fine and reasonable with you?

@davidosterberg
Copy link
Contributor Author

The plan is the following:

  1. finish [Part] handle inner wires for Extrude #5367

This way we have code that can handle inner wires.

  1. using the solution of this PR for Part Extrude. If an Extrude cannot be made with your drafting solution, we take the lofting existing method as fallback

Would this be fine and reasonable with you?

Sounds good

@donovaly
Copy link
Member

donovaly commented Feb 2, 2022

The mentioned PRs are in.

@donovaly donovaly removed the ✋ On hold This PR must not be merged before some condition is met label Feb 2, 2022
@davidosterberg
Copy link
Contributor Author

Tapered pad has been merged in master with a different implementation. Unlike the implementation in this PR, that implementation supports having two separate taper angles Forward and Reverse. Implementing this with the method here would involve a lot of additional code and it would not be elegant. Therefore I have decided to close this PR and work on other things.

@donovaly
Copy link
Member

Tapered pad has been merged in master with a different implementation.

What do you mean with different? As I always wrote, the Part Extrude feature offered tapered extrude in two directions so we cannot drop this since this will break existing documents. All I did was to use the existing Part Extrude code also for PartDesign. I did not reinvent or rewrote anything.

@davidosterberg
Copy link
Contributor Author

davidosterberg commented Feb 12, 2022

It was not meant as a complaint. Only as info. What I mean is that the method in this PR can only deal with one taper angle, not two. This is because it works by taking the prism, then making it tapered. You are right that it was known for long how the other implementation was working. But only now did I actually have time to have a look and try to integrate my code into ExtrudeHelper, I conclude that it is not worth doing. At least not to me.

@donovaly
Copy link
Member

OK. I understand. Thank you for clarification.

@donovaly
Copy link
Member

Wait, I think we are too fast. We can of course rewrite the code so that tapering is done the following way:

  • a prism is created
  • if there is a second direction, a second prism is created
  • the prism(s) are tapered using Draft
  • if there is a second direction, the two drafted prism is merged to one solid

What do you think?

Note that as it is, sketches containing splines cannot be taper-extruded anyway. so with the proposed method, I don't see a regression.

@donovaly donovaly reopened this Feb 13, 2022
@adrianinsaval
Copy link
Member

appears to work but only with negative angles
splineTaper.zip

@donovaly
Copy link
Member

donovaly commented Apr 3, 2022

Since David said he will not continue on this, closing this PR. It can be reopened if necessary or a new PR can be made.

@donovaly donovaly closed this Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Part Design Related to the Part Design Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants