sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
934 stars 312 forks source link

BoxROI and doUpdate not working as I expect #38

Open damienmarchal opened 8 years ago

damienmarchal commented 8 years ago

Whatever I set doUpdate to true or false the BoxROI object seems to update the selected triangles so I wonder what is supposed to be the behavior of this data field ?

hugtalbot commented 8 years ago

From what I know, it is supposed to tell if the indices included in the BoxROI must be updated at each time step or not. So it's not the expected behavior.

JeremieA commented 8 years ago

I believe it's a hack that is probably no longer required. It was forcing an update at each step, probably from an ancien time when this code was not an Engine or when Engines were not implemented correctly. Now the Engine is automatically updated when someone read its output and any input changed so that this flag should no longer make any difference.

damienmarchal commented 8 years ago

So I deprecate it :)

hugtalbot commented 8 years ago

Would it not make sense to keep a boolean allowing not to recompute the selection within the ROI at each time steps ? For efficiency purposes

damienmarchal commented 8 years ago

I agree with Hugo this make sense to be able to control if the update happens or not.

So I finally fixed the doUpdate behavior. If you want to have a look at this happens in commit: https://github.com/sofa-framework/sofa/commit/111e21cc23a76c7d3c0844ce7b64aa81e381d614

JeremieA commented 8 years ago

I don't agree, the correct way to have BoxROI not update itself, like any other engine, is to link it to inputs that are not changing, i.e. rest_position instead of position. Otherwise this will break updates after topological changes for instance...

damienmarchal commented 8 years ago

Thanks for the feedbacks,

This seems related to the fact that Engines are designed to be data flow oriented and, keeping that in mind, we shouldn't break the general design of engines.

But, it also appears that in several scenarios, ROIs are used to select features, eg 'position', but not necessarily at initialization time and not necessarily at rest_position and that there may not have an associated 'not changing' data field. My impression is that being able to pick a set of features from data field in a quick way, at controlled instant, is as desirable as not breaking the Engine logics.

What do you think about refactoring that and separate the two behaviors in different components. BoxROISelector and BoxROIEngine ?

DM.

JeremieA commented 8 years ago

It's something that is too general to make it specific to BoxROI. What about SphereROI, EllipsoidROI, ... ? There are several ways to handle this use case (using an Engine to compute something once or only at specific triggers):

  1. don't use a link but copy the data manually (but easily) using python for instance
  2. add a new link syntax that would not automatically trigger updates, or that would break itself after the first update / the init is done (which is a common request for use-cases for computing initial positions for instance)
  3. add a new component that can be used instead of a Link for use cases where we want more control

In our codes we use the third option, we added a trivial component that we call ValueStore, which has a Data value inside with the ability to copy the value (and implicitly request the update of an Engine linked to it) to another Data manually (but as efficiently as a Link) at specific times (currently after each time step, but it could be customised).

hugtalbot commented 8 years ago

Ok, I am no expert in design and I did not understand this data flow meaning of Engines. Well, third option is ok, and the first one is yet basic but more efficient. Thank you for the discussion.

damienmarchal commented 8 years ago

@hugtalbot as you asked: https://en.wikipedia.org/wiki/Dataflow_programming :)