PR #3456 - Adding chamfer angle field to PartDesign

[rynn]

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]

Merged rynn's post.

rynn: you may upload or link your model here.

[wmayer]

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.

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

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.

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]

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 (31.91 KiB) Viewed 1797 times

The Result seems even to be different, depending on the order of the edges in the dialog.

[chrisb]

¹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]

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]

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]

I think there should be a toogle that switches between distance-distance and distance-angle. Previous method was distance-distance with D1=D2.

[abdullah]

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]

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.