[PartDesign] Fix 'Reversed' no more available in Pad 'toFirst/toLast' + Pocket '2dims'

[0penBrain]

While useless 'Midplane' was still visible
Bug introduced in commit cf11f38, not fixed by commmit b4b1cbe
Also fixing some typos

• Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
• In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
• Your branch is rebased on latest master git pull --rebase upstream master
• All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
• All commit messages are well-written ex: Fixes typo in Draft Move command text
• Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
• Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

[0penBrain]

@donovaly would you be interested in having a look at this as you made last changes?

[adrianinsaval]

on a related note, could we enable this for two dimensions too? You can later change the parameter in the property editor and it has an effect so it doesn't make sense to disable it in my opinion, it's also useful because you can't input a negative number in the first input field and you sometimes want a negative dimension in the direction controlled by that

[adrianinsaval]

ok I realized that's already been done in master for Pad, should have checked before talking, but the unnecessary restriction is still there for pocket, would you mind fixing that? @0penBrain

[luzpaz]

Bug introduced in commit cf11f38, not fixed by commmit b4b1cbe

FTFY 😄

[0penBrain]

@adrianinsaval : Added a 2nd commit to restore ability to reverse in Pocket '2 dimensions' mode

[luzpaz]

cc @AjinkyaDahale

[AjinkyaDahale]

@adrianinsaval : Added a 2nd commit to restore ability to reverse in Pocket '2 dimensions' mode

I don't see why we would want to reverse in "2 dimensions" type pocket, or at least it shouldn't be a checkbox, IMHO. Instead it could be a simple button that switches the numbers.

[adrianinsaval]

because you can't input negative numbers on the first box. Besides, why not use the same control that is already available for the other types? It still makes sense, you are inverting the orientation of the dimensions.

[AjinkyaDahale]

because you can't input negative numbers on the first box. Besides, why not use the same control that is already available for the other types? It still makes sense, you are inverting the orientation of the dimensions.

I think I see what you mean. So we invert and put a negative number in the second box instead. My philosophy with the change in UI was to avoid confusion (though there's nothing wrong in keeping the checkbox as well), but given the constraint it sounds reasonable.

[AjinkyaDahale]

After rebasing onto 8b061ce: Compiles and passes TestPartDesignApp and TestPartDesignGui.

The "reverse" checkbox is now visible where described. However, some comments.

1. Reverse does not seem to work for "to first" pocket (TODO: create a forum post and upload file). I don't know if this is intended for this particular case. Pressing OK gives an error. Same does not happen with pad.
   
   

2. @0penBrain you might want to make reverse active for "up to face" type pad/pockets as well, for cases where the face loops around both sides of the sketch plane. The reversed checkbox can stay selected if selected in any other mode and changes the resulting pocket.
   
   

[adrianinsaval]

interesting, then the logic to enable/disable this checkbox should be removed as it is an unnecessary (and counterproductive) restriction. The issue with the reversed to first is a separate problem and most likely related with how up to face pockets are made rather than the reverse checkbox if you set your sketch was oriented the other way the pocket probably would fail when you don't check reversed but work when you check reversed.

[0penBrain]

@AjinkyaDahale
Reversing on 'ToFirst' works correctly. Your error is something else (no or partial face in the reverse direction probably)

I had a check at the special case of reversing in the case of 'ToFace' (curved faces looping around) as I wasn't sure how it will be handled. It appears that it is correctly handled by geometric functions ...

...And moreover it reacts well when the target face is only available on one side

So I added another commit that enable 'Reversed' for 'ToFace' mode.

For other remarks, please keep in mind that originally this PR just fixes a regression. We can take occasion of it for slight improvements, but for bigger changes we need a forum discussion and another PR to keep clean management of code. ;)

[AjinkyaDahale]

@AjinkyaDahale
Reversing on 'ToFirst' works correctly. Your error is something else (no or partial face in the reverse direction probably)

It does work on a pad, but for some reason did not seem to work for pocket. But as you mentioned we should discus this in a forum thread.

I had a check at the special case of reversing in the case of 'ToFace' (curved faces looping around) as I wasn't sure how it will be handled. It appears that it is correctly handled by geometric functions ...

...And moreover it reacts well when the target face is only available on one side

So I added another commit that enable 'Reversed' for 'ToFace' mode.

Thanks for that. I guess you could add the same to pocket as well, or remove the disabling of reverse option entirely as @adrianinsaval suggested. Though again, maybe another time.

For other remarks, please keep in mind that originally this PR just fixes a regression. We can take occasion of it for slight improvements, but for bigger changes we need a forum discussion and another PR to keep clean management of code. ;)

I understand. Sorry for the clutter.

[donovaly]

@donovaly would you be interested in having a look at this as you made last changes?

Thanks for the fix. The first commit looks good.

What is the reason for the changes in
commit 84e9449
commit d395c78

---

I have further patches ready for the pad feature but since nobody is at the moment merging PRs, I cannot move on. (Any idea what is with Werner?)
For example my PR for padding along edges was approved 4 months ago but still not merged:
#4685

When this is in, I will distribute the pad features also to pocket (to pocket along edges would be very useful).

[0penBrain]

@donovaly 1st commit restores 'Reversed' for Pad 'ToFirst' and 'ToLast'. 2nd commit makes the same for Pocket 'TwoDimensions' mode. 3rd commit add the same ability for Pad 'UpToFace' mode as it can make sense in some situations.
And no, I have no idea what's going on at Werner's side. I saw he didn't commit for more than a month. Hope he's well and soon back as I also have some PR pending in the Core/Gui.

[donovaly]

1st commit restores 'Reversed' for Pad 'ToFirst' and 'ToLast'. 2nd commit makes the same for Pocket 'TwoDimensions' mode.

Many thanks. I tested this thoroughly and it works perfectly.

3rd commit add the same ability for Pad 'UpToFace' mode as it can make sense in some situations.

Do you have an example for me please? I cannot find a test case to see if this works properly.

Here is my test file I used to test the PR:
Pad-Test.zip

[AjinkyaDahale]

3rd commit add the same ability for Pad 'UpToFace' mode as it can make sense in some situations.

Do you have an example for me please? I cannot find a test case to see if this works properly.

Hi @donovaly,
Please take a look at this example. Try padding/pocketing Sketch001.
pad-up-to-face.zip

[donovaly]

Please take a look at this example. Try padding/pocketing Sketch001.
pad-up-to-face.zip

Thanks. I could test with this.

So the PR works fine and should go in.

[donovaly]

I tested the PR with example files posted in the discussion as well as with complex real-life documents.

The code looks also good, so approved to go in.

[adrianinsaval]

could you enable the reversed checkbox for up to face pockets too? I could make a PR to your repo with that change if you want

[0penBrain]

@adrianinsaval it should be possible but better if you can provide a use case for it as I can't figure out one. Also, is this regression from 0.19 or new feature?

[adrianinsaval]

Same as up to face pad, you could call it a new feature but I'm thinking this is a UI bug, reversing the pocket works from the porperties panel but can't be set in the task dialog, in this example (rename .zip to .fcstd) I have set the Reversed property to true in 0.19 and it works
reversedPocketExample.zip

[adrianinsaval]

actually I just tried it and you can check reversed while in a different mode then switch to up to face and it has the same effect so definitively a bug

[AjinkyaDahale]

@adrianinsaval it should be possible but better if you can provide a use case for it as I can't figure out one. Also, is this regression from 0.19 or new feature?

@0penBrain, you can take a look at the file I posted here some days ago. It was meant for pads, but you could as well try to make a pocket with that sketch and see the need for a reverse in "up to face" pockets.

[0penBrain]

@adrianinsaval added a commit to enable 'Reverse' for Pocket/UpToFace.