PR #3456 - Adding chamfer angle field to PartDesign
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
Re: PR #3456 - Adding chamfer angle field to PartDesign
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: 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.
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: 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.
Re: PR #3456 - Adding chamfer angle field to PartDesign
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.
- adrianinsaval
- Veteran
- Posts: 5541
- Joined: Thu Apr 05, 2018 5:15 pm
Re: PR #3456 - Adding chamfer angle field to PartDesign
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);
}
- adrianinsaval
- Veteran
- Posts: 5541
- Joined: Thu Apr 05, 2018 5:15 pm
Re: PR #3456 - Adding chamfer angle field to PartDesign
https://github.com/armandas/FreeCAD/blo ... r.cpp#L136armandas wrote:
Here Size is not checked to be >0. I don't know if its really necessary but its checked in the other cases.
Re: PR #3456 - Adding chamfer angle field to PartDesign
The number of silly mistakes I've made so far is stunning Thank you again!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:
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?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.
Good catch, thank you! I updated the parameter validation code.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.
Re: PR #3456 - Adding chamfer angle field to PartDesign
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: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.
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;
}
}
}
I am trying to see if this behaviour can be adapted.
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 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, 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!adrianinsaval wrote: ↑Thu May 14, 2020 6:58 pm Why is DressUp::onChanged called anyway if the property change is handled already?
We all make mistakes. It is positive... it is part of the learning process.
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.
Adrian is a good code reviewer
Re: PR #3456 - Adding chamfer angle field to PartDesign
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°
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°
- adrianinsaval
- Veteran
- Posts: 5541
- Joined: Thu Apr 05, 2018 5:15 pm
Re: PR #3456 - Adding chamfer angle field to PartDesign
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.abdullah wrote: ↑Fri May 15, 2020 3:13 pmAFAIU 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 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.
EDIT: nevermind, I didn't see the ChamferType.isTouched() portion of the code I get it now.
That's what happens when you don't understand crap and you read over and over untill you doAdrian is a good code reviewer
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#L120uwestoehr 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.
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.- 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.
Last edited by adrianinsaval on Fri May 15, 2020 6:35 pm, edited 1 time in total.
Re: PR #3456 - Adding chamfer angle field to PartDesign
I want to improve the code though...
Re: PR #3456 - Adding chamfer angle field to PartDesign
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 (16.04 KiB) Viewed 2404 times
Last edited by armandas on Sat May 16, 2020 1:45 pm, edited 2 times in total.