New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataUpdate] ADD: necessary State Setting mechanisms #823

Open
wants to merge 10 commits into
base: master
from

Conversation

3 participants
@marques-bruno
Copy link
Contributor

marques-bruno commented Nov 9, 2018

See ComponentState Diagram here

This PR sets up a Component State Checking mechanism that allows to know if a call to init / reinit is necessary, and whether or not the component took into account any change applied to its data fields.

The idea is that Each component's state can be:

  • Unspecified: The component has just been created, init has not been called
  • Valid: The component is ready to be used, and should behave as expected
  • Dirty: The component is ready to be used, but requires a call to reinit() to update some of its datafields.
  • invalid:
    1. An error happened during init, reinit or any of the methods called during the simulation step. A call to reinit might fix it
    2. A call to reinit has been made, but the method is improperly implemented, such as one or more Data field has not been updated (call to getValue / updateIfDirty etc...)

Because I don't trust programmes to implement the mechanism in their own components, and because it would require all already-implemented component to perform checks in order to get a consistent idea of what's happening, I decided to (try to) embed the state setting mechanisms inside the simulation:

  • setDirtyValue() on a data sets the dirty state on its owner
  • cleanDirty() on a data looks at all its owner's data fields and if they're all clean, sets the Valid state on it
  • initVisitor() / init / bwdInit python binding uses the MessageHandler API to verify that no msg_error has been triggered, and sets either the Valid or Invalid state on the component
  • reinit pythonBinding / the Qt GUI's ModifyObject calls reinit, and that call is wrapped the same MessageHandling mechanism to set or unset the Valid / Invalid flag

LIMITATIONS
There is a weak link in this approach:
reinit() will NOT unset the Invalid flag from a component if it is called from elsewhere than the GUI's ModifyObject api. This can happen often: when calling reinit from a c++ program that doesn't use runSofa's GUI for instance.
In order to guarantee that the state is set properly on reinit(), reinit should be a delegate method nested in a method performing the check on the msg_error, and setting the Valid / Invalid flag appropriately. This would be a VERY breaking, and time consuming task. calling the state updating mechanisms from the python bindings, visitors & GUI is kind of QuickNDirty but should reveal all components that aren't implementing properly their init / reinit / bwdInit methods properly, and show at the component level a hint at how often components stay in a "Dirty' state

TODO:

  • Create a StateCheckerVisitor and run it at the end of the DefaultAnimationLoop's step function
  • Use the StateCheckerVisitor to either
    • show a warning message if a component is in an Invalid / Unspecified state
    • run init() / reinit() on the components whose states are invalid or unspecified
    • display a warning icon in runSofa's Scene graph to reflect the component's state

@damienmarchal do you see a non-intrusive solution to the reinit() problem...?


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Nov 21, 2018

I would add the ComponentState as a Data<> but I'm not sure if this is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment