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

PR #3456 - Adding chamfer angle field to PartDesign

Postby armandas » Sun May 10, 2020 2:03 pm

Hi everyone.

Contributing to FreeCAD for the first time. I was designing a part and found it frustrating that the chamfer feature was fixed at 45 degrees.

Well, it looks like adding an angle field was not too difficult, so I implemented it. Please let me know your comments.

https://github.com/FreeCAD/FreeCAD/pull/3456

P.S.

I would like to add distance-distance and length options, but I think I need to study the Qt first to be able to implement a nice UI. However even this feature alone is a huge improvement, IMO.
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Sun May 10, 2020 5:04 pm

armandas wrote:
Sun May 10, 2020 2:03 pm
Hi everyone.

Contributing to FreeCAD for the first time. I was designing a part and found it frustrating that the chamfer feature was fixed at 45 degrees.

Well, it looks like adding an angle field was not too difficult, so I implemented it. Please let me know your comments.

https://github.com/FreeCAD/FreeCAD/pull/3456

P.S.

I would like to add distance-distance and length options, but I think I need to study the Qt first to be able to implement a nice UI. However even this feature alone is a huge improvement, IMO.
That's the attitude!! :mrgreen:

Code looks good. Compiles. Works as advertised. Loads legacy. Legacy loads the output (the only drawback being that angle defaults to 45 degrees upon recompute). Merged.
adrianinsaval
Posts: 319
Joined: Thu Apr 05, 2018 5:15 pm

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

Postby adrianinsaval » Sun May 10, 2020 5:45 pm

Great news! Thanks for your contribution, I hope you manage to implement distance-distance too :)
adrianinsaval
Posts: 319
Joined: Thu Apr 05, 2018 5:15 pm

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

Postby adrianinsaval » Sun May 10, 2020 9:23 pm

Some weird behavior:
Screenshot_20200510_170609.png
Screenshot_20200510_170609.png (12.09 KiB) Viewed 554 times

Code: Select all

Size.setUnit(Base::Unit::Angle);
In line 64 should probably be:

Code: Select all

Anlge.setUnit(Base::Unit::Angle);
Additionally, I was wondering if the 89.99 deg limit really should be there? I know 90 deg would fail for a 90 deg intersection, bu take this example:
Screenshot_20200510_171314.png
Screenshot_20200510_171314.png (22.64 KiB) Viewed 554 times
There's no reason why 90 deg or more should be dissallowed. Or this example:
Screenshot_20200510_171946.png
Screenshot_20200510_171946.png (28.99 KiB) Viewed 554 times
Which fails much earlier than 90 deg. Or is 90 deg the limit imposed by OCCT?
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Postby armandas » Sun May 10, 2020 11:03 pm

Thank you for your feedback, guys!
abdullah wrote:
Sun May 10, 2020 5:04 pm
the only drawback being that angle defaults to 45 degrees upon recompute
I don't quite understand what this means, but if there is a solution, I would be happy to implement it.
adrianinsaval wrote:
Sun May 10, 2020 9:23 pm
In line 64 should probably be:

Code: Select all

Anlge.setUnit(Base::Unit::Angle);
Ah, the infamous copy-paste bug. I should have reviewed my code better...
adrianinsaval wrote:
Sun May 10, 2020 9:23 pm
Additionally, I was wondering if the 89.99 deg limit really should be there?
You are right. I was using Draft as a reference and they have this limit there. I suppose the logical limit is 180 degrees, but, like you said, it can fail before 90 degrees too, so maybe just set the limit to FLT_MAX and let OCC deal with it?

I will create another PR with the fixes.
adrianinsaval
Posts: 319
Joined: Thu Apr 05, 2018 5:15 pm

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

Postby adrianinsaval » Mon May 11, 2020 12:58 am

armandas wrote:
Sun May 10, 2020 11:03 pm
I don't quite understand what this means, but if there is a solution, I would be happy to implement it.
It means if you save a file with a chamfer with a different angle and you open in 0.18 or early and recompute, freecad will use the 45 deg angle because its all it knows, that can't be fixed.
so maybe just set the limit to FLT_MAX and let OCC deal with it?
I think that is better, this way it will be the same wether it fails at 60 deg or 120 deg or whatever.

Since you're working with chamfer, would you mind adding variable length too? I think that is an option in part workbench.
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Postby armandas » Mon May 11, 2020 3:39 am

adrianinsaval wrote:
Mon May 11, 2020 12:58 am
Since you're working with chamfer, would you mind adding variable length too? I think that is an option in part workbench.
I'll look into the Part workbench code for inspiration.

On an unrelated note, it looks to me that what is called variable-length in FreeCAD is what I called distance-distance setting. I imagined variable length would look something like this:
Image
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Postby armandas » Mon May 11, 2020 3:53 am

I have created a draft PR with the fixes: https://github.com/FreeCAD/FreeCAD/pull/3460

Any comments would be much appreciated.
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Mon May 11, 2020 1:51 pm

armandas wrote:
Sun May 10, 2020 11:03 pm
Thank you for your feedback, guys!
abdullah wrote:
Sun May 10, 2020 5:04 pm
the only drawback being that angle defaults to 45 degrees upon recompute
I don't quite understand what this means, but if there is a solution, I would be happy to implement it.
Adrian got it right.

What I meant is that if you open a file saved with your branch in an older version of FreeCAD, and the angle of the chamfer is not 45 degrees, on recompute it will be made 45 degrees. This is only logical because the older version does not have angled support. It is not fixable (other than backporting the commit). It is just ok.
armandas wrote:
Sun May 10, 2020 11:03 pm
adrianinsaval wrote:
Sun May 10, 2020 9:23 pm
In line 64 should probably be:

Code: Select all

Anlge.setUnit(Base::Unit::Angle);
Ah, the infamous copy-paste bug. I should have reviewed my code better...
I missed it too. However, this one is easy to fix.
armandas wrote:
Sun May 10, 2020 11:03 pm

You are right. I was using Draft as a reference and they have this limit there. I suppose the logical limit is 180 degrees, but, like you said, it can fail before 90 degrees too, so maybe just set the limit to FLT_MAX and let OCC deal with it?

I will create another PR with the fixes.
Edit box limitation shall be the maximum possible value. Whether OCC fails or not is a different problem. OCC may fail because it is requested an impossible champfer or because of the algorithm used fails.

Opencascade documentation does not set bounds for the angle parameter.
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Mon May 11, 2020 2:14 pm

armandas wrote:
Mon May 11, 2020 3:53 am
I have created a draft PR with the fixes: https://github.com/FreeCAD/FreeCAD/pull/3460

Any comments would be much appreciated.
I am not sure FLT_MAX is the right option. The reason is there should be maximum limits for an angle. 3600 degrees does not sound reasonable (but it is accepted as input). If there is a hard limit, my opinion is that the input should be limited to this hard limit (but I am as always open to expert opinion :) ).

Sure higher than 90 degress (e.g. 100 degrees) is possible in some (acute) corners. But, I am not myself technically knowledgeable enough about champfers. It looks to me that 180 degrees should be the hard limit, but let's try to get somebody who knows...
chrisb wrote: ...
Sorry to disturb, can a champfer have an angle higher than 180 degrees? If yes, what is the limit?