#619 Made TreeView stable

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Post by realthunder »

DeepSOIC wrote:I caught the crash in debugger.
Yep, stack frame 26, Tree.cpp:1006, the assertion failure. I've removed that assertion. Please sync and try again.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Post by DeepSOIC »

realthunder wrote:Yep, stack frame 26, Tree.cpp:1006, the assertion failure. I've removed that assertion. Please sync and try again.
Done. No crashes so far...
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Post by DeepSOIC »

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!
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Post by DeepSOIC »

This causes an endless loop:

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)
I was quite surprised to find out it doesn't eat up more and more memory. Just hangs.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Post by DeepSOIC »

And another example with same result, that can actually happen for real.

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
Hangs, too.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Post by realthunder »

DeepSOIC wrote:And another example with same result, that can actually happen for real.
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.

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.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Post by DeepSOIC »

realthunder wrote:To be fair, FC without my patch will lock up too.
Indeed it does, I haven't checked this :oops: . But it shouldn't. I was using an artificial circular dependency in Lattice2 as a method to temporarily block recomputes...

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.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Post by realthunder »

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

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.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Post by DeepSOIC »

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.
realthunder wrote:If you enable DAGView, you'll be spammed with endless error message.
Well, at least it doesn't block one from fixing the loop ;)
realthunder wrote:I added the check nevertheless. We'll see how it affects FC performance. Please sync the branch.
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
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Post by realthunder »

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?
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.
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
Post Reply