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] Support for point sections in loft #5170

Merged
merged 3 commits into from
Nov 22, 2021

Conversation

AjinkyaDahale
Copy link
Contributor

@AjinkyaDahale AjinkyaDahale commented Nov 12, 2021

The BRepOffsetAPI_ThruSections method that forms the crux of loft supports the first and the last sections to be vertices instead of wires.

Addresses issue#4607.

@donovaly
Copy link
Member

What is the status of this PR?

Concerning the feature, it should support to use vertices of 3D objects as well as sketches just consisting of a point object but not datum points.
The sketch point support might be tricky since I remember a discussion once I proposed to change the Hole feature so that one only has to provide a point. At the moment one has to produce a circle, but only the center pint of it is used as position of the hole).
So if it is possible to have only a sketcher point, I would try if I can change the Hole feature according to your solution for lofts.
(Maybe this is not that tricky, I simply never looked into this yet.)

However, I could not resist to merge already the 2 typos you corrected in the PR.

@AjinkyaDahale
Copy link
Contributor Author

AjinkyaDahale commented Nov 13, 2021

What is the status of this PR?

I answered some questions in #5168 (comment). In summary, we can use vertices of the solid object as the last section for a loft, and I'm working on trying to support solid vertices for start and later sketch vertices.

...The sketch point support might be tricky since I remember a discussion once I proposed to change the Hole feature so that one only has to provide a point. At the moment one has to produce a circle, but only the center pint of it is used as position of the hole). So if it is possible to have only a sketcher point, I would try if I can change the Hole feature according to your solution for lofts. (Maybe this is not that tricky, I simply never looked into this yet.)

If anything this support forces us to address supporting vertex-only sketches, or only vertices from sketches, because for loft (and pipe) vertices and circles mean different things.

However, I could not resist to merge already the 2 typos you corrected in the PR.

That's perfectly fine :)

@AjinkyaDahale
Copy link
Contributor Author

Started a forum thread to keep the PR thread clean: https://forum.freecadweb.org/viewtopic.php?f=19&t=63697. @donovaly @adrianinsaval perhaps we should move the discussion there.

@AjinkyaDahale AjinkyaDahale marked this pull request as ready for review November 13, 2021 21:05
@AjinkyaDahale
Copy link
Contributor Author

@donovaly @adrianinsaval there are a few things remaining to be done, but overall I would call this ready for review.

@freecadci
Copy link

pipeline status for feature branch PR_5170. Pipeline 408635926 was triggered at 08e6c44. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_5170. Pipeline 409255305 was triggered at 557193c. All CI branches and pipelines.

@AjinkyaDahale AjinkyaDahale force-pushed the pd-loft-support-point branch 2 times, most recently from c34b682 to 5d1e4d9 Compare November 16, 2021 09:58
@yorikvanhavre yorikvanhavre added the Mod: Part Design Related to the Part Design Workbench label Nov 16, 2021
@AjinkyaDahale AjinkyaDahale changed the title [PD] [do not merge] Support for point sections in loft [PD] Support for point sections in loft Nov 16, 2021
@AjinkyaDahale AjinkyaDahale force-pushed the pd-loft-support-point branch 2 times, most recently from 58c4a62 to f9a50d8 Compare November 18, 2021 01:54
@freecadci
Copy link

pipeline status for feature branch PR_5170. Pipeline 411472361 was triggered at f9a50d8. All CI branches and pipelines.

@AjinkyaDahale AjinkyaDahale changed the title [PD] Support for point sections in loft [PD] [do not merge] Support for point sections in loft Nov 21, 2021
@AjinkyaDahale
Copy link
Contributor Author

AjinkyaDahale commented Nov 21, 2021

Adding back the "do not merge" tag, because after rebasing with PR #5176 merged, the "front" face is not built.

@AjinkyaDahale
Copy link
Contributor Author

Adding back the "do not merge" tag, because after rebasing with PR #5176 merged, the "front" face is not built.

This bug should now be fixed.

@AjinkyaDahale AjinkyaDahale changed the title [PD] [do not merge] Support for point sections in loft [PD] Support for point sections in loft Nov 21, 2021
This is a combination of 4 commits. Original commit messages follow.

[PD] Initial support for point sections in loft

This commit allows the last section in a loft to be a single vertex of a solid.
Currently single vertices of sketches or datum points are NOT supported.

[PD] Allow loft "profiles" to be points

Most reliably done in dialog-based workflow.

[PD] Allow loft last section to be sketch point

[PD] Refactor `Loft::execute`

Makes it easier to support adding a single-vertex sketch in profile or sections
field when selecting the sketch in tree (i.e. without selecting subelements).

[PD] Refactoring after PR FreeCAD#5176 is merged
@freecadci
Copy link

pipeline status for feature branch PR_5170. Pipeline 413453485 was triggered at f8da88c. All CI branches and pipelines.

Copy link
Member

@donovaly donovaly left a comment

Choose a reason for hiding this comment

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

Excellent work.
I stress-tested the feature and it works nicely. I have only minor comments/clarifications.

src/Mod/PartDesign/App/FeatureLoft.cpp Outdated Show resolved Hide resolved
src/Mod/PartDesign/App/FeatureLoft.cpp Show resolved Hide resolved
src/Mod/PartDesign/Gui/Command.cpp Show resolved Hide resolved
src/Mod/PartDesign/Gui/TaskLoftParameters.cpp Show resolved Hide resolved
@donovaly
Copy link
Member

Many thanks for this PR. This improves the loft feature a lot since pyramids are not that rare (own experience).

Please have a look at my very minor comments. I will merge it then and then you can continue to bring Sweep/Pipe to the same functionality ;-)

(By the way, after sweep/pipe can handle vertices as well, I propose to make a PR to uniform the naming. At the moment the tern "sweep" is mixed with "pipe" in the code. We should only use one term and "pipe" is for non-native speakers like me even misleading.)

@donovaly
Copy link
Member

I now found a test case that fails:

FreeCAD_MdqpV6Szrx

Here is the test file:
loftbug.zip

So the loft is nicely created but then not visible. Maybe this is because a body cannot contain of several volume shapes and in my test case they form one but the most thin point is a vertex?
However, this would not be a stopper for the PR.

@AjinkyaDahale
Copy link
Contributor Author

I now found a test case that fails:
...
So the loft is nicely created but then not visible. Maybe this is because a body cannot contain of several volume shapes and in my test case they form one but the most thin point is a vertex? However, this would not be a stopper for the PR.

That is probably exactly what's happening. Could you try deleting the box and using a sketch instead?

@donovaly
Copy link
Member

That is probably exactly what's happening. Could you try deleting the box and using a sketch instead?

This works, here an example where the begin and end is a single-point sketch:
FreeCAD_Z23ccfTCcM

However, what I wanted to test is if a vertex from a surface could be taken. Therefore i need an object like the box.

@AjinkyaDahale
Copy link
Contributor Author

AjinkyaDahale commented Nov 22, 2021

However, what I wanted to test is if a vertex from a surface could be taken. Therefore i need an object like the box.

Try one of the vertices on the left face. There should be some 2D (at least) intersection with the solid, or so I would imagine.

@donovaly
Copy link
Member

Merging now, since the comments and wording changes are in.

The issue I mentioned in #5170 (comment)
is due to the fact that PD cannot have two 3D objects within one body yet. This is something @realthunder has in his branch, so in future this would be possible. For now, it is sufficient to mention the special case in the Wiki.

@donovaly donovaly merged commit ab26d4d into FreeCAD:master Nov 22, 2021
AjinkyaDahale added a commit to AjinkyaDahale/FreeCAD that referenced this pull request Nov 27, 2021
Similar to PR FreeCAD#5170 for loft. This commit squashes the following commits.

[PD] Refactor `Pipe::execute` and support point sections

[PD] Allow point profile in selection-based pipe workflow

[PD] Only add sketch subs if it is vertex

[PD] Make both end faces of pipe regardless of point sections

Earlier we were checking if these faces correspond to point sections, but
apparently the end faces are independent of the order in which the sections are
added, so the "front" may be the face closest to the last added section, rather
than the "profile". Creating null faces and adding them for sewing together into
a solid does not appear to have side-effects so far.
AjinkyaDahale added a commit to AjinkyaDahale/FreeCAD that referenced this pull request Nov 28, 2021
Similar to PR FreeCAD#5170 for loft. This commit squashes the following commits.

[PD] Refactor `Pipe::execute` and support point sections

[PD] Allow point profile in selection-based pipe workflow

[PD] Only add sketch subs if it is vertex

[PD] Make both end faces of pipe regardless of point sections

Earlier we were checking if these faces correspond to point sections, but
apparently the end faces are independent of the order in which the sections are
added, so the "front" may be the face closest to the last added section, rather
than the "profile". Creating null faces and adding them for sewing together into
a solid does not appear to have side-effects so far.
AjinkyaDahale added a commit to AjinkyaDahale/FreeCAD that referenced this pull request Nov 28, 2021
Similar to PR FreeCAD#5170 for loft. This commit squashes the following commits.

[PD] Refactor `Pipe::execute` and support point sections

[PD] Allow point profile in selection-based pipe workflow

[PD] Only add sketch subs if it is vertex

[PD] Make both end faces of pipe regardless of point sections

Earlier we were checking if these faces correspond to point sections, but
apparently the end faces are independent of the order in which the sections are
added, so the "front" may be the face closest to the last added section, rather
than the "profile". Creating null faces and adding them for sewing together into
a solid does not appear to have side-effects so far.
donovaly pushed a commit that referenced this pull request Nov 28, 2021
Similar to PR #5170 for loft. This commit squashes the following commits.

[PD] Refactor `Pipe::execute` and support point sections

[PD] Allow point profile in selection-based pipe workflow

[PD] Only add sketch subs if it is vertex

[PD] Make both end faces of pipe regardless of point sections

Earlier we were checking if these faces correspond to point sections, but
apparently the end faces are independent of the order in which the sections are
added, so the "front" may be the face closest to the last added section, rather
than the "profile". Creating null faces and adding them for sewing together into
a solid does not appear to have side-effects so far.
@AjinkyaDahale AjinkyaDahale deleted the pd-loft-support-point branch February 26, 2022 02:55
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.

5 participants