[all] Fix the cause of failling test scenes (for a greener dashboard) #548
Conversation
Those file have been merge by mistake some time ago but they have no meaning in the current Sofa version. So I remove them.
Otherwise some test scenes using Fluid2D & Fluid3D are failing.
The <include> directive was using local path instead of relative to the 'example' directory as other examples. NB: I wonder why the shared "Object" directory is in examples. If it is shared. it should be in the 'share' directory ?
…d on a plugin.HighOrder NB: It would be better to generate the .scene-tests from the cmake depending on the other plugin found.
… as this make the scene fail. This scene is failing since its addition in PR: sofa-framework#270
…is printed. This is implemened only at init & reinit.
… since PR sofa-framework#521 The more I see PR the more I wonder why the algorithm allow 'den' to be zero. Shouldn't this issue a warning saying that the scene is probably broken ?
The plugin is broken since PR sofa-framework#549 as it removes the initialization of the mutex. Possible explaination is that v_ is implementation dependent but the existing initialization was done incorrectly using a (int). Now use BOOST_DETAIL_SPINLOCK_INIT which insure that the initialization is coherent with the implementation used. (I also cannot prevent myself to remove the two ugly commented lines)
…e merge of #PR 243
In #PR 243 the following change was introduced in MechanicalObject.inl::init():
```cpp
if (!l_topology && d_useTopology.getValue())
{
l_topology.set( this->getContext()->getActiveMeshTopology() );
}
```
In place of
```cpp
//Look at a topology associated to this instance of MechanicalObject by a tag
this->getContext()->get(m_topology, this->getTags());
// If no topology found, no association, then look to the nearest one
if(m_topology == NULL)
m_topology = this->getContext()->getMeshTopology();
```
I assume that the in the scene was the EdgeSetTopologyContainer is not returned by
the getActiveMeshTopology method.
NB: All those implicit linking looks really problematic to me as they are error prone.
…lace some sout with msg_info.
hugtalbot
added
pr: status ready
…4 (WIP) MacOs is hard to fix. But fundamentally...using anything from boost::details is fundamentally bad.
| @@ -165,7 +161,7 @@ namespace sofa | |||
|
|
|||
|
|
|||
|
|
|||
| mutable boost::detail::spinlock mTaskMutex; | |||
| mutable boost::detail::spinlock mTaskMutex {BOOST_DETAIL_SPINLOCK_INIT}; | |||
There was a problem hiding this comment.
GG for the fix!
I think you should choose between keeping a TAB indentation or fixing it with spaces for the whole file ;-)
There was a problem hiding this comment.
Well...I'm not in favor of changing the whole file indent for a one line modification. In that case isn't it better to apply the astyle just before merging the PR ?
Shouldn't I wait for the automatic application of the astyle formatting sheet by the CI ?
| if( verbose ) | ||
| { | ||
| sout<<"CGLinearSolver, den = "<<den<<", smallDenominatorThreshold = "<<f_smallDenominatorThreshold.getValue()<<sendl; | ||
| } |
There was a problem hiding this comment.
hi @damienmarchal
I thought it over, my fix was wrong but yours is still not optimal.
I would like to fix it in another PR, would you agree ? I will revert this in your PR as well if you agree.
There was a problem hiding this comment.
Of course, if you think it is relevant you can fix it in another PR, just do it, but please keep in mind that if everyone open PR to fix scenes it will be a big mess very quickly and we will have to "ping pong" between all the PRs.
EDIT: pushing regularly & working in this single PR have the advantage that we can see each other progress and activity.
EDIT: I didn't responded clearly but yes you can fix everything you want I didn't planned to work on this one and the commited version was just there to show you the problems.
There was a problem hiding this comment.
agreed for the working one single for this purpose, just the CG case is wider than just scenes. It's a real bug.
New PR for the CG: #556
| if( fabs(den) == 0.0 ) | ||
| { | ||
| endcond = "div by zero. You scene is probably buggy."; | ||
| if( verbose ) |
There was a problem hiding this comment.
by the way @damienmarchal shouldn't we choose between verbose and printLog .. isn't it redundant ?
There was a problem hiding this comment.
Well...
yes but no but yes but no :)
the only totally redundent thing is:
if(printLog.getValue())
msg_info() << "Blahblah"printLog is the way to activate or deactive all the messages going through msg_info() but as it is implemented there is no fine grain control. This is why adding an additional option like "verbose" (boolean or int) allows to implement multiple level of 'info' verbosity for your component.
In sofa there is a lot of component that hard code multi-levels verbosity but in my component I prefer to stick to printLog as it make things easier and more consistant.
There was a problem hiding this comment.
ok, you would not remove the 'verbose' use (despite the ugly multi-levels verbosity) ?
There was a problem hiding this comment.
This PR was initially on fixing the failing test...thus I'm not sure it is good if we add too much cleaning or refactoring or cosmetic changes as it will make the review harder. I just cannot prevent myself to force the printLog when verbose is set to simplify a bit the things (in init & reinit).
|
I did not have a lot of time to progess here but still a reminder for later:
|
damienmarchal
force-pushed
the
fixTestScene
branch
from
dead30a to
abbfb74
Compare
|
[ci-build][with-scene-tests] |
|
[ci-build][with-scene-tests] |
|
caduceus.scn crashes on both macos_default and macos_options on master when running in batch mode. |
|
@guparan Thank for the report. |
|
Below is the stack. I do not understand why the visual objects are created in the batch mode.
|
|
@untereiner I think that this is because batch mode does not "remove" element from the scene Thanks for the stack trace... I will try to make something about it (even on macos). |
|
Launching an opengl context "by hand" is complicated if I remember correctly. I suggest that the batch mode uses GLEW to handle the opengl context. In the other options it is Qt that handles the context creation. |
The old version of the plugin is loading as plugins the content of pluginName or, if empty, the "name" of the component. To improve functionnal consistency this, when the name is use as pluginName, adds it to the pluginName lists. So that pluginName contains all the "informations".
|
It is definitively hard to have an opengl context by hand...and having one for offscreen (without a windows) seems even more tricky (anyone's help is welcome). I know @ErwanDouaille search for that in his HeadLessRecorder I assume it work on linux/windows...because they just don't crash/segfault when gl function are called without a buffer :) Too bad... I tried a lot of approach based on QOffscreenSurface and friends, they work on my system but as soon as I move that to a CI machine (which does not have X or GLX or whatever)...the application crash. I reseted the branch but you can still see the garbage in the Dashboard. I will try a different approach tomorrow. |
damienmarchal
force-pushed
the
fixTestScene
branch
from
3688dd2 to
6da1db4
Compare
|
[ci-build][with-scene-tests] |
damienmarchal
force-pushed
the
fixTestScene
branch
from
c3dfac7 to
448e493
Compare
|
Ho damn...two full build + scene that are green builds... and not one to cheers (deep sadness) |
|
[ci-build][with-scene-tests] |
|
Wow, such green, very stable, many thanks! ;-) |
|
More seriously...I don't know how to fix the MacOS version (and I think the batch mode is fundamentally broken :)). Tomorrow I will clean the PR, possibly cutting it in pieces because I doubt guys here will accept to merge it "as it is" :) |
|
@epernod Could you quick review the commit 47ef2e6 please? How could CubeTopology.scn have been passing scene-tests without this fix? |
# Conflicts: # applications/plugins/CMakeLists.txt
|
This PR seems quite empty now. Shall we close it @damienmarchal ? |
|
Yes, |
|
Thank you for your hard work! :-) |
hugtalbot
removed
the
pr: status to review
This PR:
Reviewers will merge only if all these checks are true.