Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SofaKernel] Refactor the MutationListener #917

Merged
merged 43 commits into from
Apr 17, 2019
Merged

[SofaKernel] Refactor the MutationListener #917

merged 43 commits into from
Apr 17, 2019

Conversation

marques-bruno
Copy link
Member

@marques-bruno marques-bruno commented Feb 5, 2019 •
edited
Loading

The MutationListener in Sofa is used to notify (A GUI for instance) of the SceneGraph's edition (addition / deletion / move of nodes, objects, or slaves)
The MutationListener itself does not perform anything, but once inherited, its methods can be overloaded to perform operations on a ListView widget for instance, as does the GrapListenerQListView, in applications/sofa/gui/qt.

The current implementation stores a pointer to every registered listeners in every node in the scene graph. Instead, the implementation proposed in this PR registers the listeners in the root node and child nodes forward the notifications to it.

Also, the overloads are currently relying on the call super (the child class reimplements the method, then calls its base class implementation) This approach is known to be error prone and can be reimplemented using delegates instead.

Finally, this PR also adds extra delegates in the Nodes listeners notifications system and in the MutationListener, called AFTER the graph's edition. those are used in SofaQtQuick to be notified that the graph has indeed been updated.

Here are 2 quick UML diagrams showing the 2 architecture's differences. This one is the current implementation in sofa:
code_smell

And this one is what this PR does:
code_clean

Also, if someone could tell me what the master / slave relationship between BaseObjects are, I'd be curious to know if it is still in use in SOFA. It doesn't seem to be used in Listeners.. maybe added for Compliant / Flexible?

This PR could potentially be breaking for plugins & projects implementing their own MutationListeners

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
[SofaKernel] Refactor the MutationListener, & Node's listerner's noti…
b76c61e 
…fication system

- Replace call super with delegates
- Centralise Listeners in root node
- Forward notifications to root node
- CLEAN affected files (removing C-style casts, zero as nullptr constants, move class declarations in .h, definitions in .cpp...)
@marques-bruno marques-bruno added enhancement About a possible enhancement pr: breaking Change possibly inducing a compilation error pr: status to review To notify reviewers to review this pull-request pr: clean Cleaning the code labels Feb 5, 2019
@marques-bruno marques-bruno self-assigned this Feb 5, 2019
@marques-bruno marques-bruno added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Feb 5, 2019
@marques-bruno
[SofaKernel] Various improvements to MutationListener's API
e2542f4 
- Transmitting notifications to root through getRootContext instead of recursion
- Better methods naming (Begin / End instead of doXX / XXXXDone)
- Better refactoring of Node / DDGNode / GNode's graph edit methods
- more clean
@marques-bruno
Copy link
Member Author

[ci-build][with-scene-tests][with-regression-tests]

@marques-bruno marques-bruno added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Feb 5, 2019
@marques-bruno
Copy link
Member Author

[ci-build][with-scene-tests][with-regression-tests]

@damienmarchal
Copy link
Contributor

Thanks Bruno,

To me the pro in this PR:

  • the design seems clearer with the use of final and the delegates.
  • more efficient (no listener in each Nodes, no need to maintaint).
  • more consistent with begin/end notifications.

The bad:

  • no backward compatibility (can one be implemented ?), but this listener are not used so much (only in GUI).

@marques-bruno
Copy link
Member Author

Concerning backward compatibility:

  • The MutationListener is inherited by only 2 classes in the SOFA repository, if we exclude SofaQtQuick for which this PR is profitable anyway:

    • GraphListenerQListView, which is used in runSofa to display the scene graph
    • ChangeListener: which is an unused & unimplemented class hanging around in SofaKernel/framework/sofa/simulation and should be removed.

  • Nodes and its child classes (DAGNode & GNode in SOFA, FailNode in Compliant) are the only ones affected by the refactoring, and there's probably not much code inheriting those classes & modifying their notification behavior.

So While we can't guarantee that nobody rewrote a GUI using the MutationListener, IMHO we should be able to provide support through the forum / gitter channels if that breaks anything to anyone.
Let's be breaking ;)

marques-bruno and others added 2 commits February 11, 2019 21:21
@marques-bruno @damienmarchal
Merging master (#22)
e5540a4 
* [CMake] SofaMacros.cmake: deprecating sofa_create_package in favor of sofa_generate_package (Named arguments)

* FIX compilation failures on some systems caused by improper paths to CImg

* [SofaMacros] Better warning message

* FIX: moving INCLUDES from optional to oneValueArgs

* Redirecting from sofa_create_package to sofa_generate_package & renaming INCLUDES to INCLUDE_ROOT_DIR

* Fix missing variable names in sofa_create_package

* [examples] Remove scenes about deprecated components

* [Changelog] Update deprecated components

* [examples] Remove scene using deprecated WashingMachineForceField

* [SofaBaseMechanics] removed a mistakenly introduced return

* [examples] Ignore scenes with removed components
@marques-bruno
Merge branch 'master' into clean_mutationListener
ce5ca0c
@marques-bruno
Copy link
Member Author

[ci-build][with-scene-tests][with-regression-tests]

@guparan guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Feb 13, 2019
@damienmarchal
Copy link
Contributor

Hi Bruno

I forgot to say that few weeks ago during the meeting was asked why not naming "doXXX" the methods to override to receive event. I think there is a good reason for that...it is because those are not delegates...those are more like "events".

So to me I expect this convention:

  • doXXXX when there is an XXX method called delegating some part to the an inherited class
  • onXXXX to do something when the XXX "event is received.

I'm sorry to remind me that so late as I see you have changes all the names in your PR.

@marques-bruno marques-bruno added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Mar 20, 2019
@marques-bruno
Copy link
Member Author

[ci-build]

guparan
guparan reviewed Mar 20, 2019
View reviewed changes
SofaKernel/framework/sofa/simulation/Node.h

virtual void notifyEndAddChild(Node::SPtr parent, Node::SPtr child);
virtual void notifyEndRemoveChild(Node::SPtr parent, Node::SPtr child);
Copy link
Contributor

Choose a reason for hiding this comment

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

notify*End?

SofaKernel/framework/sofa/simulation/Node.h

virtual void notifyEndAddObject(Node::SPtr parent, sofa::core::objectmodel::BaseObject::SPtr obj);
virtual void notifyEndRemoveObject(Node::SPtr parent, sofa::core::objectmodel::BaseObject::SPtr obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

notify*End?

SofaKernel/framework/sofa/simulation/Node.h
virtual void notifySleepChanged(Node* node);

virtual void notifyBeginAddSlave(sofa::core::objectmodel::BaseObject* master, sofa::core::objectmodel::BaseObject* slave);
virtual void notifyBeginRemoveSlave(sofa::core::objectmodel::BaseObject* master, sofa::core::objectmodel::BaseObject* slave);
Copy link
Contributor

Choose a reason for hiding this comment

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

notify*Begin?

SofaKernel/framework/sofa/simulation/Node.h
virtual void notifyBeginRemoveSlave(sofa::core::objectmodel::BaseObject* master, sofa::core::objectmodel::BaseObject* slave);

virtual void notifyEndAddSlave(sofa::core::objectmodel::BaseObject* master, sofa::core::objectmodel::BaseObject* slave);
virtual void notifyEndRemoveSlave(sofa::core::objectmodel::BaseObject* master, sofa::core::objectmodel::BaseObject* slave);
Copy link
Contributor

Choose a reason for hiding this comment

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

notify*End?

SofaKernel/framework/sofa/simulation/Node.h
virtual void notifyMoveObject(sofa::core::objectmodel::BaseObject::SPtr obj, Node* prev);
virtual void notifySleepChanged();
virtual void notifyBeginAddObject(Node::SPtr parent, sofa::core::objectmodel::BaseObject::SPtr obj);
virtual void notifyBeginRemoveObject(Node::SPtr parent, sofa::core::objectmodel::BaseObject::SPtr obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

notify*Begin?

SofaKernel/framework/sofa/simulation/Node.h

std::stack<Visitor*> actionStack;
private:
virtual void notifyBeginAddChild(Node::SPtr parent, Node::SPtr child);
virtual void notifyBeginRemoveChild(Node::SPtr parent, Node::SPtr child);
Copy link
Contributor

Choose a reason for hiding this comment

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

notify*Begin?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized I went for the other way around, notify{Add,Remove,Move}XXX{Begin,End} instead of notify{Begin,End}{Add,Remove,Move}XXX. I guess as long as it's uniform throughout the code base, it should be alright.. If that's OK for you

@guparan
Copy link
Contributor

guparan commented Mar 27, 2019

[ci-build][with-all-tests]

@guparan
Copy link
Contributor

guparan commented Mar 27, 2019

I just had to manually kill Regression_test that was stuck on all VMs for this PR.
See output of:
Regression_test/StateRegression_test.sceneTest/collisionMultiple
Regression_test/StateRegression_test.sceneTest/BilateralInteractionConstraint
Regression_test/StateRegression_test.sceneTest/RuleBasedContactManager

@guparan guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Mar 27, 2019
@marques-bruno
Copy link
Member Author

[ci-build][with-all-tests]

@marques-bruno marques-bruno added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 2, 2019
@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Apr 3, 2019
@marques-bruno marques-bruno added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 11, 2019
@hugtalbot
Copy link
Contributor

Conflicting file : DAGNode

@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 17, 2019
@epernod epernod merged commit 26f0baa into sofa-framework:master Apr 17, 2019
@marques-bruno marques-bruno mentioned this pull request Apr 23, 2019
Merged
6 tasks
@guparan guparan added this to the v19.06 milestone Jun 20, 2019
alxbilger pushed a commit to alxbilger/sofa that referenced this pull request Oct 27, 2023
@epernod
Merge pull request sofa-framework#917 from SofaDefrost/clean_mutation…
a5efdf4 
…Listener

[SofaKernel] Refactor the MutationListener
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guparan guparan guparan left review comments

@damienmarchal damienmarchal Awaiting requested review from damienmarchal

Assignees

@marques-bruno marques-bruno

Labels
enhancement About a possible enhancement pr: breaking Change possibly inducing a compilation error pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v19.06
Development

Successfully merging this pull request may close these issues.
5 participants
@marques-bruno @damienmarchal @guparan @hugtalbot @epernod