PR #3794 - pad in any direction

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #3794 - pad in any direction

Post by uwestoehr »

DeepSOIC wrote: Sun Aug 09, 2020 6:33 pm But Part and PartDesign diverge even further.
Is this important? If so, why?

As said, once we have an agreement on the feature, I will add this to the part WB as well and also to PD's pocket.
chrisb
Veteran
Posts: 53922
Joined: Tue Mar 17, 2015 9:14 am

Re: PR #3794 - pad in any direction

Post by chrisb »

A clear UI is better. The Part function has proven to be useful. It's difficult enough to argue why there are two workbenches after all. They should not have slightly different functions. If there's something better it should be implemented in both.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PR #3794 - pad in any direction

Post by DeepSOIC »

uwestoehr wrote: Mon Aug 10, 2020 2:37 am
DeepSOIC wrote: Sun Aug 09, 2020 6:33 pm But Part and PartDesign diverge even further.
Is this important? If so, why?
That's a very difficult question. I don't have an answer. Ideally, Part and Partdesign should merge or deeply cooperate, but I'm afraid there is no specific plan to do so (as far as i'm aware), and in any case it will probably be a revolution of both.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PR #3794 - pad in any direction

Post by wmayer »

Code review:
  1. The property UseNormalVector is actually superfluous
  2. The tooltips of XSkew, YSkew and ZSkew contain a typo: X-component iof pad direction vector
  3. This feature makes sure that the length of the computed vector is equal to the pad length. The question is if not the z-value of this vector should be equal to the pad length
    In this case instead of 3 additional properties only two are needed.
  4. The modes To last and To first are not supported. But Up to face is supported and there the z-value of the vector is equal to the pad length.
  5. The spinboxes are not expression aware
  6. The user can enter the null vector but this is not checked properly
  7. The tooltip for the reverse button is: Reverses direction for Height
    This is confusing because the property to control the height is called Length and IMO not easy to translate. Better use something like: Reverses pad direction
  8. The shortcut Ctrl+S is set for the symmetric option. However, this shortcut is already used for saving files and thus won't work and raises an ambiguity warning.
  9. In the tooltip of this option again the confusing word Height is used
  10. The panel can be simplified a bit. When making the group box checkable you can remove the extra checkbox. But you then you have to change its title: User-defined direction. By default it will be off.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #3794 - pad in any direction

Post by uwestoehr »

wmayer wrote: Mon Aug 10, 2020 1:55 pm Code review:
Many thanks!

Since padding is an essential feature, its UX must be valuable for most users. Therefore thanks also to all others in having a look and giving feedback.

I hacked the latest version of the PR it a little bit together to present the vector idea for discussions here. This new approach of defining the direction needs some polishing. I am away the week, so I will have a look at this on Friday.

However, I also have question: For the custom vector I used QDoubleSpinBox but one cannot bind expressions to this. I first tried Gui::PrefDoubleSpinBox, but this does not allow expression bindings as well. Then I tried Gui::PrefQuantitySpinBox but since a vector component has no unit, I don't know if I can use it but simply set the unit to an empty string.
What do you recommend me here?

[*]The property UseNormalVector is actually superfluous
At first I did not have this, only a checkbox to use a custom vector. But then I realized that it is then not clear what direction will be used without a custom vector. But of course it is easier to omit the UseNormalVector.
[*]The tooltips of XSkew, YSkew and ZSkew contain a type: X-component iof pad direction vector
Thanks.
[*]This feature makes sure that the length of the computed vector is equal to the pad length. The question is if not the z-value of this vector should be equal to the pad length
In this case instead of 3 additional properties only two are needed.
The point is here that sketches have any possible planes. Take for example a Sketch in a plane with normal vector (1,1,1). Then it is hard to understand the padding direction. I played a bit yesterday with such cases and had a hard time to predict how to pad from such a sketch to get a 30° angle to its normal vector. With the custom vector, I can put the components of the normal vector into e.g. Wolfram Alpha or a CAS system to get a vector back that is e.g. by 30° rotated around the x-axis. This result can be input to the custom padding vector.

Concerning the pad length. I think in most cases you want the given height as result.
[*]The modes To last and To first are not supported.
Oh then I broke something because in my first attempt (using angles for the direction) it worked. I'll have a look.
But Up to face is supported and there the z-value of the vector is equal to the pad length.
I cannot test right now, but I wonder since I think depends on the selected face, no?
[*]The spinboxes are not expression aware
See my question above.
[*]The user can enter the null vector but this is not checked properly
I'll will add a check.
[*]The tooltip for the reverse button is: Reverses direction for Height
This is confusing because the property to control the height is called Length and IMO not easy to translate. Better use something like: Reverses pad direction
[*]In the tooltip of this option again the confusing word Height is used
Yes, thanks.
[*]The shortcut Ctrl+S is set for the symmetric option. However, this shortcut is already used for saving files and thus won't work and raises an ambiguity warning.
I cannot remember having set any shortcut. I'll have a look.
[*]The panel can be simplified a bit. When making the group box checkable you can remove the extra checkbox. But you then you have to change its title: User-defined direction. By default it will be off.
OK.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #3794 - pad in any direction

Post by uwestoehr »

OK, I found some time today.

I think I fixed your remarks (see GitHub):

2. was simple
5. I use simply an empty unit
6. I there would be an empty vector, z is set to 1.0
7. was simple
8. don't know where this came from
9. was simple
10. as you suggested:
new dialog layout
new dialog layout
FreeCAD_ANGIVP2NMv.png (11.59 KiB) Viewed 6318 times
Concerning 4. This works for me. Here is an example file:
Sketch-Test.FCStd
example file
(408.75 KiB) Downloaded 154 times
With this I can use a custom vector:
"To last" works
"To last" works
9of3s2Ovuk.gif (460.66 KiB) Viewed 6318 times
Of course the range of allowed vectors is limited.

Concerning 1. I have now a property UseCustomVector. I need this setting for the case that the user does not use the task dialog but the Data tab.

Concerning 3. I understand you now. The pad length should also be matched if skewed. However, at a quick look I could not find out how I could assure this.
chrisb
Veteran
Posts: 53922
Joined: Tue Mar 17, 2015 9:14 am

Re: PR #3794 - pad in any direction

Post by chrisb »

uwestoehr wrote: Tue Aug 11, 2020 12:32 am 5. I use simply an empty unit
Which unit do you take then, is it always mm?
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
C_h_o_p_i_n
Posts: 225
Joined: Fri Apr 26, 2019 3:14 pm

Re: PR #3794 - pad in any direction

Post by C_h_o_p_i_n »

Hi,

reading about the PR, ... wouldn't it be better (or an also useful alternative way) to specify an certain angle regarding to a plane to specify the direction instead ?

Just as an suggestion ...

Kind regards,
Stefan
chrisb
Veteran
Posts: 53922
Joined: Tue Mar 17, 2015 9:14 am

Re: PR #3794 - pad in any direction

Post by chrisb »

C_h_o_p_i_n wrote: Tue Aug 11, 2020 6:10 am wouldn't it be better (or an also useful alternative way) to specify an certain angle regarding to a plane to specify the direction instead ?
If you look at the first post: there are fields for angles. Allowing arbitrary reference planes seems to be a completely new game.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
Jee-Bee
Veteran
Posts: 2566
Joined: Tue Jun 16, 2015 10:32 am
Location: Netherlands

Re: PR #3794 - pad in any direction

Post by Jee-Bee »

May i ask why not using sherical coordinate system and not a vector. I think that using r (extrusion length) and phi, theta(direction) is easy to understand but a vector is difficult.
https://en.wikipedia.org/wiki/Spherical ... ate_system

I understand that it is under the hood (code) a vector is the way you want. but that is i think easy to convert
Post Reply