PR #3456 - Adding chamfer angle field to PartDesign

[armandas]

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?

How about this approach:

• Store distance as Dimension1.
• Store distance or angle as Dimension2.
• Store the type of Chamfer (e.g. distance, distance-distance, distance-angle)
• Use D1 and D2 based on the type.

The benefits I can think of:

• Multi-chamfer support (each edge has separate values) could use the same approach.
• Provides a way to implement "chamfer width" (i.e. hypotenuse of a triangle) support.

[adrianinsaval]

[*] Store distance or angle as Dimension2.

Can the dimension be dynamically changed to go from mm to deg and back?

[*] Provides a way to implement "chamfer width" (i.e. hypotenuse of a triangle) support.
[/list]

Would that be possible for other intersection angles besides 90 deg?

[armandas]

Can the dimension be dynamically changed to go from mm to deg and back?

That's what I'm interested in too. Alternatively, the input field could be dimensionless and the code would treat the value as needed.

Would that be possible for other intersection angles besides 90 deg?

In theory, it's just a simple calculation. Though implementing it using OCC API may not be trivial. I just checked how Solidworks does it, and actually, they ask to select the faces (makes sense, since you need to know the angle between the faces).

It's probably not the most important feature, I just mentioned it as an extensibility example.

[rynn]

Why not use three values, D1, D2 and alpha. This will waste 8 bytes in memory and probably 30 bytes in the uncompressed xml but prevents any confusion about degree/mm/inches/yards/foots/rods/chains/…. The gui could use a QStackedWidget if enable/disable seems insufficient.
If one wants to pack the values one should probably use std::variant or similar.

[abdullah]

• Store distance or angle as Dimension2.

Can the dimension be dynamically changed to go from mm to deg and back?

That's what I'm interested in too. Alternatively, the input field could be dimensionless and the code would treat the value as needed.

I have not seen properties changing type dynamically. I do not claim they do not exist. I just have not seen them.

The editors for properties in the property editor depend on the property types. If we use an adimensional property, we will not have the angle editor (degrees symbol) or the dimension editor (mm) when editing.

I would start under the assumption that method1 (distance-angle) will use two properties of those types and method2 (distance-distance) will use two properties of the dimension type. Still method3 (distance, the one previously existing in PD) would use only one distance property. The default, I think, should be method3, because it minimizes the surprise of a user when loading a legacy file.

There are examples in FreeCAD PD of unused properties (for example a pad has a second length that is only used when another property of enum type says "two dimensions").

We were assuming a difference with our situation, in that having a degree with a wrong value may reasonably be perceived as misleading. Hey I have distance1=5mm, distance2=10mm, but the angle property is shown as 45 degrees, how can this be? In the case of the pad is easier to ignore (oh yes this length2 is 100mm, but my pad only has one dimension). We were saying than then we will have to update some properties upon changing others. This behaviour we want to avoid (see also Werner's comment). However, there might be a path in the middle: 1) Have 3 properties, 2) Dynamically change them so that the UI only shows the relevant ones, 3) Preferably, if possible setting the unused ones to non-persistent state, as why use up space in a file if it is not needed (this is a bonus requirement).

The alternative to this is to have different object classes. I think that for this specific situation it may be too much.

n theory, it's just a simple calculation. Though implementing it using OCC API may not be trivial. I just checked how Solidworks does it, and actually, they ask to select the faces (makes sense, since you need to know the angle between the faces).

It's probably not the most important feature, I just mentioned it as an extensibility example.

Eventually, extending to a method 4 sounds plausible with a system with an enum property supporting different methods of creation.

When you are convinced of a technical solution for implementing the different modes, I would appreciate if we could fix the current master behaviour, so that it defaults to the previous behaviour, so that users of the development branch are not hindered when opening older designs and get the behaviour they expected (and obtained) when they created the model.

[armandas]

Hi Guys.

It's getting late here, so I will resume the work tomorrow. For now, I have started to implement the changes using a QStackedWidget and two new properties - Type and Size2.

https://github.com/FreeCAD/FreeCAD/comp ... erDistDist

One issue is that my stackedWidget is not visible in the UI. I'm not sure if it is a problem with the XML or the code. If someone could check, I would appreciate it. Otherwise, I will resume debugging tomorrow.

Of course, any other comments are welcome.

[abdullah]

Hi Guys.

It's getting late here, so I will resume the work tomorrow. For now, I have started to implement the changes using a QStackedWidget and two new properties - Type and Size2.

https://github.com/FreeCAD/FreeCAD/comp ... erDistDist

One issue is that my stackedWidget is not visible in the UI. I'm not sure if it is a problem with the XML or the code. If someone could check, I would appreciate it. Otherwise, I will resume debugging tomorrow.

Of course, any other comments are welcome.

Nice progress!

Copy&Paste played a trick on you with the connection of the enum combobox. With this, the showing part works:

Code: Select all

connect(ui->chamferType, SIGNAL(currentIndexChanged(int)),
        this, SLOT(onTypeChanged(int)));

But, then it does not quite follow the selection and the new edit controls are unaligned... WIP .

On a personal opinion, "Type" to refer to the "Method" or "Mode" may not be the best variable/function naming. "Type" has a very specific meaning in FreeCAD, and it may be misleading for developers to understand the code.

I have realised that editing a property that is not taken into account by the current method triggers a recompute. This is also the case in Pad. It may be possible to avoid having the object touch if the property is not affecting the method (bonus). I have never done this myself. In App/Property.h you can check the possible values of status of the property. Maybe the NoRecompute status can be set for the unused properties (and removed from the used ones). Not sure. Requires testing. In the same place you have the PropNoPersist, which may cause the unnecessary properties not to be written upon saving.

It is going to end up being a nice addition

[rynn]

Code: Select all

connect(ui->chamferType, SIGNAL(currentIndexChanged(int)),
        this, SLOT(onTypeChanged(int)));

The connection between a QCombobox and a QStackedWidget can be made in designer, so no code necessary.

Example can be run with Ctrl-R from designer.

[armandas]

Thanks for the help, guys, I made some more progress.

Copy&Paste played a trick on you with the connection of the enum combobox. With this, the showing part works:

Good spot, it would have taken me a while to debug that...

On a personal opinion, "Type" to refer to the "Method" or "Mode" may not be the best variable/function naming. "Type" has a very specific meaning in FreeCAD, and it may be misleading for developers to understand the code.

Purely from UI perspective, Chamfer Type makes sense, Solidworks, Onshape and Fusion360 use this term. I have renamed Type to ChamferType, hopefully this will be clearer.

The connection between a QCombobox and a QStackedWidget can be made in designer, so no code necessary.

Thanks for the tip, I was able to use your method. Is it correct to assume that I can also define all combo box items in GUI? I saw them being populated in code and assumed that it was done for translation purposes, but looking at setupUi(), I can see that retranslateUi() is called, so maybe I don't need to populate the items in code?

The next confusing issue I have now is that the Angle field does not get populated in the dialog, even though the property is set...

[abdullah]

Purely from UI perspective, Chamfer Type makes sense, Solidworks, Onshape and Fusion360 use this term. I have renamed Type to ChamferType, hopefully this will be clearer.

Clearer, but I still think it would be better not to use "Type" at all. Other software packages are not opensource. Users and developers live in separate universes. Here users and developers belong to a single reality. Using easy to misinterpret variable names in the code makes it cumbersome for other developers to understand what is intended. The effect is that we spend more time reading code than we should. It also makes it easier to introduce new bugs when modifying the code or performing code maintenance. As general statement (your case is not so extreme), we all prefer a well written story that we get right away, rather than a poor translation that we have to work hard to make sense out of it.

This is just to make my reasons understood. If you do not want to change it, I will no further try to persuade you to.

Is it correct to assume that I can also define all combo box items in GUI? I saw them being populated in code and assumed that it was done for translation purposes, but looking at setupUi(), I can see that retranslateUi() is called, so maybe I don't need to populate the items in code?

I am not sure if there is a requirement to have them in code (somebody correct me if there is one).

In any case, I think it makes sense to have it in code, to make the code more readable. If it is in the code, a person reading the code will understand it directly. If it is not, one has to go to the ui file to check. This goes again in the direction of investing more time to understand code written by another person. Similarly, somebody wanting to modify it to add a new method does not need to bring modifications to the ui file. A PR adding a new method would also be easier to review and easier to identify problems, such as having added the method to the FeatureChamfer (in App), but having forgotten to update the ui file accordingly. The whole idea goes into the direction of "in some cases, it may make sense to invest more time to code something, if that makes it easier for other developers' to understand it".

The next confusing issue I have now is that the Angle field does not get populated in the dialog, even though the property is set...

I have tracked this down to the maximum being set to 0 when setting the angle, as 45 degrees (default value) is higher than zero, the code setting quantity truncates the value to the maximum (zero). Inverting the lines to set the boundaries first and the value afterwards gets the wanted behaviour:

Code: Select all

    ui->chamferAngle->setUnit(Base::Unit::Angle);
    ui->chamferAngle->setMinimum(0.0);
    ui->chamferAngle->setMaximum(180.0);
    ui->chamferAngle->setValue(pcChamfer->Angle.getValue());