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

[SofaCore] Add new (simpler) DataEngine implementation #760

Merged
merged 7 commits into from Oct 11, 2018

Conversation

@marques-bruno
Copy link
Member

marques-bruno commented Aug 30, 2018

Here's a simpler implementation of sofa::core::DataEngine. This implementation hides the code related to updating datafields and calling cleanDirty() in an attempt to reduce human errors.


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.

@marques-bruno marques-bruno self-assigned this Aug 30, 2018

@guparan guparan changed the title [core][DataEngine] ADD: New (simpler) DataEngine implementation [SofaCore] Add new (simpler) DataEngine implementation Aug 30, 2018

@epernod
Copy link
Contributor

epernod left a comment

as it is more the behavior we want, maybe one of the current engine could already become a simpleDataEngine?
As an example and it would go through the CI tests

Show resolved Hide resolved SofaKernel/framework/framework_test/core/DataEngine_test.cpp
/// Automatically adds the input fields to the datatracker
void addInput(sofa::core::objectmodel::BaseData* data)
{
m_dataTracker.trackData(*data);

This comment has been minimized.

@epernod

epernod Sep 3, 2018

Contributor

I'm not use to the dataTracker, if the data are added in the dataTracker, is it not possible to use it in the update method to check the data instead of calling the updateIfDirty?

This comment has been minimized.

@marques-bruno

marques-bruno Sep 4, 2018

Author Member

The datatracker is not influenced by the updateIfDirty() call. You can absolutely use the datatracker in the update() method if needed (to check which inputs have been changed, and to specific actions for each dirty data. The call to UpdateIfDirty() simply guarantees that all data have been accessed (thus updated upstream) before calling cleanDirty().

This comment has been minimized.

@epernod

epernod Sep 4, 2018

Contributor

ok so the dataTracker just return dirty if on of its Data is dirty?

This comment has been minimized.

@marques-bruno

marques-bruno Sep 4, 2018

Author Member

yes, the dataTracker is not at all used in the internal mechanism of the DataEngine class actually. It's a helper object (one instance per dataEngine) that tracks data. It has a very simple interface, and you can just call
trackData(BaseData*) << sets a datafield in its map. for now on, its counter will be tracked
clean() / clean(BaseData*) << resets the data's counter
isDirty() : isDirty() << checks if the counter is up to date

You can then use this in your update() ( or doUpdate() now) method to see if a specific datafield is dirty or not.

@marques-bruno

This comment has been minimized.

Copy link
Member Author

marques-bruno commented Sep 4, 2018

@epernod Good idea. I would suggest the TransformEngine, since it's a very basic engine (the most used also) so the feature would be more visible to users with minimal code change.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Sep 14, 2018

This PR was set to ready too soon. DataEngine_test doesn't compile ^^

@marques-bruno

This comment has been minimized.

Copy link
Member Author

marques-bruno commented Sep 24, 2018

Waiting for CI but I think it's better now:
used enable_if<is_base_of> to compile or not the doUpdate / update methods in the test. should be clean enough

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Sep 24, 2018

CI was missing disk space. I relaunch ;)
[ci-build][with-scene-tests]

@marques-bruno

This comment has been minimized.

Copy link
Member Author

marques-bruno commented Sep 25, 2018

alright, that should fix the failing test. Although, the test doesn't reflect very well the behavior of the component. the SimpleDataEngine must exclusively be used in cases where the process performed the engine has to be applied no matter which datafield is set to dirty.
Currently, the engine would even be called if NO data is set to dirty, which is also the case in the core::DataEngine if there's no check performed on the DataTracker. I believe this is a mistake. I think that doUpdate() should be called iff at least 1 data field is dirty.
What do you think?

concretely, it means that in DataEngine_test.cpp, line 151 would assert FALSE

@marques-bruno

This comment has been minimized.

Copy link
Member Author

marques-bruno commented Sep 26, 2018

Following our discussions:
SimpleDataEngine's update method now looks like this:

void update() final
{
   updateAllInputs() // a method that calls `updateIfDirty()` on all inputs: can be overridden, but rarely necessary
   DDGNode::cleanDirty() // same as cleanDirty(), but does NOT call m_datatracker->clean() so counters are still valid after
   doUpdate() // actual magic
   m_datatracker->clean() // cleaning the counters
}

To my understanding, DataTrackerDDGNode's cleanDirty() method becomes useless in DataEngines at this point, making the workflow much smoother for the user's impl of doUpdate().

I believe there would be no side effects (apart from being BRAKING...) if we would replace core::DataEngine with core::SimpleDataEngine, and rename all engine's udpate() methods to doUpdate()

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Oct 10, 2018

What about ?

void update() final
{
   updateAllInputs(); // a method that calls `updateIfDirty()` on all inputs: can be overridden, but rarely necessary
   cleanAllInputsDirty(); // same as cleanDirty(), but does NOT call m_datatracker->clean() so counters are still valid after
   doUpdate() // actual magic
   cleanAllInputsCounter() // cleaning the counters
}

@guparan guparan added status: wip and removed status: ready labels Oct 10, 2018

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Oct 10, 2018

I'm rebasing this before merge ;-)

@guparan guparan force-pushed the mimesis-inria:simpleDataEngine branch from d313293 to 87dab0b Oct 11, 2018

@guparan guparan added status: ready and removed status: wip labels Oct 11, 2018

@guparan guparan merged commit cf0af8f into sofa-framework:master Oct 11, 2018

6 checks passed

Jk2/Dashboard Builds triggered.
Details
Jk2/Scene tests Triggered in latest build.
Details
Jk2/centos_clang-3.4_options Build OK. FIXME: 0 unit tests
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 1 unit tests
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 0 scene tests
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 2 unit tests, 1 scene tests
Details

@marques-bruno marques-bruno deleted the mimesis-inria:simpleDataEngine branch Oct 23, 2018

@guparan guparan added this to the v18.12 milestone Oct 26, 2018

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