Yep, stack frame 26, Tree.cpp:1006, the assertion failure. I've removed that assertion. Please sync and try again.DeepSOIC wrote:I caught the crash in debugger.
#619 Made TreeView stable
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: #619 Made TreeView stable
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: #619 Made TreeView stable
Done. No crashes so far...realthunder wrote:Yep, stack frame 26, Tree.cpp:1006, the assertion failure. I've removed that assertion. Please sync and try again.
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: #619 Made TreeView stable
Good news. I lowered PoM's timer interval from 300 to 30 ms. This made it hit the assert every time. With your latest change, no asserts, and no crashing. Works fine!
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: #619 Made TreeView stable
This causes an endless loop:
I was quite surprised to find out it doesn't eat up more and more memory. Just hangs.
Code: Select all
class proxy(object):
def __init__(self, obj):
obj.Proxy = self
class vpproxy(object):
def __init__(self,vobj):
vobj.Proxy = self
def attach(self, vobj):
self.ViewObject = vobj
self.Object = vobj.Object
def claimChildren(self):
return [self.Object]
if not App.ActiveDocument:
App.newDocument()
obj = App.ActiveDocument.addObject("App::FeaturePython", "chicken_or_egg")
proxy(obj)
vpproxy(obj.ViewObject)
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: #619 Made TreeView stable
And another example with same result, that can actually happen for real.
Hangs, too.
Code: Select all
if not App.ActiveDocument:
App.newDocument()
import Part
chicken = App.ActiveDocument.addObject("Part::Extrusion", "chicken")
egg = App.ActiveDocument.addObject("Part::Extrusion", "egg")
chicken.Base = egg
egg.Base = chicken
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: #619 Made TreeView stable
Hmm... It's easy for me to fix this up by extra checking, and just not create item for this kind of scenario. But it can't prevent cyclic dependency, and it is still there. There bound to be other problems, e.g. if DAGView is enabled, it doesn't freeze the GUI, but spits error message endlessly. To be fair, FC without my patch will lock up too.DeepSOIC wrote:And another example with same result, that can actually happen for real.
It is actually a deeper problem than it appears. It is reasonable to demand that FC shall guarantee no cyclic dependency when creating models using GUI, but what about using python? None of the programming language that I know of prevent you from locking yourself up with infinite loop. Is it worth the trouble to detect such thing, and to what extent? Anyway, if checking is desirable, then it is better done in App namespace, before it reaches GUI, probably in link property assignment.
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: #619 Made TreeView stable
Indeed it does, I haven't checked this . But it shouldn't. I was using an artificial circular dependency in Lattice2 as a method to temporarily block recomputes...realthunder wrote:To be fair, FC without my patch will lock up too.
People arrive to cyclic dependencies in projects quite often, and it should be possible to fix the loop. Also, the loop may just be a temporarily state when some operation is in progress, like OpenSCAD ReplaceObject.
I'll have a look at what's going on, because I now have a feeling it's not your code that hangs.
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: #619 Made TreeView stable
It is kinda of. It actually hangs inside Qt::TreeWidgetItem.addChild. They don't bother checking cyclic reference either. And for a good reason, as it may be a potentially expensive operation.DeepSOIC wrote:I'll have a look at what's going on, because I now have a feeling it's not your code that hangs.
I added the check nevertheless. We'll see how it affects FC performance. Please sync the branch. As I mentioned, it may cause other problems. If you enable DAGView, you'll be spammed with endless error message.
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: #619 Made TreeView stable
I checked down to:
OS: Windows 8
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.16.6219 (Git)
Build type: Release
Branch: FreeCAD-DeepSOIC6
Hash: 1aff8d612e303501d401891e75aae8680b8c130f
Python version: 2.7.8
Qt version: 4.8.6
Coin version: 4.0.0a
OCC version: 6.7.1
And it hangs too. So we either were lucky and cyclic dependencies made so far never involved cyclic tree. Or they were, but people couldn't reproduce it, so didn't bother reporting.
OS: Windows 8
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.16.6219 (Git)
Build type: Release
Branch: FreeCAD-DeepSOIC6
Hash: 1aff8d612e303501d401891e75aae8680b8c130f
Python version: 2.7.8
Qt version: 4.8.6
Coin version: 4.0.0a
OCC version: 6.7.1
And it hangs too. So we either were lucky and cyclic dependencies made so far never involved cyclic tree. Or they were, but people couldn't reproduce it, so didn't bother reporting.
Well, at least it doesn't block one from fixing the looprealthunder wrote:If you enable DAGView, you'll be spammed with endless error message.
It may, as it looks like tree update is fired for every change to a property... So maybe just leave it alone for now?realthunder wrote:I added the check nevertheless. We'll see how it affects FC performance. Please sync the branch.
-
- Veteran
- Posts: 2190
- Joined: Tue Jan 03, 2017 10:55 am
Re: #619 Made TreeView stable
The cyclic check only happens when a free item (i.e. an item at document root) is newly claimed, which already has children. The check needs to visit all its children. That's the only case requiring a check. We'd be creating new items (without children obviously) for all other cases. So, I think the performance impact should be limited.DeepSOIC wrote:It may, as it looks like tree update is fired for every change to a property... So maybe just leave it alone for now?