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.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

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

Post by DeepSOIC »

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

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

Post by DeepSOIC »

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

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

Post by DeepSOIC »

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
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

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

Post by wmayer »

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

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

Post by DeepSOIC »

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
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

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

Post by wmayer »

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

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

Post by DeepSOIC »

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

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

Post by wmayer »

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

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

Post by DeepSOIC »

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
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

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

Post by wmayer »

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