Skip to content

[all] Use NVI design pattern for drawVisual#3931

Merged
alxbilger merged 22 commits into
sofa-framework:masterfrom
CRIStAL-PADR:pr-use-pattern-nvi-for-drawvisual
Jul 6, 2023
Merged

[all] Use NVI design pattern for drawVisual#3931
alxbilger merged 22 commits into
sofa-framework:masterfrom
CRIStAL-PADR:pr-use-pattern-nvi-for-drawvisual

Conversation

@damienmarchal
Copy link
Copy Markdown
Contributor

@damienmarchal damienmarchal commented Jun 8, 2023

Actually what we called "delegate" pattern (the one we used to think about to fix the "call-super" anti-pattern) is often refereed on intenret as Non Virtual Interface (a pattern very similar to the Template Method Pattern).

Here is a attempt to see it in use on VisualModel::drawVisual;


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

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

@damienmarchal damienmarchal added the pr: status to review To notify reviewers to review this pull-request label Jun 8, 2023
…ode;

Signed-off-by: Damien Marchal <damien.marchal@univ-lille1.fr>
@hugtalbot hugtalbot added the pr: enhancement About a possible enhancement label Jun 8, 2023
Comment thread Sofa/framework/Core/src/sofa/core/visual/VisualModel.cpp Outdated
Comment thread Sofa/framework/Simulation/Core/src/sofa/simulation/VisualVisitor.cpp Outdated
Comment thread Sofa/framework/Simulation/Core/src/sofa/simulation/VisualVisitor.cpp Outdated
damienmarchal and others added 2 commits June 14, 2023 00:01
Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>
…or.cpp

Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>
@damienmarchal
Copy link
Copy Markdown
Contributor Author

@alxbilger thank for the review.

@damienmarchal
Copy link
Copy Markdown
Contributor Author

Do everyone agrees on having the conditions I moved up into the NVI VisualModel::drawVisual() and with the one I added into the VisualVisitor ?

Eg:

void VisualModel::drawVisual(const VisualParams* vparams)
{
    // don't draw if specified not to do so in the user interface
    if (!vparams->displayFlags().getShowVisualModels())
        return;

    // don't draw if the component is not in valid state (we can change the condition so Loading/Unknow states are drawn differently) 
    if( d_componentState.getValue() == sofa::core::objectmodel::ComponentState::Invalid )
        return;

    // don't draw if this component is specifically configured to be disabled (It is questionnable to have a data for that). 
    // having a d_enable on Base would be better. 
    if (!d_draw.getValue())
        return;

    // RAI to be sure the states changed during the rendering don't leaks 
    const auto stateLifeCycle = vparams->drawTool()->makeStateLifeCycle();

    doDrawVisual(vparams);
}

Comment thread Sofa/framework/Core/src/sofa/core/visual/VisualModel.h Outdated
Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>
Comment thread Sofa/framework/Core/src/sofa/core/visual/VisualModel.h Outdated
@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 Jun 21, 2023
Comment thread Sofa/framework/Core/src/sofa/core/visual/VisualModel.cpp
@hugtalbot hugtalbot changed the title Use NVI design pattern for drawVisual [all] Use NVI design pattern for drawVisual Jun 26, 2023
@damienmarchal damienmarchal 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 Jun 28, 2023
@alxbilger
Copy link
Copy Markdown
Contributor

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

Comment thread Sofa/framework/Core/src/sofa/core/visual/VisualModel.cpp Outdated
@alxbilger alxbilger added the pr: status ready Approved a pull-request, ready to be squashed label Jun 28, 2023
@damienmarchal
Copy link
Copy Markdown
Contributor Author

CI passed...

Copy link
Copy Markdown
Contributor

@alxbilger alxbilger left a comment

Choose a reason for hiding this comment

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

Now that you have renamed d_draw into d_enable, you need to deprecate d_draw in the derived classes instead of removing them.

@alxbilger alxbilger removed the pr: status ready Approved a pull-request, ready to be squashed label Jun 29, 2023
Comment thread Sofa/framework/Config/src/sofa/config.h.in Outdated
Comment on lines +44 to +45
if( d_componentState.getValue() == sofa::core::objectmodel::ComponentState::Invalid )
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should be :

Suggested change
if( d_componentState.getValue() == sofa::core::objectmodel::ComponentState::Invalid )
return;
if( !this->isComponentStateValid() )
return;

However, this will invalidate all components which do not support their componentState

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So ? It looks like you are making a suggestion of change but saying it will not work the same.

@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 Jul 5, 2023
Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>
@alxbilger alxbilger 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 Jul 6, 2023
@alxbilger
Copy link
Copy Markdown
Contributor

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

@alxbilger alxbilger 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 Jul 6, 2023
@alxbilger alxbilger merged commit 9cfa31a into sofa-framework:master Jul 6, 2023
@hugtalbot hugtalbot added this to the v23.12 milestone Jul 21, 2023
@damienmarchal damienmarchal deleted the pr-use-pattern-nvi-for-drawvisual branch July 24, 2023 13:23
bakpaul pushed a commit to bakpaul/sofa that referenced this pull request Feb 23, 2024
* Implement the drawVisual method using the NVI pattern to factorize access control.

* Propagate the change.

* FIXUP

* [Sofa.Core] Refactor VisualVisitor::processVisualModel to factorize code;

Signed-off-by: Damien Marchal <damien.marchal@univ-lille1.fr>

* FIXUP

* Adds a transitional "virtual" keyword on drawVisual to not break third party plugin.

* Update Sofa/framework/Core/src/sofa/core/visual/VisualModel.cpp

Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>

* Update Sofa/framework/Simulation/Core/src/sofa/simulation/VisualVisitor.cpp

Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>

* FIX alignement

* Update Sofa/framework/Core/src/sofa/core/visual/VisualModel.h

Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>

* FIXUP

* Add proper deprecation of "visible" flag in OglLabel

* FIXUP

* Implement top level control of drawWireFrame

* [all] Rename d_draw by d_enable VisualModel to make it consistent with other components

* Use the newly introduced RemovedData to manage the use of "enable" instea of "draw" and "visible"

* Test for d_enable before d_componentState == Valid to avoid updating the component if disable.

* Fix OglLable_test to remove the "visible" attribute;

* Update Sofa/framework/Config/src/sofa/config.h.in

Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>

---------

Signed-off-by: Damien Marchal <damien.marchal@univ-lille1.fr>
Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement About a possible enhancement pr: status ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants