[All] Fix visualisation

[bakpaul]

Visual models where broken since this PR #4827

Actually they were not always broken, when you started the simulation, everything was ok, but then if you unchecked visualModels in the View panel, and check it back, the visual models didn't follow the mechanical one anymore. The visual models where stuck in the position when you unchecked the VisualModel (see gif).

The problem was that the Visual params where not updated from the VisualStyle component at the right moment. It was only done in the VisualDrawVisitor, through the fwdDraw method. This enabled the drawing, but not the update of the visual models, because it was done before.

Why was it working when the visual flags remained untouched ? I don't know.

---

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).

[hugtalbot]

Shouldn't we rather properly update the visualparams ?
Before this change in #4827 the update was working fine.

The update process is done by RealGUI while the draw is done by the Viewer (Qt or QGL):

• Viewer calls the function Simulation::draw(sofa::core::visual::VisualParams* vparams, Node* root) ending in the object draw() function with up-to-date visual flags:
  • DefaultVisualManagerLoop::drawStep() ➡️ VisualDrawVisitor::processObject() ➡️ drawVisual() ➡️ draw()
• RealGUI calls sofa::simulation::node::updateVisual(root); (from Simulation::updateVisualContext()) ending in calling updateVisual() in OglModel where the visual flags are not up to date
  • DefaultVisualManagerLoop::updateStep() ➡️ VisualUpdateVisitor::processVisualModel() ➡️ updateVisual()

I therefore suspect the problem to come from the inconsistent API where draw relies on the VisualParams while the updateVisual needs to recover itself the VisualParams using :

sofa::core::visual::VisualParams* vparams = sofa::core::visual::visualparams::defaultInstance();

To sum up, could it be possible to make sure that using the defaultInstance() method returns up-to-date VisualParams?

[bakpaul]

The defaultInstance() actually returns a singleton. It coud stay updated. The thing that makes the VisualParams come back to outdated values if the call to bwdVisual in the end of the drawVisual call. This put back the old ones. I don't actually know why we need to go back to the old ones. It is such as we don't want to keep the values present in the visual style component in the global instance.

IMO, VisualStyle should have a DataCallback where every modification of its visual flag data is applied to the defaultInstance(). This removes all that I have introduced but impose that the default instance always follows the VisualStyle.

[fredroy]

The defaultInstance() actually returns a singleton. It coud stay updated. The thing that makes the VisualParams come back to outdated values if the call to bwdVisual in the end of the drawVisual call. This put back the old ones. I don't actually know why we need to go back to the old ones. It is such as we don't want to keep the values present in the visual style component in the global instance.

IMO, VisualStyle should have a DataCallback where every modification of its visual flag data is applied to the defaultInstance(). This removes all that I have introduced but impose that the default instance always follows the VisualStyle.

To be franck this is a bit confusing 🤔 it seems that there are some mixup between VisualParams & VisualStyle. It would have been great to directly quote the aforementioned code with a direct link 😅

In any case, I think this is a good idea to remove the VisualModel inheritance of VisualStyle, this does not make sense

[bakpaul]

No mixup here :
The fwd and bwd methods in VisualStyle :

sofa/Sofa/Component/Visual/src/sofa/component/visual/VisualStyle.cpp

Line 65 in 37ab743

void VisualStyle::fwdDraw(VisualParams* vparams)

Those are called by the drawVisualVisitor. This is when the DefaultInstance is modified. And this is the reason why in the VisualModels, during the the call to drawVisual vparams->displayFlags().getShowVisualModels() returned true, but not in the method updateVisual. here :

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

Line 33 in 37ab743

void VisualModel::drawVisual(const VisualParams* vparams)

The drawVisual visitor called fwdDraw first, so the VisualStyle updated the DefaultInstance, then he does the draw, then the call to bwdDraw put back the old values of the flags inside DefaultInstance. Because this was not called for the updateVisual, then it wasn't working. So removing the inheritance might make sense but doesn't solve the problem.

Why don't we leave the most updated values of the visual flags inside the default instance. Why do we reupload the backup values inside ? Couldn't it be constantly consistent with the actual visual flag values ?

[fredroy]

This is much clearer with direct quote with the code 🧑‍💻

Why don't we leave the most updated values of the visual flags inside the default instance. Why do we reupload the backup values inside ? Couldn't it be constantly consistent with the actual visual flag values ?

A wild guess would be to manage the fact there can be other VisualStyle deeper in your scene. A node can set some flags, and its subnodes can set(or unset) other flags.

[bakpaul]

PR updated regarding discussions. I'll add an issue regarding the testing of this feature in a unit test, I'll not have time to do it here.

[hugtalbot]

Just tested the PR, it fixes indeed the visualization problem

It remains that if we want to have specific VisualStyle : it has to be defined per Node and the last one override for the rest of the graph. VisualStyle should follow the mechanism we plan to deploy on gravity with data/links and tests should be added as suggested in #5157

[alxbilger]

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

[alxbilger]

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