Part: Fix Part_Extrude taper angle regression for internal faces#26781
Conversation
The taper angle for holes (inner wires) in `Part_Extrude` was not being negated after the toponaming refactor, causing internal faces to taper in the wrong direction compared to v0.21.2. The original makeDraft function correctly handled inner wires by: - Negating taper for all inner wires in Part_Extrude - Negating taper only for multi-edge inner wires in PartDesign This logic was controlled via an isPartDesign function parameter. When makeElementDraft was introduced for toponaming support, it was designed to use innerTaperAngleFwd/innerTaperAngleRev fields that were never ported from realthunder's branch. The cleanup (commit c31ebee) removed references to these non-existent fields, leaving makeElementDraft with no inner wire taper handling at all. So, this fix ports the makeDraft inner wire logic to makeElementDraft by adding the flag, and counting wires and then triggering proper angle flip depending on the flag/wire situation.
github-actions
bot
added
Mod: Part Design
maxwxyz
added
Type: Bug
|
Thanks for working on this. I am missing some version related code. |
src/Mod/Part/App/ExtrusionHelper.h
Outdated
| double taperAngleFwd {0}; // in radians | ||
| double taperAngleRev {0}; | ||
| std::string faceMakerClass; | ||
| bool isPartDesign {false}; // affects inner wire taper handling for single-edge circles |
There was a problem hiding this comment.
We should not make booleans like "isPartDesign" - "PartDesign" does not exists as a conecept in the domain of "Extrusion Paremeters". Instead we need to find the concept that part design does, name it and use here.
There was a problem hiding this comment.
Done, changed to an enum, I can't find anything that I could tightly couple that with PartDesign, so I just went with that. I guess the enum can be reused with the new property (not the hidden one for compat) later on as well.
| // process each inner wire individually to check edge count | ||
| // Inner wires (holes) need negated taper angles | ||
| for (auto& innerWire : wires) { | ||
| ExtrusionParameters innerParams = params; | ||
|
|
||
| // count edges and if it's a circle and partDesign object, then don't flip the angle | ||
| int numEdges = 0; | ||
| TopExp_Explorer xp(innerWire.getShape(), TopAbs_EDGE); | ||
| while (xp.More()) { | ||
| numEdges++; | ||
| xp.Next(); | ||
| } | ||
|
|
||
| if (numEdges > 1 || !params.isPartDesign) { | ||
| innerParams.taperAngleFwd = -params.taperAngleFwd; | ||
| innerParams.taperAngleRev = -params.taperAngleRev; | ||
| } | ||
| makeElementDraft(innerParams, innerWire, inner, hasher); | ||
| } |
There was a problem hiding this comment.
As @Roy-043 noted it will fix regression for 0.21 but will introduce one for 1.0. We need compatibility measures here - basically if choosing the method based on the property. For now we can create it as hidden, compatibility one but maybe it is worth to expose it to users?
I did something like that here: #26141. As for the default value - @Roy-043 which behavior makes more sense? The one in 1.0 or 0.21?
There was a problem hiding this comment.
Just for consistency the 1.0 behavior seems OK - why should the angle be changed between using Part WB or PD WB to extrude a profile?
There was a problem hiding this comment.
Looking at the picture the "fixed" version has a draft angle that allows you to safely pull the feature upwards (because faces are slanted correctly) while the 1.0 is not achieving that goal. From design perspective if you need draft you probably need it because of it's self-releasing ability from molds etc so that seems to make more sense.
I agree however that Part and Part Design should have consistent behavior. At this point it seems that we simply should have property for it (Inner Faces Draft Orientation? Maybe something like that) which will be supported both by Part and Part Design.
There was a problem hiding this comment.
Done, added hidden compat property for compatibility.
kadet1090
added
Requires: Testing
src/Mod/Part/App/ExtrusionHelper.cpp
Outdated
| for (auto& innerWire : wires) { | ||
| ExtrusionParameters innerParams = params; | ||
|
|
||
| // count edges and if it's a circle and partDesign object, then don't flip the angle |
There was a problem hiding this comment.
Is this comment still accurate?
) * Part: Fix Part_Extrude taper angle regression for internal faces The taper angle for holes (inner wires) in `Part_Extrude` was not being negated after the toponaming refactor, causing internal faces to taper in the wrong direction compared to v0.21.2. The original makeDraft function correctly handled inner wires by: - Negating taper for all inner wires in Part_Extrude - Negating taper only for multi-edge inner wires in PartDesign This logic was controlled via an isPartDesign function parameter. When makeElementDraft was introduced for toponaming support, it was designed to use innerTaperAngleFwd/innerTaperAngleRev fields that were never ported from realthunder's branch. The cleanup (commit c31ebee) removed references to these non-existent fields, leaving makeElementDraft with no inner wire taper handling at all. So, this fix ports the makeDraft inner wire logic to makeElementDraft by adding the flag, and counting wires and then triggering proper angle flip depending on the flag/wire situation. --------- Co-authored-by: Chris Hennes <[email protected]> (cherry picked from commit bb48de8)
|
Successfully created backport PR for |
) * Part: Fix Part_Extrude taper angle regression for internal faces The taper angle for holes (inner wires) in `Part_Extrude` was not being negated after the toponaming refactor, causing internal faces to taper in the wrong direction compared to v0.21.2. The original makeDraft function correctly handled inner wires by: - Negating taper for all inner wires in Part_Extrude - Negating taper only for multi-edge inner wires in PartDesign This logic was controlled via an isPartDesign function parameter. When makeElementDraft was introduced for toponaming support, it was designed to use innerTaperAngleFwd/innerTaperAngleRev fields that were never ported from realthunder's branch. The cleanup (commit c31ebee) removed references to these non-existent fields, leaving makeElementDraft with no inner wire taper handling at all. So, this fix ports the makeDraft inner wire logic to makeElementDraft by adding the flag, and counting wires and then triggering proper angle flip depending on the flag/wire situation. --------- Co-authored-by: Chris Hennes <[email protected]> (cherry picked from commit bb48de8)
kadet1090
added
Approved: Tested
…eCAD#26781) * Part: Fix Part_Extrude taper angle regression for internal faces The taper angle for holes (inner wires) in `Part_Extrude` was not being negated after the toponaming refactor, causing internal faces to taper in the wrong direction compared to v0.21.2. The original makeDraft function correctly handled inner wires by: - Negating taper for all inner wires in Part_Extrude - Negating taper only for multi-edge inner wires in PartDesign This logic was controlled via an isPartDesign function parameter. When makeElementDraft was introduced for toponaming support, it was designed to use innerTaperAngleFwd/innerTaperAngleRev fields that were never ported from realthunder's branch. The cleanup (commit c31ebee) removed references to these non-existent fields, leaving makeElementDraft with no inner wire taper handling at all. So, this fix ports the makeDraft inner wire logic to makeElementDraft by adding the flag, and counting wires and then triggering proper angle flip depending on the flag/wire situation. --------- Co-authored-by: Chris Hennes <[email protected]>
6 participants
The taper angle for holes (inner wires) in
Part_Extrudewas not being negated after the toponaming refactor, causing internal faces to taper in the wrong direction compared to v0.21.2.The original
makeDraftfunction correctly handled inner wires by:This logic was controlled via an isPartDesign function parameter.
When
makeElementDraftwas introduced for toponaming support, it was designed to useinnerTaperAngleFwd/innerTaperAngleRevfields that were never ported from realthunder's branch. The cleanup (commits 4697de1 + c31ebee) removed references to these non-existent fields, leavingmakeElementDraftwith no inner wire taper handling at all.So, this fix ports the
makeDraftinner wire logic tomakeElementDraftby adding the flag, and counting wires and then triggering proper angle flip depending on the flag/wire situation.Note: I'm not even sure why we skip the angle flip for circles in PartDesign, although that seems to match 0.21.2 behavior which is apparently correct.
Before:
After:
Resolves: #26772