-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaKernel] Refactor the MutationListener #917
[SofaKernel] Refactor the MutationListener #917
Conversation
…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...)
- 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
[ci-build][with-scene-tests][with-regression-tests] |
[ci-build][with-scene-tests][with-regression-tests] |
Thanks Bruno, To me the pro in this PR:
The bad:
|
Concerning backward compatibility:
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. |
* [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
[ci-build][with-scene-tests][with-regression-tests] |
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:
I'm sorry to remind me that so late as I see you have changes all the names in your PR. |
[ci-build] |
|
||
virtual void notifyEndAddChild(Node::SPtr parent, Node::SPtr child); | ||
virtual void notifyEndRemoveChild(Node::SPtr parent, Node::SPtr child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify*End
?
|
||
virtual void notifyEndAddObject(Node::SPtr parent, sofa::core::objectmodel::BaseObject::SPtr obj); | ||
virtual void notifyEndRemoveObject(Node::SPtr parent, sofa::core::objectmodel::BaseObject::SPtr obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify*End
?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify*Begin
?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify*End
?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify*Begin
?
|
||
std::stack<Visitor*> actionStack; | ||
private: | ||
virtual void notifyBeginAddChild(Node::SPtr parent, Node::SPtr child); | ||
virtual void notifyBeginRemoveChild(Node::SPtr parent, Node::SPtr child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify*Begin
?
There was a problem hiding this comment.
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
[ci-build][with-all-tests] |
I just had to manually kill Regression_test that was stuck on all VMs for this PR. |
[ci-build][with-all-tests] |
Conflicting file : DAGNode |
…Listener [SofaKernel] Refactor the MutationListener
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:
And this one is what this PR does:
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:
Reviewers will merge only if all these checks are true.