Skip to content

Commit

Permalink
[FEM] fix sphere cut filter
Browse files Browse the repository at this point in the history
when using a sphere as function for a filter, the pipeline must be recomputed when the sphere geometry is changed, not only the sphere
  • Loading branch information
donovaly committed Mar 29, 2022
1 parent 5974abe commit 2853f67
Showing 1 changed file with 24 additions and 2 deletions.
26 changes: 24 additions & 2 deletions src/Mod/Fem/Gui/ViewProviderFemPostFunction.cpp
Expand Up @@ -576,14 +576,36 @@ void ViewProviderFemPostSphereFunction::updateData(const App::Property* p) {
getManipulator()->setMatrix(t);
}
Gui::ViewProviderDocumentObject::updateData(p);
}

// after updating the geometry we must recompute the pipeline the sphere is in
if (p == &func->Center || p == &func->Radius) {
auto directParents = func->getInList();
// directParents is at the level of the functions container, so we must read the parent of this container
if (!directParents.empty()) {

This comment has been minimized.

Copy link
@wwmayer

wwmayer Mar 29, 2022

Contributor

This extra if condition is superfluous because in case it's empty the for loop doesn't do anything.

This comment has been minimized.

Copy link
@donovaly

donovaly Mar 30, 2022

Author Member

Thanks, fixed

for (auto obj : directParents) {
if (obj->getTypeId() == Base::Type::fromName("Fem::FemPostFunctionProvider")) {
auto outerParents = obj->getInList();
if (!outerParents.empty()) {

This comment has been minimized.

Copy link
@wwmayer

wwmayer Mar 29, 2022

Contributor

Same here

for (auto objOuter : outerParents) {
if (objOuter->getTypeId() == Base::Type::fromName("Fem::FemPostPipeline") ) {
if (!isDragging())
// not recursve, otherwise VTK will show an error on initialization
objOuter->recomputeFeature();
else
objOuter->recomputeFeature(true);
}
}
}
}
}
}
}
}

FunctionWidget* ViewProviderFemPostSphereFunction::createControlWidget() {
return new SphereWidget();
}


SphereWidget::SphereWidget() {

ui = new Ui_SphereWidget();
Expand Down

6 comments on commit 2853f67

@ickby
Copy link
Contributor

@ickby ickby commented on 2853f67 Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the added logic is required? It seems very strange to me.

  • The function updateData is called after a property of the Viewproviders base DocumentObject was changed. In this function it should only take actions to visualize this change
  • As a property of the base object was changed, the full pipeline recompute is to be handled by the default FreeCAD dependency recomputation logic. It must not be triggered by the viewprovider, but freecad should auomatically detect and recompute all dependencies

Hence if there is a problem with the update of the pipeline, one needs to look into the base object and it interaction with the dependency re-computation logic.

Notes:

  • the very same behavior is implemented for the plane function. Does this work correctly for you? Would not make much sense to change sphere and not plane in behavior
  • The VTK pipeline handling has a special option that allows to enable live updates or updates on manual recompute only. This needs to be respected wiki

@donovaly
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* The function updateData is called after a property of the Viewproviders  base DocumentObject was changed. In this function it should only take actions to visualize this change

But this is the aim. Here is the bug I fixed:
FC 0.19:
FreeCAD_eyZVmrcF4c

So nothing happened, the whole sphere manipulator object was useless/had no effect.

Now, with my fix:
FreeCAD_UGpp5R3RAr

* As a property of the base object was changed, the full pipeline recompute is to be handled by the default FreeCAD dependency recomputation logic. It must not be triggered by the viewprovider, but freecad should auomatically detect and recompute all dependencies

I already fixed some pipeline bugs, but there are simply too many. Here just a list from my last hour of testing:
https://forum.freecadweb.org/viewtopic.php?f=18&t=67579&p=584217#p584217

And this is only the Clip filter.

What I can tell so far is that the recomputation handling in pipelines has many bugs. Every fix is welcome.

* the very same behavior is implemented for the plane function. Does this work correctly for you?

Plane is broken and I could not yet find time to fix it. For example its manipulator is not visible. So dragging like with the sphere is not possible.:

Would not make much sense to change sphere and not plane in behavior

It does, because now at least the sphere is working at it should ;-) Plane needs to fixed in further steps. But I am alone and my time is limited.

* The VTK pipeline handling has a special option that allows to enable live updates or updates on manual recompute only. This needs to be respected [wiki](https://wiki.freecadweb.org/FEM_PostApplyChanges)

? you linked an empty Wiki page.

@ickby
Copy link
Contributor

@ickby ickby commented on 2853f67 Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the forum link. I'm going to have a look this evening and try to support you on your bug fixing effort.

"? you linked an empty Wiki page." --> Yes, sorry. The idea behind this tool is to allow for delayed filter compute. So if "apply changes" is activated, any change in the E.g. sphere should directly be processed by the filters and the mesh output changes. If it is not activated, one should be able to change the sphere and the filters do not process it directly. The reason behind this implementation is the possibility of very large results, where the filter processing is slow. For this case having a direct update is ultra annoying as it makes dragging almost impossible. So the user should be able to choose the behavior he wants on a global scale.

I will post back in the forum after I looked into it later.

@wwmayer
Copy link
Contributor

@wwmayer wwmayer commented on 2853f67 Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, with my fix:

When I open the test project from the forum, select the sphere as function for the Clip001 object and drag the sphere then nothing happens after releasing it.
It still have to always manually press the refresh button to update the visualization.

EDIT: Found the problem. I have to activate the Analysis object first.

@donovaly
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: Found the problem. I have to activate the Analysis object first.

Yes, this is a requirement for many FEM objects

@donovaly
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind this tool is to allow for delayed filter compute. So if "apply changes" is activated,

I did not change this. For the manipulators a delayed computation is not sensible. I mean when I want to adjust the sphere and later want to make the cut, we have the sphere settings dialog. There I can adjust, then click on "Apply". But for the manipulators, I want to see the change immediately. One needs to use it so set the sphere that I see what I need to see.

Can you please add something to the Wiki that users know about the Apply feature? (I can format the Wiki for you if you like, just add the info.)

Concerning the Plane function, for my use cases, I would need to set the cutting plane using the manipulators like I can do for the https://wiki.freecadweb.org/FEM_ClippingPlaneAdd feature. This is the most intuitive and quickest way to define a cut to see the detail one needs.
Therefore, if you have time, and could have a look at the Plane function to get for its the manipulator working the way it works for FEM_ClippingPlaneAdd and the sphere, this would be great. I see there some manipulator code but it has no visible effect.

Please sign in to comment.