Ticket #6266: recompute undos removeSplitter

Have some feature requests, feedback, cool stuff to share, or want to know where FreeCAD is going? This is the place.
Forum rules
Be nice to others! Read the FreeCAD code of conduct!
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Ticket #4754: recompute undos removeSplitter

Post by openBrain »

adrianinsaval wrote: Tue Jan 25, 2022 7:06 pm but why is removeSplitter() allowed without using copy()?
Don't know. This is the real issue here IMO.
TheMarkster
Veteran
Posts: 5505
Joined: Thu Apr 05, 2018 1:53 am

Re: Ticket #4754: recompute undos removeSplitter

Post by TheMarkster »

Well, obj.Shape.removeSplitter() does not modify obj.Shape, but instead returns a new refined shape object, so there's no need to disallow calling it on obj.Shape. Unless I'm not thinking of some good reason to disallow it.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Ticket #4754: recompute undos removeSplitter

Post by openBrain »

TheMarkster wrote: Thu Jan 27, 2022 3:36 am Well, obj.Shape.removeSplitter() does not modify obj.Shape, but instead returns a new refined shape object, so there's no need to disallow calling it on obj.Shape. Unless I'm not thinking of some good reason to disallow it.
You're totally right.
I pushed the test and the problem is far more generic. To reproduce :
* New blank document, add default Part/Cube and default Part/Cylinder
* Move Cylinder with it's Placement value (say [10,0,0])
* Enter App.ActiveDocument.Box.Shape = App.ActiveDocument.Cylinder.Shape
* Recompute => The Cube shape is kept, but it is moved to Cylinder placement

I have absolutely no idea where the magics take place in the code when assigning a new value to '.Shape'. :?
wmayer wrote: Ping
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Ticket #4754: recompute undos removeSplitter

Post by adrianinsaval »

TheMarkster wrote: Thu Jan 27, 2022 3:36 am Well, obj.Shape.removeSplitter() does not modify obj.Shape, but instead returns a new refined shape object, so there's no need to disallow calling it on obj.Shape. Unless I'm not thinking of some good reason to disallow it.
Yes as I said the issue has nothing to do with removeSplittter() (as such, can we change the title as it is misleading?) however (going off topic here) I don't understand what's the rationale behind allowing/disallowing functions, AFAIK obj.Shape.scaled(3) doesn't modify obj.Shape either, it returns a scaled shape, so why disallow? I understand obj.Shape.scale(3) should be disallowed since this does modify it.
openBrain wrote: Thu Jan 27, 2022 10:55 am I pushed the test and the problem is far more generic. To reproduce :
So the issue is that when trying to assign a new shape to the object, that placement is taken but not the actual shape. I wonder if it should really take the placement at all, even when changing the shape is allowed, aren't they supposed to be independent?
TheMarkster
Veteran
Posts: 5505
Joined: Thu Apr 05, 2018 1:53 am

Re: Ticket #4754: recompute undos removeSplitter

Post by TheMarkster »

adrianinsaval wrote: Thu Jan 27, 2022 5:16 pm
Yes as I said the issue has nothing to do with removeSplittter() (as such, can we change the title as it is misleading?) however (going off topic here) I don't understand what's the rationale behind allowing/disallowing functions, AFAIK obj.Shape.scaled(3) doesn't modify obj.Shape either, it returns a scaled shape, so why disallow? I understand obj.Shape.scale(3) should be disallowed since this does modify it.
I was also thinking the same thing. Any function that does not modify the shape, but returns a new one should be allowed.

Shape and Placement are different things, but the Shape also has a placement. Try this: create a new cylinder in Part workbench. Move it to 10,0,0. Select and press Ctrl+Shift+P. In the console enter obj.Placement and obj.Shape.Placement. You'll get the same placement for both the object and the shape.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Ticket #4754: recompute undos removeSplitter

Post by openBrain »

adrianinsaval wrote: Thu Jan 27, 2022 5:16 pmI don't understand what's the rationale behind allowing/disallowing functions, AFAIK obj.Shape.scaled(3) doesn't modify obj.Shape either, it returns a scaled shape, so why disallow? I understand obj.Shape.scale(3) should be disallowed since this does modify it.
You're right. Maybe open a new topic to fix this ?
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Ticket #4754: recompute undos removeSplitter

Post by adrianinsaval »

Ok then here's the thing, you CAN change the shape, it's just that it's regenerated the moment you recompute. Try this:

Code: Select all

box=App.ActiveDocument.addObject("Part::Box","Box")
cyl=App.ActiveDocument.addObject("Part::Cylinder","Cylinder")
cyl.Visibility= False
box.Visibility= False
App.ActiveDocument.recompute()
cyl.Shape=box.Shape
Part.show(cyl.Shape)
A new object with the cube shape is created, despite providing the cylinder's shape. So when you assign the shape you also assign the placement, however recomputing then regenerates the correct shape but placement is untouched as recompute has no reason to change it. Should we then actually forbid changing the shape of these objects? Ideally IMO assigning a shape shouldn't change the placement at all but I don't know how complicated that would be.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Ticket #4754: recompute undos removeSplitter

Post by openBrain »

adrianinsaval wrote: Thu Jan 27, 2022 5:41 pm Should we then actually forbid changing the shape of these objects? Ideally IMO assigning a shape shouldn't change the placement at all but I don't know how complicated that would be.
There is a global problem. I have no idea what the best solution would be. @wmayer probably could be of good advice here, because he may know about the impacts of choosing this or that solution. ;)
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Ticket #4754: recompute undos removeSplitter

Post by adrianinsaval »

Yes, I'm talking out of my ass here, I have no idea were any of this is handled in code or the implications. I guess this is way above our paygrade ;) :lol:
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Ticket #4754: recompute undos removeSplitter

Post by wmayer »

openBrain wrote: Thu Jan 27, 2022 10:55 am I have absolutely no idea where the magics take place in the code when assigning a new value to '.Shape'. :?
adrianinsaval wrote: Thu Jan 27, 2022 5:41 pm Should we then actually forbid changing the shape of these objects? Ideally IMO assigning a shape shouldn't change the placement at all but I don't know how complicated that would be.
An object's placement is always kept in sync with the placement of its shape. This means when you change the placement of an object it will be applied to the shape.

And when you assign a new shape to an object then its placement will change, too. However, in a feature's execute() function there is a mechanism implemented that checks if the shape is being changed during a recompute and if yes the object's placement is applied to the shape. This is to make sure that object placement and shape placement is equal.

So, this means if you call removeSplitter() inside the execute() method

Code: Select all

class MyFeature:
  def execute(self, fp):
     shape = ...
     fp.Shape = shape.removeSplliter()
you don't have to worry about the placement. But if you call removeSplitter() outside the context of a recompute it's up to you to handle the placement yourself. The above code snippet has to be changed to:

Code: Select all

from freecad import app as FreeCAD

import Draft
doc = FreeCAD.newDocument("testDoc")
box1 = doc.addObject("Part::Box", "Box1")
doc.recompute()
box2 = Draft.move(box1, FreeCAD.Vector(1, 0, 0), copy=True)
placement = box2.Placement
box2.Shape = box2.Shape.removeSplitter()
box2.Placement = placement
doc.recompute()
or alternatively

Code: Select all

from freecad import app as FreeCAD

import Draft
doc = FreeCAD.newDocument("testDoc")
box1 = doc.addObject("Part::Box", "Box1")
doc.recompute()
box2 = Draft.move(box1, FreeCAD.Vector(1, 0, 0), copy=True)
shape = box2.Shape.removeSplitter()
shape.Placement = box2.Placement
box2.Shape = shape
doc.recompute()
So, issue #4754 actually isn't a bug but the pitfalls should be documented.
Post Reply