Skip to content

Commit

Permalink
[FEM] fix direction handling in Force dialog
Browse files Browse the repository at this point in the history
- the direction handling did not work because of yesterdays' PR from me
  • Loading branch information
donovaly authored and berndhahnebach committed Feb 21, 2020
1 parent 361acd5 commit 6cc0e35
Showing 1 changed file with 64 additions and 7 deletions.
71 changes: 64 additions & 7 deletions src/Mod/Fem/Gui/TaskFemConstraintForce.cpp
Expand Up @@ -86,6 +86,8 @@ TaskFemConstraintForce::TaskFemConstraintForce(ViewProviderFemConstraintForce *C
this, SLOT(onButtonDirection()));
connect(ui->checkReverse, SIGNAL(toggled(bool)),
this, SLOT(onCheckReverse(bool)));
connect(ui->listReferences, SIGNAL(itemClicked(QListWidgetItem*)),
this, SLOT(setSelection(QListWidgetItem*)));

this->groupLayout()->addWidget(proxy);

Expand Down Expand Up @@ -284,14 +286,69 @@ void TaskFemConstraintForce::onReferenceDeleted() {
TaskFemConstraintForce::removeFromSelection(); //OvG: On right-click face is automatically selected, so just remove
}

void TaskFemConstraintForce::onButtonDirection(const bool pressed) {
if (pressed) {
selectionMode = seldir;
} else {
selectionMode = selnone;
void TaskFemConstraintForce::onButtonDirection(const bool pressed)
{
// sets the normal vector of the currently selecteed planar face as direction

//get vector of selected objects of active document
std::vector<Gui::SelectionObject> selection = Gui::Selection().getSelectionEx();
if (selection.size() == 0) {
QMessageBox::warning(this, tr("Selection error"), tr("Nothing selected!"));
return;
}
ui->buttonDirection->setChecked(pressed);
Gui::Selection().clearSelection();
Fem::ConstraintForce* pcConstraint = static_cast<Fem::ConstraintForce*>(ConstraintView->getObject());

// we only handle the first selected object
std::vector<Gui::SelectionObject>::iterator selectionElement = selection.begin();
std::string TypeName = static_cast<std::string>(selectionElement->getTypeName());

// we can only handle part objects
if (TypeName.substr(0, 4).compare(std::string("Part")) != 0) {

This comment has been minimized.

Copy link
@wwmayer

wwmayer Feb 22, 2020

Contributor

Checking the type of a feature this way is quite bad because it completely fails for Part features coming from other modules. If e.g. the FEM module has a feature class derived from Part::Feature this check will ignore it.
And even worse is that an object is from the Part module but does not inherit from Part::Feature. Then you will perform an insecure static_cast and get an invalid pointer.

To avoid such problems either use the isDerivedFrom(Part::Feature::getClassTypeId()) function followed by a static_cast or do a dyanmic_cast and check the pointer afterwards whether it's null or not.

This comment has been minimized.

Copy link
@donovaly

donovaly Feb 22, 2020

Member

many thanks for your review! I need to learn. Since I am not yet familiar with the code, I just used what was already there. And if (***.substr(0, 4).compare(std::string("Part")) != 0) { was already used.

In #3088 qingfengxia mentioned this as well. I run some tests but could of course not see this way the issue with Part::Feature.

The issue you found applies for all FEM constraint dialogs because this insecure check is used in the 'addToSelection' routine.

This comment has been minimized.

Copy link
@donovaly

donovaly Feb 23, 2020

Member

To avoid such problems either use the isDerivedFrom(Part::Feature::getClassTypeId()) function followed by a static_cast or do a dyanmic_cast and check the pointer afterwards whether it's null or not.

I don't know why I must cast. I need to find out if the selection is a face/edge/vertex of a Part or PartDesign object. Therefore I tried this:
// we can only handle part objects if (!selectionElement->getTypeId().isDerivedFrom(Part::Feature::getClassTypeId())) { QMessageBox::warning(this, tr("Selection error"), tr("Selected object is not a part!")); return; }

But selections of a PartDesign object trigger this too. As I understood it, FEM can handle Part and PartDesign features.
So I need your help to understand and find a suitable solution.

QMessageBox::warning(this, tr("Selection error"), tr("Selected object is not a part!"));
return;
}
// get the names of the subobjects
std::vector<std::string> subNames = selectionElement->getSubNames();

if (subNames.size() > 1) {
QMessageBox::warning(this, tr("Selection error"), tr("Only one planar face or edge can be selected!"));
return;
}
// we are now sure we only have one object
std::string subNamesElement = subNames[0];
// vector for the direction
std::vector<std::string> direction(1, subNamesElement);

App::DocumentObject* obj = ConstraintView->getObject()->getDocument()->getObject(selectionElement->getFeatName());
Part::Feature* feat = static_cast<Part::Feature*>(obj);
TopoDS_Shape ref = feat->Shape.getShape().getSubShape(subNamesElement.c_str());

if (TypeName.substr(0, 4).compare(std::string("Part")) != 0) {

This comment has been minimized.

Copy link
@wwmayer

wwmayer Feb 22, 2020

Contributor

@donovaly this check is obsolete because a few lines above you have the exact same check.

This comment has been minimized.

Copy link
@donovaly

donovaly Feb 22, 2020

Member

Sorry.

This comment has been minimized.

Copy link
@wwmayer

wwmayer Feb 26, 2020

Contributor

See 1b57cd3

This comment has been minimized.

Copy link
@donovaly

donovaly Feb 26, 2020

Member

Many thanks. Since this issue affects most FEM constraint dialogs, I'll make a PR to spread your solution to the other dialogs.

This comment has been minimized.

Copy link
@donovaly

donovaly Feb 28, 2020

Member

Since this issue affects most FEM constraint dialogs, I'll make a PR to spread your solution to the other dialogs.

Here it is: #3113

QMessageBox::warning(this, tr("Selection error"), tr("Selected object is not a part!"));
return;
}
if (subNamesElement.substr(0, 4) == "Face") {
if (!Fem::Tools::isPlanar(TopoDS::Face(ref))) {
QMessageBox::warning(this, tr("Selection error"), tr("Only planar faces can be picked for 3D"));
return;
}
}
else if (subNamesElement.substr(0, 4) == "Edge") { // 2D or 3D can use edge as direction vector
if (!Fem::Tools::isLinear(TopoDS::Edge(ref))) {
QMessageBox::warning(this, tr("Selection error"), tr("Only planar edges can be picked for 2D"));
return;
}
}
else {
QMessageBox::warning(this, tr("Selection error"), tr("Only faces for 3D part or edges for 2D can be picked"));
return;
}
// update the direction
pcConstraint->Direction.setValue(obj, direction);
ui->lineDirection->setText(makeRefText(obj, subNamesElement));

//Update UI
updateUI();
}

void TaskFemConstraintForce::onCheckReverse(const bool pressed)
Expand Down

0 comments on commit 6cc0e35

Please sign in to comment.