PR #3456 - Adding chamfer angle field to PartDesign

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby armandas » Sat May 16, 2020 12:43 pm

uwestoehr wrote:
Fri May 15, 2020 3:22 pm
- the angle "0°" is allowed despite it is invalid
How do you work around this? Set the minimum value to something like FLT_MIN?
uwestoehr wrote:
Fri May 15, 2020 3:22 pm
- the angle does not check if the result will be invalid or not: Take e.g. a 5mm cube and add a 80° chamfer to see that this is invalid because this would be a distance of > 5mm. I am not sure this is easy to fix.
Well, I noticed the chamfer Build() documentation says this:
Warning The construction of chamfers implements highly complex construction algorithms. Consequently, there may be instances where the algorithm fails, for example if the data defining the parameters of the chamfer is not compatible with the geometry of the initial shape. There is no initial analysis of errors and these only become evident at the construction stage.
wmayer
Site Admin
Posts: 15723
Joined: Thu Feb 19, 2009 10:32 am

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby wmayer » Sat May 16, 2020 1:22 pm

armandas wrote:
Sat May 16, 2020 12:43 pm
How do you work around this? Set the minimum value to something like FLT_MIN?
No, the minimum should be 0 deg and the fillet feature must raise an error if it's not > 0. The dialog must handle this error and disallow to accept invalid input.

In general it's anyway bad practice to add a lot of code to check if an operation can be performed or not. Much better is to just perform it and then react on thrown exceptions or an invalid shape. Also because there are so many different cases so that we will never find all examples where it can fail.
vocx
Posts: 3988
Joined: Thu Oct 18, 2018 9:18 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby vocx » Sat May 16, 2020 4:56 pm

armandas wrote:
Sat May 16, 2020 12:25 pm
...
Also, while we're talking about UI, any idea how to align the input fields? It looks a bit messy now.
In the .ui file, you can add a QGroupBox and set it to something like grid, so you have left columns and right columns.
Always add the important information to your posts if you need help.
To support the documentation effort, and code development, your donation is appreciated: paypal.
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby abdullah » Sun May 17, 2020 7:06 am

This is just a short post to say that I have not forgotten about this.

I see that there is a flip property now added to control the side of the distance in the distance-angle version.

There is an already increasing number of properties which are not used in all modes.

Discussing about changing the persistence of properties, realthunder suggested that maybe dynamic properties would be more appropriate to achieve that.

I have never used dynamic properties, but I will try to make it work with them. We might them see what is better and we might learn a couple of things from it.
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby armandas » Sun May 17, 2020 8:22 am

I got the Flip Direction button working. I think this implementation is OK. The edge can only have two associated faces, right?
Attachments
Flipdirection.gif
Flipdirection.gif (256.28 KiB) Viewed 268 times
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby armandas » Sun May 17, 2020 9:29 am

vocx wrote:
Sat May 16, 2020 4:56 pm
armandas wrote:
Sat May 16, 2020 12:25 pm
...
Also, while we're talking about UI, any idea how to align the input fields? It looks a bit messy now.
In the .ui file, you can add a QGroupBox and set it to something like grid, so you have left columns and right columns.
I tried using glid layout and it almost works. For some reason, the angle field is smaller than Size and Size 2, even though the widget type is the same...
Attachments
TaskChamferParameters.ui
(6.52 KiB) Downloaded 1 time
Screenshot from 2020-05-17 18-23-48.png
Screenshot from 2020-05-17 18-23-48.png (25.24 KiB) Viewed 262 times
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby abdullah » Sun May 17, 2020 2:06 pm

abdullah wrote:
Fri May 15, 2020 3:13 pm
adrianinsaval wrote:
Thu May 14, 2020 6:58 pm
Why is DressUp::onChanged called anyway if the property change is handled already?
AFAIU, OnChanged is intended to follow a "chain of responsibility" kind of design pattern. This means that when ChamferType changes, Chamfer is responsible for taking into account the change. Unless there is some kind of shared responsibility, there is indeed no need to call up in the hierarchy if the property is already handled. A return statement should follow updateProperties(). Good eye!
Hi Adrian,

I must correct my answer. In this case, there is piece of common code (relates to touching and recomputes) executed at the end of the responsibility chain relating to the status bits. For this reason, it is not possible to early return, unless you want this common code to be ignored for some reason. Take a look to void DocumentObject::onChanged(const Property* prop) in the calls to testStatus().
vocx
Posts: 3988
Joined: Thu Oct 18, 2018 9:18 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby vocx » Sun May 17, 2020 5:29 pm

armandas wrote:
Sun May 17, 2020 9:29 am
... even though the widget type is the same...
Check the geometry property of both widgets. Set a large maximum value for horizontal size, try different things. In some configurations the widget should "grow" to occupy the entire available horizontal space.
Always add the important information to your posts if you need help.
To support the documentation effort, and code development, your donation is appreciated: paypal.
adrianinsaval
Posts: 319
Joined: Thu Apr 05, 2018 5:15 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby adrianinsaval » Mon May 18, 2020 2:20 am

The size policy may be more relevant, you could also add spacers in between
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

Re: PR #3456 - Adding chamfer angle field to PartDesign

Postby armandas » Wed May 20, 2020 11:38 am

Hi everyone!

I tried to mess a bit with the Qt layouts and options to make the fields aligned, but I could not get it to work well. I think I will go with the current layout.

I made a couple of small changes in the code and added a separate icon for the Flip Direction button. Other than the issue with the properties, I think this is ready for review. Please let me know if there is anything else I have missed or done wrong.

The full changeset can be seen here: https://github.com/FreeCAD/FreeCAD/comp ... erDistDist
abdullah wrote:
Sun May 17, 2020 7:06 am
I have never used dynamic properties, but I will try to make it work with them. We might them see what is better and we might learn a couple of things from it.
Abdullah, any progress on this front?