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.
rynn
Posts: 146
Joined: Tue Jul 31, 2018 7:00 am

Problem with new chamfer

Postby rynn » Tue May 12, 2020 1:30 pm

Hello,

there surfaced a problem with the new chamfers functionality in the german forum¹
Seems chamfers can fail even with the default 45° if the faces are not perpendicular to each other. It seems to be impossible to get the old behavior with the new dialog.
___________
¹https://forum.freecadweb.org/viewtopic.php?p=397944
chrisb
Posts: 24284
Joined: Tue Mar 17, 2015 9:14 am

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

Postby chrisb » Tue May 12, 2020 2:18 pm

Merged rynn's post.

rynn: you may upload or link your model here.
wmayer
Site Admin
Posts: 15723
Joined: Thu Feb 19, 2009 10:32 am

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

Postby wmayer » Tue May 12, 2020 2:32 pm

abdullah wrote:
Tue May 12, 2020 1:29 pm
I am not sure there is a "rule" for this. Inter-releases for sure we provide legacy support. Inside a release, I have not seen the case so far.
It depends. If there is an important feature implemented in a pre-release version but has been there for many months then I think we should try to support old project files, too.
abdullah wrote:
Tue May 12, 2020 1:29 pm
My own opinion, that may not be shared by others, is that if there are people using the development version, it is not nice to give a new functionality and then remove it. Of course it is more acceptable to change behaviour in a couple of weeks than changing behaviour after several months. In any case, it is not the end of the world to provide the code for a property that existed and does not exist anymore, either. I can find an example if necessary.
+1
abdullah wrote:
Tue May 12, 2020 1:29 pm
The drawback of having both as properties is that one of the properties is redundant,
Redundancies of properties is something we must avoid. Currently we have a situation like this with the color properties of the view providers which is a historically grown mess. I have it on my todo list to clean this up at some point.

A problem may arise when replacing a certain property with a new one that has another name because then Python scripts may get broken. But this can be solved by overriding the functions getCustomAttributes() and setCustomAttributes() of the Python wrapper class.
abdullah wrote:
Tue May 12, 2020 1:29 pm
The discussion above is about:
a) whether to have 3 properties (dimension, dimension, angle), where one is obviously redundant or have 2 properties (e.g. dimension, dimension) and use the UI to offer extra functionality.
Before working on this we should first discuss if we don't want to make the PD chamfer/fillet as powerful as its counterpart in the Part wb. There we can set a different radius for each edge independently and that's an often missed feature in PD.
rynn
Posts: 146
Joined: Tue Jul 31, 2018 7:00 am

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

Postby rynn » Tue May 12, 2020 2:43 pm

chrisb wrote:
Tue May 12, 2020 2:18 pm
Merged rynn's post.

rynn: you may upload or link your model here.
I’m not the OP.

This is the Chamfer with 41° (45° does not work at all):
Bildschirmfoto von »2020-05-12 16-36-51«.png
Bildschirmfoto von »2020-05-12 16-36-51«.png (31.91 KiB) Viewed 240 times
The Result seems even to be different, depending on the order of the edges in the dialog.
chrisb
Posts: 24284
Joined: Tue Mar 17, 2015 9:14 am

Re: Problem with new chamfer

Postby chrisb » Tue May 12, 2020 3:02 pm

rynn wrote:
Tue May 12, 2020 1:30 pm
¹https://forum.freecadweb.org/viewtopic.php?p=397944
You linked the wrong topic. Better: https://forum.freecadweb.org/viewtopic. ... 03#p397876

Here is a link to the object: https://forum.freecadweb.org/download/f ... ?id=113525
adrianinsaval
Posts: 319
Joined: Thu Apr 05, 2018 5:15 pm

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

Postby adrianinsaval » Tue May 12, 2020 3:43 pm

Calculating the angle asuming 90 degree intersection is trivial but that is not the case, you would need to first retrieve the angle between faces, possibly with special treatment depending on if it is internal or external. Then if a change the design and the angle between faces change, the calculated angle will be different and the angle I chose when creating the chamfer will be lost.
The same will happen the other way around if we store angle and calculate distance.
wmayer
Site Admin
Posts: 15723
Joined: Thu Feb 19, 2009 10:32 am

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

Postby wmayer » Tue May 12, 2020 3:57 pm

The regression is caused by replacing

Code: Select all

mkChamfer.Add(size, size, edge, face);
with

Code: Select all

mkChamfer.AddDA(size, angle, edge, face);
which in general does different things. For simple cases the output might be the same but not for more complex cases.

The class BRepFilletAPI_MakeChamfer forwards everything to ChFi3d_ChBuilder and when comparing the implementation of the corresponding functions you will see some differences:

Code: Select all

void  ChFi3d_ChBuilder::Add(const Standard_Real Dis1,
			    const Standard_Real Dis2,
			    const TopoDS_Edge& E,
			    const TopoDS_Face& F)
{
  if (!Contains(E)  && myEFMap.Contains(E)) {

    TopoDS_Edge E_wnt = E;
    E_wnt.Orientation(TopAbs_FORWARD);
    
    Handle(ChFiDS_Stripe) Stripe = new ChFiDS_Stripe();
    Handle(ChFiDS_Spine)& Sp = Stripe->ChangeSpine();
    Sp = new ChFiDS_ChamfSpine(tolesp);
    Handle(ChFiDS_ChamfSpine) 
      Spine = Handle(ChFiDS_ChamfSpine)::DownCast(Sp);
    
    Spine->SetMode(myMode);
    Standard_Real Offset = -1;
    if (myMode == ChFiDS_ConstThroatWithPenetrationChamfer)
      Offset = Min(Dis1,Dis2);;
    
    Spine->SetEdges(E_wnt);
    if(PerformElement(Spine, Offset, F)){
      Spine->Load();
      myListStripe.Append(Stripe);
      
      Spine->SetDists(Dis1, Dis2);
      
      PerformExtremity(Spine);
    }
  }
}

Code: Select all

void  ChFi3d_ChBuilder::AddDA(const Standard_Real Dis1,
			      const Standard_Real Angle,
			      const TopoDS_Edge& E,
			      const TopoDS_Face& F)
{
  if (!Contains(E)  && myEFMap.Contains(E)) {

    TopoDS_Edge E_wnt = E;
    E_wnt.Orientation(TopAbs_FORWARD);
    
    Handle(ChFiDS_Stripe) Stripe = new ChFiDS_Stripe();
    Handle(ChFiDS_Spine)& Sp = Stripe->ChangeSpine();
    Sp = new ChFiDS_ChamfSpine(tolesp);
    Handle(ChFiDS_ChamfSpine) 
      Spine = Handle(ChFiDS_ChamfSpine)::DownCast(Sp);
    
    Spine->SetEdges(E_wnt);
    if(PerformElement(Spine, -1, F)) {
      Spine->Load();
      myListStripe.Append(Stripe);
      
      Spine->SetDistAngle(Dis1, Angle);
      
      PerformExtremity(Spine);
    }
  }
}
So, in order to support both methods we need a way to control them.

Salome provides a chamfer function too and from the screenshot you can see that they support four different modes:
https://docs.salome-platform.org/7/gui/ ... _page.html

So, I think we should look how they do it and adapt our implementation to theirs.
adrianinsaval
Posts: 319
Joined: Thu Apr 05, 2018 5:15 pm

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

Postby adrianinsaval » Tue May 12, 2020 6:03 pm

I think there should be a toogle that switches between distance-distance and distance-angle. Previous method was distance-distance with D1=D2.
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Tue May 12, 2020 6:14 pm

wmayer wrote:
Tue May 12, 2020 3:57 pm
So, in order to support both methods we need a way to control them.

Salome provides a chamfer function too and from the screenshot you can see that they support four different modes:
https://docs.salome-platform.org/7/gui/ ... _page.html

So, I think we should look how they do it and adapt our implementation to theirs.
It appears they support four different modes depending on the selection (whether an full object, two faces, one face, or a plurality of edges are selected). For the full object, it uses what we had in PartDesign before this commit. For each of the rest, there are two modes (distance-distance) and (distance-angle).

If the creation methods are different and the properties are of different types, I can think of:
a) add an enum property (build method: distance, distance-distance, distance-angle), create 3 properties (distance, distance, angle) and enable/disable properties depending on the enum value. B enable/disable I mean (hide, make non-persistent) (show/make persistent).

b) If not having extra properties is a must, maybe we can create different filletfeatures, one implementing each method. Each of them is totally independent from the others.

Just to clarify:
Do you see a better technical solution?
abdullah
Posts: 3446
Joined: Sun May 04, 2014 3:16 pm

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

Postby abdullah » Tue May 12, 2020 6:15 pm

adrianinsaval wrote:
Tue May 12, 2020 6:03 pm
I think there should be a toogle that switches between distance-distance and distance-angle. Previous method was distance-distance with D1=D2.
That is my opinion too for the UI.