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.
abdullah
Posts: 3435
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Wed May 20, 2020 1:33 pm

armandas wrote:
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?
Yes! Sorry I have been absent from this discussion. It has been a tough week with not that much time for FC.

I have just finished the implementation with dynamic properties. As far as I know, this is the first feature using persistent dynamic properties. This is the diff.

Because we were working in parallel with different base and in order to avoid having too many meaningless commits (e.g. adapting task code to inclusion of flip property as static, while it was being added as dynamic), I have squashed some of my lines into your commits (I believe not to have misappropriated any line throughout the process). On the other side, I have split the icon (and resources) into an independent commit (this generally helps review).

I have integrated your latest commit above (I have not reviewed the whole though, only checked that it works).

By no means I am seeking to push the idea of dynamic properties through at any cost. As I was saying, it is an experiment. Maybe there is a reason why there are no other features implemented this way.

The advantage part of the equation is that at a given time, the object only has those properties that are actually necessary to define it. There is no "size2" property if we are in size-angle construction method. The coding is a little bit more convoluted though.

I am most interested in peer review and other advantages/disadvantages you may find. My idea is that among all of us, we make an informed decision whether to pursue this kind of features defined by dynamic properties for this use case (multiple methods requiring different sets of properties), or to stay with static features (potentially hiding them).

In order not to over-extend the period with the changed construction method in master, I would say we could set up a time-limit of a couple of days to decide whether to go static or dynamic, then I will review and integrate whatever the option selected.
wmayer
Site Admin
Posts: 15716
Joined: Thu Feb 19, 2009 10:32 am

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

Postby wmayer » Wed May 20, 2020 4:24 pm

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.
For sure, dynamic properties are an option but they also have a high impact on how to access and use them. If you have a property e.g. Size2 as a normal class member you can easily access its value with "Size2.getValue()".
If it's a dynamic property you first must access it with getPropertyByName("Size2") and then you have to check that it's not null and of the expected type before querying the value.

Another inconvenience is that when you switch to another mode and back and you have removed and restored the property you will lose the old value.
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.
I wouldn't go so far to add or remove them on request. Usually you have opened the appropriate task panel and there you won't even see what happens to a property under the hood.
If you prefer to directly work with the property editor and want to make sure that a user cannot change an inactive property I would just set it to read-only -- I would not even hide it.
By no means I am seeking to push the idea of dynamic properties through at any cost. As I was saying, it is an experiment. Maybe there is a reason why there are no other features implemented this way.
In v0.18 and earlier dynamic properties were a key component of Python features but not C++ features. We had one or two C++ features that used them too but these classes needed to explicitly implement it.
abdullah
Posts: 3435
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Fri May 22, 2020 5:39 pm

wmayer wrote:
Wed May 20, 2020 4:24 pm
I wouldn't go so far to add or remove them on request. Usually you have opened the appropriate task panel and there you won't even see what happens to a property under the hood.
If you prefer to directly work with the property editor and want to make sure that a user cannot change an inactive property I would just set it to read-only -- I would not even hide it.
Sorry for the delay, I have been rather busy debugging the sketch with the expression being assigned to the wrong property.

Thank you for the feedback. Then, we go with static. It is indeed easier to work with them from the console.
armandas wrote:
Wed May 20, 2020 11:38 am
Abdullah, any progress on this front?
I will them review the PR as it is now. I will probably do this tomorrow. Sorry for all the delay.
abdullah
Posts: 3435
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Sat May 23, 2020 6:45 am

armandas wrote:
Wed May 20, 2020 11:38 am
Hi everyone!
I have reviewed it and added a commit with some minor changes, mostly relating to not hiding properties but making then read-only (as per Werner's suggestion):
https://github.com/FreeCAD/FreeCAD/pull ... 1e5432cb55

Any legacy chamfer defaults to the legacy one distance construction method (aka "Equal Distance"). Chamfers created with the "Distance and Angle" method (available since last week) also default to the "Equal Distance" construction method. This means that the users having used this method with an angle different from 45 degrees will have to manually change the "type" of chamfer to the "Distance and Angle" method (the angle they used is loaded).

I have packed all into one new PR for CI:
https://github.com/FreeCAD/FreeCAD/pull/3499
abdullah
Posts: 3435
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Sat May 23, 2020 10:33 am

armandas wrote:
Wed May 20, 2020 11:38 am
Abdullah, any progress?
Thank you very much for contributing to FreeCAD. :)

I have just merged the functionality. :D

Keep it coming ;)
adrianinsaval
Posts: 317
Joined: Thu Apr 05, 2018 5:15 pm

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

Postby adrianinsaval » Sat May 23, 2020 9:19 pm

Great work guys! Thanks!
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Postby armandas » Sat May 23, 2020 10:55 pm

abdullah wrote:
Sat May 23, 2020 10:33 am
Thank you very much for contributing to FreeCAD. :)

I have just merged the functionality. :D

Keep it coming ;)
Wow, that was quick!

Thanks for the help, everyone!
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Postby armandas » Sat May 23, 2020 11:01 pm

By the way, this can be closed now.

https://tracker.freecadweb.org/view.php?id=1204
abdullah
Posts: 3435
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Sun May 24, 2020 6:32 am

armandas wrote:
Sat May 23, 2020 11:01 pm
By the way, this can be closed now.

https://tracker.freecadweb.org/view.php?id=1204
Done!
wmayer
Site Admin
Posts: 15716
Joined: Thu Feb 19, 2009 10:32 am

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

Postby wmayer » Sun May 24, 2020 8:16 am

The angle doesn't seem to work. I cannot set a value different from 45 degree in the task panel. In the property editor I can change it.

When looking at the property editor then all properties of the chamfer appear in the Base group. Since it has four new properties I think they deserve their own group name.
git commit 00be985c971