[Fixed] Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
wmayer
Founder
Posts: 20202
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by wmayer »

they share some commonalities (both arise during the recompute mechanism, both relate to current limitations to the recompute mechanism,
No, actually not. In both cases the object is in an inconsistent state before the recompute starts. It then expects that a recompute may solve this but in one case it's the object itself that returns the error flag and in the other case a recompute cannot be performed. Again, the solution can only be:
1. the object guarantees to be consistent all the time, then its methods or its view provider can rely on this and don't have to double-check all accesses
2. the object doesn't guarantee this, then its methods or view provider have to double check each access.

So, the limitation is not the recompute mechanism but the fact that the calling instance (which triggers the recompute) doesn't care about the state of the object beforehand and afterwards.
To the first: it is caused by operating by the choice of returning control directly after the first error when recomputing a DocumentObject. I do not know if this was a decision made by design (I was not here at the time), but it is indeed a pattern that can be seen in most execute() methods of DocumentObject derivatives.
It's always up to each DocumentObject subclass how it wants to handle unexpected situations. It's just that in many it cases it doesn't make sense to continue because following operations are based on a previously successful operation. But it's also possible that something failed it tries an alternative way. So, there is and was never a design decision that it must return after the first problem.

All what the Document is interested in is a DocumentObject's interface, i.e. that it returns StdReturn or the error structure or raises a well-defined exception. It doesn't make any assumption about implementation details.
In any case, I agree that this ticket can be solved by a sketcher-only solution, only it is a non-sense to do it so, if another mechanism is needed for the other ticket that could solve both. It was done so in the original PR:
The PR aimed to solve two different problems with what you call a general solution. But in fact the PR only solves one specific problem where a call of the solve() method was missing. And from a DocumentObject's point of view it doesn't even know why onRecomputeFailed() is getting called.
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.
Sorry, I thought you meant to interrupt the recompute of Document, not a DocumentObject. But again as said above: a DocumentObject can do what ever it wants to do, i.e. it can give up after the first error or it can try to continue. The Document class doesn't make any assumptions about this.
In any case, I can accept your decision. So let's concentrate in what is acceptable:
Look at the PR799. It made basically two changes:
1. ViewProviderSketch::drawConstraintIcons() double-checks that it can safely access the SoNode of the constraint group. If not it prints a warning and stops the loop
2. solve() is now called independently if auto-recompute is on of off which makes it consistent with many other cases where it was done like this. And the view provider is notified about important changes of the sketch without relying on a successful or failing recompute of the document. And as far as I understood the auto-recompute option its purpose in the first place is to recompute all other objects that somehow depend on the sketch.
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.
Currently I am getting flooded with PRs and thus have much less time to do my actual work. What I want to do at some point is re-factoring the code of ViewProviderSketch which has grown to a big and unhandable monster over time. There are methods with more than thousand lines of code and similar code all over the places.
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.
My solution causes way less overhead than yours. How can a direct call of solve() cause more overhead than calling it by several cascades inside a virtual method?
Question: Is it acceptable that I do not return just after the first error has been detected? For example, it is acceptable to define booleans for the different errors and move all the return statements to the end of the function, so that I return the right error message, but I can still leave my sketch in a sync state?
You can do whatever you want as long as you don't make any assumptions about implementation details of other classes.
wmayer wrote:Or we simply call the recompute for each object in a linear order.
But that should be the very last option. First we have to try to figure out which objects cause the cyclic dependency, update the others as we do now and then update the problematic objects in a linear or any arbitrary order.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by abdullah »

wmayer wrote: Fri Jun 02, 2017 1:09 pm In any case, I agree that this ticket can be solved by a sketcher-only solution, only it is a non-sense to do it so, if another mechanism is needed for the other ticket that could solve both. It was done so in the original PR:

The PR aimed to solve two different problems with what you call a general solution. But in fact the PR only solves one specific problem where a call of the solve() method was missing. And from a DocumentObject's point of view it doesn't even know why onRecomputeFailed() is getting called.
Wrong. That is not the PR I linked. Please follow the link:
https://github.com/FreeCAD/FreeCAD/pull/771/files

My original PR is not the PR about onRecomputeFailed. It is just a lambda function that gets called in execute method to just update the geometry on failure.
wmayer wrote: Fri Jun 02, 2017 1:09 pm Currently I am getting flooded with PRs and thus have much less time to do my actual work. What I want to do at some point is re-factoring the code of ViewProviderSketch which has grown to a big and unhandable monster over time. There are methods with more than thousand lines of code and similar code all over the places.
Sorry for those originating from me.
wmayer wrote: Fri Jun 02, 2017 1:09 pm Look at the PR799. It made basically two changes:
1. ViewProviderSketch::drawConstraintIcons() double-checks that it can safely access the SoNode of the constraint group. If not it prints a warning and stops the loop
2. solve() is now called independently if auto-recompute is on of off which makes it consistent with many other cases where it was done like this. And the view provider is notified about important changes of the sketch without relying on a successful or failing recompute of the document. And as far as I understood the auto-recompute option its purpose in the first place is to recompute all other objects that somehow depend on the sketch.
Good for the refactoring of tryAutoRecompute().
wmayer wrote: Fri Jun 02, 2017 1:09 pm My solution causes way less overhead than yours. How can a direct call of solve() cause more overhead than calling it by several cascades inside a virtual method?
Your current solution causes way more overhead than mine in the PR I linked. Currently the sketcher takes great care not to duplicate effort in solve()+execute(). When "auto update" is checked, you are basically duplicating effort on all the situations (including those with a correct outcome) except in those where the bug is present (where one of them is not actually executed, the execute() one). With that you indeed fix the bug.

Your solution has one advantage over the linked PR and is that it maintains always the solver in sync if the sketch was modified (when in edit mode) also in a non-DAG error case (in a non-DAG case, my PR won't work as the execute() does not get executed, as it was intended to fix the former bug, not the latter).

If you are going to solve before recompute (which also solves), then I think there should be some mechanism to avoid the duplicated solve/update of Geometry in execute() during the recompute. In such a case an execute should just reuse existing solve() effort.

Because I never use the Auto-Update checked, it won't affect my performance, so if you want to push this solution. Fine with me.
wmayer
Founder
Posts: 20202
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by wmayer »

Wrong. That is not the PR I linked. Please follow the link:
https://github.com/FreeCAD/FreeCAD/pull/771/files

My original PR is not the PR about onRecomputeFailed. It is just a lambda function that gets called in execute method to just update the geometry on failure.
Oh, I never looked at this PR since you closed it yourself and thus I thought it contained some mistakes. Currently I am busy with integrating the remaining py3 changes and can't easily switch to other branches for testing. So, does pr771 fix the issue 2710, too?
Your current solution causes way more overhead than mine in the PR I linked. Currently the sketcher takes great care not to duplicate effort in solve()+execute(). When "auto update" is checked, you are basically duplicating effort on all the situations (including those with a correct outcome) except in those where the bug is present (where one of them is not actually executed, the execute() one). With that you indeed fix the bug.
OK, I see.
If you are going to solve before recompute (which also solves), then I think there should be some mechanism to avoid the duplicated solve/update of Geometry in execute() during the recompute. In such a case an execute should just reuse existing solve() effort.
I thought about this too where we could reserve a certain bit of the StatusBits but I think it's getting too complicated to set/reset the bit when needed. So, I think we shouldn't go this way.
Because I never use the Auto-Update checked, it won't affect my performance, so if you want to push this solution. Fine with me.
Since you put quite some effort into reducing superfluous updates & recomputes in the past I don't intent to (partially) revert this again. So, I think the path to go could be:
1. Merge pr771
2. change the return value of tryAutoRecompute to bool and if false (which means a recompute wasn't performed) invoke solve(). (So, we have exact the same behaviour is now but with the advantage of the code-refactoring)
3. add the security check into drawConstraintIcons() as without it still crashes for non-DAGs
4. Think about a solution how to recompute a non-DAG document

This way I think we have put together the best parts of all ideas. Any comments?
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by abdullah »

wmayer wrote: Sat Jun 03, 2017 1:09 am Oh, I never looked at this PR since you closed it yourself and thus I thought it contained some mistakes. Currently I am busy with integrating the remaining py3 changes and can't easily switch to other branches for testing. So, does pr771 fix the issue 2710, too?
pr771 fixes only #2710. It does not solve #2836 (non-DAG).

However, I think that the underlaying idea of that commit can be improved in view of this discussion.
wmayer wrote: Sat Jun 03, 2017 1:09 am I thought about this too where we could reserve a certain bit of the StatusBits but I think it's getting too complicated to set/reset the bit when needed. So, I think we shouldn't go this way.
I thought about reusing a bit of DocumentObject StatusBits, but the idea never achieved a satisfying form.
wmayer wrote: Sat Jun 03, 2017 1:09 am This way I think we have put together the best parts of all ideas. Any comments?
First it is a big thank you for acknowledging the possibility of improving the solution. I agree to 2-4. I think it would be wise to improve 771 with the feedback from this thread.

I propose the following way forward:

I will think the whole matter through again to incorporate all ideas and try to improve them. I will come back to you in this thread with further comments or with a new PR based on 1-3 or 1-4 in a couple of days.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by abdullah »

wmayer wrote: Sat Jun 03, 2017 1:09 am This way I think we have put together the best parts of all ideas. Any comments?
Time for you to take a quick look to see if something is not acceptable, so that I do not diverge more.

Code:
https://github.com/abdullahtahiriyo/Fre ... s/new_2710

What?
- 1-3 as agreed with some refactoring of the code. As now execute() uses a call to solve() the behaviour between auto-update checked and unchecked should be harmonized.
- Partial implementation of 4.

About 4: Probably I have hit all the possible walls of boost adjacency list, invalid iterators and descriptors on removal of VecS lists, lack of vertex map index when using ListS... at the end I have managed to identify the nodes creating the dependency and recompute the acyclic part (not yet a linear recompute of the cyclic part).

This solves:
#2710 => 1-3 solves it
#2836 => Our non-DAG as discussed before (1-3 solves the crash, 4 solves the (lack of) recompute)
#2982 => Weird behaviour on non-DAG (https://freecadweb.org/tracker/view.php?id=2982) (4 solves it)

Let me know your impressions.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by abdullah »

PR:
https://github.com/FreeCAD/FreeCAD/pull/806

I would like to improve non-dag notification to the user, but I will undertake that after this is merged.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by abdullah »

Note: The unit test are failing, because the cyclic dependency test that was expected to fail, now it is not failing.

Here a decision is need on what to return. Currently the number of recomputed acyclic objects is returned. Alternatives can be "-1" (something wrong happened), or the total number acyclic+cyclic. I do not know which is one is more useful.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by abdullah »

wmayer wrote: Sat Jun 03, 2017 1:09 am This way I think we have put together the best parts of all ideas. Any comments?
abdullah wrote: Tue Jun 06, 2017 5:39 pm Note: The unit test are failing, because the cyclic dependency test that was expected to fail, now it is not failing.

Here a decision is need on what to return. Currently the number of recomputed acyclic objects is returned. Alternatives can be "-1" (something wrong happened), or the total number acyclic+cyclic. I do not know which is one is more useful.
I reviewed all the uses of recompute. The return code is only used for unit tests. I have decided that it is best to return -1 in the sense that we have a problem, a non-DAG, maybe we managed to recompute it so as not to leave objects in a bad state, but we still have a problem that should be notified to the caller (in case it is interested in doing something with it). So returning -1 seems the best option.

Returning -1 makes also unnecessary changes to the unit tests.

BTW, it also fixes #3062, so the list:

#2710 => 1-3 solves it
#2836 => Our non-DAG as discussed before (1-3 solves the crash, 4 solves the (lack of) recompute)
#2982 => Weird behaviour on non-DAG (https://freecadweb.org/tracker/view.php?id=2982) (4 solves it)
#3062 => Crash when retracing external geometry (probably 1-3 solves it, with this branch is not reproducible).
User avatar
Kunda1
Veteran
Posts: 13434
Joined: Thu Jan 05, 2017 9:03 pm

Re: Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by Kunda1 »

So just to sum things up there are 2 PRs pending as fixes for issue #2710
@wmayer's https://github.com/FreeCAD/FreeCAD/pull/799
and
@abdullah's https://github.com/FreeCAD/FreeCAD/pull/806
Thanks fellas for the effort so far... so how do we proceed?
Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
'good first issues' | Open TODOs and FIXMEs | How to Help FreeCAD | How to report Bugs
User avatar
Kunda1
Veteran
Posts: 13434
Joined: Thu Jan 05, 2017 9:03 pm

Re: [Fixed] Crash Bug #2710 Freecad doesn't show sketch elements added when overconstrained

Post by Kunda1 »

Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
'good first issues' | Open TODOs and FIXMEs | How to Help FreeCAD | How to report Bugs
Post Reply