It seems to me that you have provided proof it doesn'tchrisb wrote:FreeCAD does not rely on any order?
Well, I moved some parts to a place known from another working version. So I wouldn't really call this a proof, but I will take it for granted until someone prooves the oppositeabdullah wrote:It seems to me that you have provided proof it doesn'tchrisb wrote:FreeCAD does not rely on any order?
The solution is not the one proposed in the PR.abdullah wrote:To be reconsidered in view of:
https://freecadweb.org/tracker/view.php?id=2836
Code: Select all
try {
// this sort gives the execute
boost::topological_sort(d->DepList, std::front_inserter(make_order));
}
catch (const std::exception& e) {
std::cerr << "Document::recompute: " << e.what() << std::endl;
return -1;
}
Code: Select all
App:Document/DocumentObject onRecomputeFailed implementation
============================================================
Why?
Recompute returns on first "problem". This may leave some objects in an unwanted state (for example the sketches).
Mechanism explanation:
Document Layer:
1. If a general recompute failure, for example a non-DAG dependency happens, then on RecomputeFailed is executed for the current document. This means that:
a) For each documentobject in the document, it is checked whether must execute flag is set, IF SET, then the documentobject layer onRecomputeFailed() is executed
2. If a feature recompute returns true (this means that the recompute process must be interrupted), then a) is executed.
DocumentObject Layer:
1. If the recompute of a feature fails, but it fails in a way that "false" is returned (this means the recompute process is not interrupted), then:
b) onRecomputeFailed is unconditionally executed for that feature.
DocumentObject level onRecomputeFailed is virtual and empty. It must be overriden if the documentobject must do anything special when recomputing fails.
Why do you consider 2836 (btw did you succeed in reproducing the crash?) a generalisation of 2710? To me these are two completely different problems with different causes. 2836 fails to recompute because the graph is not a DAG and thus doesn't even start a recompute but exits immediately. In 2710 the graph is a valid DAG but the recompute of the sketch stops because it handles the redundant constraints as error but leaves the sketch object in an inconsistent state.#2836 is a generalisation of the problem in #2710. A good solution for #2836 would solve #2710 and probably other difficult to debug crashes.
No, the issue described in 2710 is a sketch-specific issue and has absolutely nothing to do with the recompute mechanism. In 2836 we have a broken linkage of objects of the document. Here the question would be to try at least to recompute the part of the document with correct links. But that's another story.The problem lays in the Sketcher and the recompute mechanism.
That's exactly the point. It's up to the sketch object itself to make sure it's in a consistent state. If for any reason it cannot guarantee this in all cases then the methods accessing the geometry/constraints (in this case when updating the SoNodes) should take care of this fact and thus has to check that it can safely access the index values.The Sketcher works by keeping synchronised the solver geometry with the 3D view geometry. Lack of synchronisation ends up in crash due to SoNode indexing.
Exactly.I agree that SoNode implementation should be more robust and not crash for any reason. Arguably, all the problems could be solved by making that implementation more robust, as then there will be no crash and eventually the user will solve the problem (or die trying to).
No, that's wrong. The recompute only stops for severe exception like when running out of memory. But for less critical exceptions it continues to recompute the whole document.Recompute returns at the first chance that recompute is not successful.
Definitely not. When an object's execute() method is called it must make sure that even if it's in an erroneous state it's not causing a crash.I think we need a general mechanism to tidy up objects when recompute is interrupted.
Inside this method you don don't even know in which context an exception occurred and thus don't know what exactly happened and what to clean-up.Something like a signal onRecomputeFailed() children can subscribe to or something similar.
Having the new method onRecomputeFailed() weakens the current design of document recompute and opens a whole can of worms mainly because we "hide" the actual problem instead of fixing it.I think it is quite well explained in the commit message, so I just copy it here. Let me know if this PR is not acceptable:
Code: Select all
if (autoRecompute)
Gui::Command::updateActive();
else
static_cast<Sketcher::SketchObject *>(sketchgui->getObject())->solve();
Code: Select all
static_cast<Sketcher::SketchObject *>(sketchgui->getObject())->solve();
if (autoRecompute)
Gui::Command::updateActive();
Probably the choice of wording was not the best. They are two related cases to which a general solution can be applied so as to solve both. It is not that an error is a generalisation of the other error, but at least one general solution to the ticket 2710 also solves 2836. In any case, they bugs themselves are related at least because they share some commonalities (both arise during the recompute mechanism, both relate to current limitations to the recompute mechanism, both ultimately leave the sketch in an out of sync situation), which is what allows for the general solution.wmayer wrote: ↑Thu Jun 01, 2017 9:45 pmWhy do you consider 2836 (btw did you succeed in reproducing the crash?) a generalisation of 2710? To me these are two completely different problems with different causes. 2836 fails to recompute because the graph is not a DAG and thus doesn't even start a recompute but exits immediately. In 2710 the graph is a valid DAG but the recompute of the sketch stops because it handles the redundant constraints as error but leaves the sketch object in an inconsistent state.
Well, either we are working with different definitions of the same reality or I must disagree.wmayer wrote: ↑Thu Jun 01, 2017 9:45 pmThe problem lays in the Sketcher and the recompute mechanism.
No, the issue described in 2710 is a sketch-specific issue and has absolutely nothing to do with the recompute mechanism. In 2836 we have a broken linkage of objects of the document. Here the question would be to try at least to recompute the part of the document with correct links. But that's another story.
The recompute of a DocumentObject generally as a pattern as said above returns on the first error. I was never referring to the recompute of the Document itself. As a consequence of an error the DocumentObject interrupts the execution of itself and returns the control to the Document, who may or may not decide to continue as you say.
You make a good point in that the method does not know what happens and that in some cases, where the object is a perfect state a call to onRecomputeFailed is useless. However, the goal was to ensure an acceptable state, which included to reach one if not in a good state at the expense of not improving if in a good state.
I am not sure if I understood you correctly a couple of weeks ago, but you said you wanted to undertake this on your own. I have put on hold anything relating to the sketcher and representation until that happens, as it makes no sense to interfere or duplicate effort.
That is a very bad solution because there is no need to solve two times and all the rest of the overhead like updating the Geometry in a non-failing case two times. Let's try to clarify the issue with the pattern in the execute functions so that we can find a really good solution and not just an easy one. Below is a fragment of SketchObject::execute() showing only the recompute return paths:wmayer wrote: ↑Thu Jun 01, 2017 9:45 pmThe next step would be to avoid to let a sketch become inconsistent where possible. As far as I can see the sketch only becomes inconsistent when in the task panel the option for automatic recompute is active. So, this should be doable with changing the code from
Code: Select all
App::DocumentObjectExecReturn *SketchObject::execute(void)
{
try {
App::DocumentObjectExecReturn* rtn = Part2DObject::execute();//to positionBySupport
if(rtn!=App::DocumentObject::StdReturn)
//error
return rtn;
}
catch (const Base::Exception& e) {
return new App::DocumentObjectExecReturn(e.what());
}
if (lastDoF < 0) { // over-constrained sketch
std::string msg="Over-constrained sketch\n";
appendConflictMsg(lastConflicting, msg);
return new App::DocumentObjectExecReturn(msg.c_str(),this);
}
if (lastHasConflict) { // conflicting constraints
std::string msg="Sketch with conflicting constraints\n";
appendConflictMsg(lastConflicting, msg);
return new App::DocumentObjectExecReturn(msg.c_str(),this);
}
if (lastHasRedundancies) { // redundant constraints
std::string msg="Sketch with redundant constraints\n";
appendRedundantMsg(lastRedundant, msg);
return new App::DocumentObjectExecReturn(msg.c_str(),this);
}
if (lastSolverStatus != 0)
return new App::DocumentObjectExecReturn("Solving the sketch failed",this);
}
I think that such a solution would still be open to similar unwanted behaviour. An object may be part of a DAG problem (e.g. because of a cyclic support dependency) and still need a recompute (a correctly coded execute() that put a solver back in sync).
This should solve any outstanding issue.