Deep python bug. Changing an object via propertyLink may touch unnecessary stuff [fix proposed]

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
User avatar
DeepSOIC
Posts: 6869
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Deep python bug. Changing an object via propertyLink may touch unnecessary stuff [fix proposed]

Postby DeepSOIC » Fri Feb 10, 2017 8:25 pm

Hi!
1. Part Cube.
2. Part Offset the Cube.
3. Run this:

Code: Select all

App.ActiveDocument.Offset.Source.hasExtension("App::GroupExtension")
Result: Offset becomes touched. It shouldn't.
User avatar
DeepSOIC
Posts: 6869
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby DeepSOIC » Sun Feb 12, 2017 10:37 am

This bug was causing mysterious changes of Tip property of Module container in Part-o-magic. After a long investigation, I think I finally understood what was actually happening. But because nobody is familiar with part-o-magic, I will present a standalone way to reproduce what I think is happening. Here it goes.

1. Open the attached file.
link-change-example.FCStd
(4.9 KiB) Downloaded 19 times
It contains a Cube, a Cylinder, and a 3d offset applied to Cylinder.

2. Now, run this in Py console

Code: Select all

App.ActiveDocument.Offset.Source   # doesn't do anything, huh? But it makes Cylinder think its parent object is Offset!

3. Now this:

Code: Select all

App.ActiveDocument.Offset.Source = App.ActiveDocument.Box # move offset over to Box
App.ActiveDocument.recompute()
Now, Offset is applied to Cube instead of cylinder. So far so good.

4. and now this:

Code: Select all

App.ActiveDocument.getObject("Cylinder").Placement = App.Placement()
App.ActiveDocument.recompute()

This shouldn't do anything at all. But, Offset is now applied to Cylinder instead of Box! WTF!

Note that it's important to obtain Box Cylinder object not as an attribute. I used getObject() method here, but in Part-o-magic, the reference comes from getSelection().

BLUE = edit after replies
User avatar
DeepSOIC
Posts: 6869
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby DeepSOIC » Sun Feb 12, 2017 11:03 am

Oh, I forgot.
OS: Windows 10
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.17.9933 (Git)
Build type: Release
Branch: master
Hash: 6c3b78e97bbb8653e1189038c99682becef71626
Python version: 2.7.8
Qt version: 4.8.7
Coin version: 4.0.0a
OCC version: 7.0.0
wmayer
Site Admin
Posts: 14635
Joined: Thu Feb 19, 2009 10:32 am

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby wmayer » Sun Feb 12, 2017 2:30 pm

These are two different problems.
Result: Offset becomes touched. It shouldn't.
Fixed in git commit 34a3039
User avatar
DeepSOIC
Posts: 6869
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby DeepSOIC » Sun Feb 12, 2017 4:35 pm

wmayer wrote:Fixed in git commit 34a3039
Thanks! That was a minor thing, and I wouldn't ever have noticed it if not for the second, more serious one.

As far as I understand, it is a consequence of the code that makes this convenience to work: App.ActiveDocument.SomeObject.Placement.Base.x = some_x_value.
wmayer
Site Admin
Posts: 14635
Joined: Thu Feb 19, 2009 10:32 am

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby wmayer » Sun Feb 12, 2017 5:33 pm

I have no clear idea what exactly happens. But if in between these two snippets

Code: Select all

App.ActiveDocument.Offset.Source = App.ActiveDocument.Box # move offset over to Box
App.ActiveDocument.recompute()
and

Code: Select all

App.ActiveDocument.getObject("Cylinder").Placement = App.Placement()
App.ActiveDocument.recompute()
you select the Cylinder and change its placement in the property editor this heals the symptom. I don't think that the notification chain in general is broken but the Cylinder still must have a reference to the Offset feature.

It's best to file a bug report and set it to high priority.
User avatar
DeepSOIC
Posts: 6869
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby DeepSOIC » Sun Feb 12, 2017 6:21 pm

wmayer wrote:It's best to file a bug report and set it to high priority.
Done. issue #2902.
wmayer
Site Admin
Posts: 14635
Joined: Thu Feb 19, 2009 10:32 am

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby wmayer » Tue Feb 14, 2017 3:45 pm

It turned out to be a tricky issue.

When using code like this

Code: Select all

App.ActiveDocument.Offset.Source
then internally it tells the PyObjectBase object that it's the attribute xyz of the parent structure. E.g. the offset feature is the attribute "Offset" of the document, the cylinder feature is the attribute 'Source' of the offset feature.

The problem is that there is no way (that I know of) to clean up all this after its execution. So, this means that if you change the link of the Offset feature several times then each object has the information it's the 'Source' attribute of the offset.

To solve this problem there is a call of _getattr inside PyObjectBase::__setattr to get the old object which claims to be the attribute of the parent structure. If it's an object of type PyObjectBase then the information is cleared.
This is done in git commit b72aa9f3 and seems to solve the issue for all C++ classes that always return the same Python instance as its wrapper.

But there are also C++ classes that always return a new Python object and with this it's still not possible to inform the Python objects (PyObjectBase) about the change.
Example:

Code: Select all

doc=App.newDocument()
cyl = doc.addObject("Part::Cylinder","Cylinder")
doc.recompute()
plm = cyl.Placement
cyl.Placement = FreeCAD.Placement()
plm.Base.x = 5
cyl.Placement.Base.x
So, in this case a PyObjectBase must also track the attribute object it returns. So, it's a linkage in both directions: a PyObjectBase knows its name of the parent structure and the parent structure keeps a reference to the attribute objects. Since more information must be stored now and I didn't want to add a few more members I added a PyObject pointer which a dict will be assigned to.
git commit 4f23b56 first implements the replacement of the old members with a dict. And in git commit f4d8945dd8 the tracking/untracking is implemented.
User avatar
DeepSOIC
Posts: 6869
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby DeepSOIC » Tue Feb 14, 2017 11:29 pm

Thank you very much for fixing. This issue was also very tricky to find, because it was a complex interplay of python and c++ code, and I don't know how can one debug C++ and python at the same time.

I've given a quick try to FC after first mentioned commit, but haven't yet tested with the two new ones. I hope if Tip starts to mysteriously start changing again, me will be the only one to blame :P .
wmayer
Site Admin
Posts: 14635
Joined: Thu Feb 19, 2009 10:32 am

Re: Deep python bug. Changing an object via propertyLink may touch unnecessary stuff

Postby wmayer » Wed Feb 15, 2017 9:04 am

Thank you very much for fixing. This issue was also very tricky to find, because it was a complex interplay of python and c++ code, and I don't know how can one debug C++ and python at the same time.
Identifying the problem was the easiest part but what took me nearly a day was to find a solution. First I thought that git commit b72aa9f3 was enough but my gut feeling told me it's not, and it was right.