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.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

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

Post by abdullah »

I have been playing for a while with this option to hide unused properties. Here is what I did:
https://github.com/abdullahtahiriyo/Fre ... erDistDist

I am still unhappy that when I change the enum property, sometimes the propertyeditor is updated (with the newly hidden properties) and sometimes it does not. I have to keep investigating this.

Some peek:
DynamicPropertyChange.gif
DynamicPropertyChange.gif (126.22 KiB) Viewed 2603 times
Here the change to change the properties is not updated for the two first changes (I have to click outside to cause the update), but it does update for the last one.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

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

Post by abdullah »

armandas wrote: Wed May 13, 2020 1:48 pm Of course, any other comments are welcome.
One thing, in void TaskChamferParameters::apply(), at the moment you are applying all the properties, regardless of the enum mode. Only the properties of the mode should be updated.

If you decide to integrate the code I posted above for auto-hidding, the properties not belonging to the enum are set read-only (so that they do not cause the object to be touched and if somebody tries to change them from the Python console, that he or she gets an error. Better an error that unexpected behaviour). The apply to all properties will fail for those that are read-only according to the mode.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

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

Post by adrianinsaval »

abdullah wrote: Thu May 14, 2020 5:43 pm I am still unhappy that when I change the enum property, sometimes the propertyeditor is updated (with the newly hidden properties) and sometimes it does not. I have to keep investigating this.
This is a wild guess but maybe you need to change the inmutable state first before unhiding it? I'm not really sure sure how these things work.

Also, are the Size/Size2/Angle properties marked as touched when you call updateProperties()? If not, mustExecute() should return 1 if chamferType is touched. Sorry if I'm saying dumb things but I'm trying to understand all this code for possible future work.

Why is DressUp::onChanged called anyway if the property change is handled already?

Code: Select all

void Chamfer::onChanged(const App::Property* prop)
{
    if (prop == &ChamferType) {
        updateProperties();
    }

    DressUp::onChanged(prop);
}
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

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

Post by adrianinsaval »

armandas wrote:
https://github.com/armandas/FreeCAD/blo ... r.cpp#L136
Here Size is not checked to be >0. I don't know if its really necessary but its checked in the other cases.
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Post by armandas »

abdullah wrote: Thu May 14, 2020 2:07 pm 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:
The number of silly mistakes I've made so far is stunning :D Thank you again!
abdullah wrote: Thu May 14, 2020 5:47 pm One thing, in void TaskChamferParameters::apply(), at the moment you are applying all the properties, regardless of the enum mode. Only the properties of the mode should be updated.

If you decide to integrate the code I posted above for auto-hidding, the properties not belonging to the enum are set read-only (so that they do not cause the object to be touched and if somebody tries to change them from the Python console, that he or she gets an error. Better an error that unexpected behaviour). The apply to all properties will fail for those that are read-only according to the mode.
You mentioned that you're not 100% happy with that change yet. How should we proceed? If you want to add those changes, could I give you access to my fork and you commit the changes directly?
adrianinsaval wrote: Thu May 14, 2020 11:58 pm Here Size is not checked to be >0. I don't know if its really necessary but its checked in the other cases.
Good catch, thank you! I updated the parameter validation code.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

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

Post by abdullah »

adrianinsaval wrote: Thu May 14, 2020 6:58 pm This is a wild guess but maybe you need to change the inmutable state first before unhiding it? I'm not really sure sure how these things work.
I am now in the middle of a massive debugging to try to understand what is happening. When the flags of a property are changed, it ends up here:

Code: Select all

void PropertyEditor::updateEditorMode(const App::Property& prop)
{
    // check if the parent object is selected
    std::string editor = prop.getEditorName();
    if (!PropertyView::showAll() && editor.empty())
        return;

    bool hidden = PropertyView::isPropertyHidden(&prop);
    bool readOnly = prop.testStatus(App::Property::ReadOnly);

    int column = 1;
    int numRows = propertyModel->rowCount();
    for (int i=0; i<numRows; i++) {
        QModelIndex item = propertyModel->index(i, column);
        PropertyItem* propItem = static_cast<PropertyItem*>(item.internalPointer());
        if (propItem && propItem->hasProperty(&prop)) {
            setRowHidden (i, QModelIndex(), hidden);

            propItem->updateData();
            if (item.isValid()) {
                updateItemEditor(!readOnly, column, item);
                dataChanged(item, item);
            }
            break;
        }
    }
}
However, the propertyModel only has the properties that were visible when the selection object was attached. This means that any property that was visible when attached, will be hidden and shown again without changing the selection, but not those that were not visible.

I am trying to see if this behaviour can be adapted.
adrianinsaval wrote: Thu May 14, 2020 6:58 pm Also, are the Size/Size2/Angle properties marked as touched when you call updateProperties()? If not, mustExecute() should return 1 if chamferType is touched. Sorry if I'm saying dumb things but I'm trying to understand all this code for possible future work.
AFAIU the touch system is for when the value changes. It relates to recomputing. When a property flags are changed, we do not need a recompute. There is the mechanism above to "handle" the situation.
adrianinsaval wrote: Thu May 14, 2020 6:58 pm Why is DressUp::onChanged called anyway if the property change is handled already?
AFAIU, OnChanged is intended to follow a "chain of responsibility" kind of design pattern. This means that when ChamferType changes, Chamfer is responsible for taking into account the change. Unless there is some kind of shared responsibility, there is indeed no need to call up in the hierarchy if the property is already handled. A return statement should follow updateProperties(). Good eye!
armandas wrote: Fri May 15, 2020 12:36 pm The number of silly mistakes I've made so far is stunning :D Thank you again!
We all make mistakes. It is positive... it is part of the learning process. :)
armandas wrote: Fri May 15, 2020 12:36 pm You mentioned that you're not 100% happy with that change yet. How should we proceed? If you want to add those changes, could I give you access to my fork and you commit the changes directly?
As you wish, but given that it is a very small development, I do not think it is necessary.

If I were you I would just cherry pick that commit from my branch and continue developing. It is faster.
armandas wrote: Fri May 15, 2020 12:36 pm Good catch, thank you! I updated the parameter validation code.
Adrian is a good code reviewer :)
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

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

Post by uwestoehr »

Many thanks for this new feature!

However, there are some issues: -> I'll try to provide a patch

- the angle "0°" is allowed despite it is invalid
- there is no option to flip the chamfer border: Set e.g. a 1mm chamfer and increase the angle to see that of course only at one side the 1mm distance from the edge can be kept. Since it is not clear what side is used for the 1mm, we need an option to flip.
- the angle does not check if the result will be invalid or not: Take e.g. a 5mm cube and add a 80° chamfer to see that this is invalid because this would be a distance of > 5mm. I am not sure this is easy to fix.
- the increment of the angle should be 1° not 0.1°
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

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

Post by adrianinsaval »

abdullah wrote: Fri May 15, 2020 3:13 pm
adrianinsaval wrote: Thu May 14, 2020 6:58 pm Also, are the Size/Size2/Angle properties marked as touched when you call updateProperties()? If not, mustExecute() should return 1 if chamferType is touched. Sorry if I'm saying dumb things but I'm trying to understand all this code for possible future work.
AFAIU the touch system is for when the value changes. It relates to recomputing. When a property flags are changed, we do not need a recompute. There is the mechanism above to "handle" the situation.
I'm not sure what you are saying, let me rephrase my question, when you change chamferType for example from equal distance to two distances, Size2 is touched? Because if it isn't there needs to be code that marks for recompute.
EDIT: nevermind, I didn't see the ChamferType.isTouched() portion of the code :oops: I get it now.
Adrian is a good code reviewer :)
That's what happens when you don't understand crap and you read over and over untill you do :lol:
uwestoehr wrote: Fri May 15, 2020 3:22 pm - there is no option to flip the chamfer border: Set e.g. a 1mm chamfer and increase the angle to see that of course only at one side the 1mm distance from the edge can be kept. Since it is not clear what side is used for the 1mm, we need an option to flip.
I was just going to ask about this, my understanding is that this is what needs a different value to flip the chamfer: https://github.com/armandas/FreeCAD/blo ... r.cpp#L120
- the angle does not check if the result will be invalid or not: Take e.g. a 5mm cube and add a 80° chamfer to see that this is invalid because this would be a distance of > 5mm. I am not sure this is easy to fix.
Neither does equal distance or two distances AFAIK, I don't think this is really a problem, there's a check in case OCC fails.
Last edited by adrianinsaval on Fri May 15, 2020 6:35 pm, edited 1 time in total.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

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

Post by abdullah »

Peek 15-05-2020 19-40.gif
Peek 15-05-2020 19-40.gif (127.76 KiB) Viewed 2467 times
Much better!

I want to improve the code though...
armandas
Posts: 25
Joined: Thu Dec 12, 2013 9:08 pm

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

Post by armandas »

uwestoehr wrote: Fri May 15, 2020 3:22 pm - there is no option to flip the chamfer border: Set e.g. a 1mm chamfer and increase the angle to see that of course only at one side the 1mm distance
Very good point! I have no idea how to implement that, but I thought about the UI. I added a toggle button, that could be disabled in the Equal distances case. What do you think about this?

Test branch: https://github.com/armandas/FreeCAD/com ... tionButton

Also, while we're talking about UI, any idea how to align the input fields? It looks a bit messy now.
Attachments
Flip_Direction_Mockup.png
Flip_Direction_Mockup.png (16.04 KiB) Viewed 2404 times
Last edited by armandas on Sat May 16, 2020 1:45 pm, edited 2 times in total.
Post Reply