-
Notifications
You must be signed in to change notification settings - Fork 312
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
[All] VisualModel: implement NVI for init/updateVisual() #4827
[All] VisualModel: implement NVI for init/updateVisual() #4827
Conversation
[ci-depends-on] detected during build #2. To unlock the merge button, you must
|
[ci-build][with-all-tests] |
[ci-depends-on] detected during build #3. To unlock the merge button, you must
|
[ci-depends-on] detected during build #4. To unlock the merge button, you must
|
Sofa/GL/Component/Rendering3D/src/sofa/gl/component/rendering3d/OglModel.cpp
Outdated
Show resolved
Hide resolved
Sofa/GL/Component/Shader/src/sofa/gl/component/shader/Light.cpp
Outdated
Show resolved
Hide resolved
@@ -54,6 +54,32 @@ void VisualModel::drawVisual(const VisualParams* vparams) | |||
doDrawVisual(vparams); | |||
} | |||
|
|||
void VisualModel::updateVisual(const VisualParams* vparams) |
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 have a deep feeling of cleaness when I see this kind of refactoring (factoring things in the base class)
[ci-depends-on] detected during build #5. To unlock the merge button, you must
|
[ci-depends-on] detected during build #6. To unlock the merge button, you must
|
[ci-depends-on] detected during build #7. To unlock the merge button, you must
|
[ci-build][with-all-tests][force-full-build] |
[ci-depends-on] detected during build #8. To unlock the merge button, you must
|
d4e7b0a
to
fd4e090
Compare
[ci-depends-on] detected during build #9. To unlock the merge button, you must
|
[ci-depends-on] detected during build #10. To unlock the merge button, you must
|
b782d08
to
e8a69ca
Compare
[ci-depends-on] detected during build #11. To unlock the merge button, you must
|
…l break overriding)
e8a69ca
to
4b3a46b
Compare
[ci-depends-on] detected during build #12. To unlock the merge button, you must
|
[ci-depends-on] detected during build #13. To unlock the merge button, you must
|
[ci-build][with-all-tests] |
[ci-depends-on] detected during build #14. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
... and add VisualParameters as a parameter to their signature.
Based on
Continue on what has been introduced by:
Little difference, this PR does not only do the NVI thing but it adds the visualparameters as input.
This seems consistent (to me) with the fact that init/update visual as supposed to be called when there is a draw context (like drawVisual).
The nice side effect (and it was the intent at the beginning) is that updatevisual wont be called anymore if the component state is wrong, showVisualModels flag is not set or not enabled.
+ cleanings here and there in the visual/gl components
Unfortunately, breaking⚠️
[ci-depends-on https://github.com/sofa-framework/SofaPython3/pull/423]
[ci-depends-on https://github.com/sofa-framework/SofaSphFluid/pull/1]
TEMP:
[ci-depends-on https://github.com/sofa-framework/BeamAdapter/pull/153]
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