[Merged] [PR #3822] Strange behavior of copyObject

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
wmayer
Founder
Posts: 18818
Joined: Thu Feb 19, 2009 10:32 am

Re: [PR #3822] Strange behavior of copyObject

Post by wmayer »

Merged.
chrisb wrote: Tue Aug 25, 2020 10:53 pm IIRC there is a function returning the placement in the global coordinate system. What does it yield for the cube, if it appears in two positions?
The function is called getGlobalPlacement() (Python) and globalPlacement() (C++) which checks its "in-list" and stops after the first parent object it finds.
I have submitted a PR to throw error in case of copy GeoFeatureGroup/OriginGroup/Part without dependency.
When duplicating the part container without sub-elements it creates a new completely empty part container which so far is correct behaviour.

However, a part container without an origin gets into a broken state as soon as you want to add an object. And it's a bit tricky to fix a broken container manually.

I wonder whether there is any sensible use case where a part container (or body object) doesn't have an origin or shares it with another container (the PR makes it more difficult but not impossible)?
If not, doesn't it make sense to make sure that a part container or body object always have its own origin?
chrisb
Veteran
Posts: 43701
Joined: Tue Mar 17, 2015 9:14 am

Re: [PR #3822] Strange behavior of copyObject

Post by chrisb »

wmayer wrote: Sun Aug 30, 2020 11:00 am The function is called getGlobalPlacement() (Python) and globalPlacement() (C++) which checks its "in-list" and stops after the first parent object it finds.
Thanks for the clarification.
If not, doesn't it make sense to make sure that a part container or body object always have its own origin?
It makes perfectly sense that Parts and even more so bodies have their own origin. A Part without origin is nothing more than the good old group.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
carlopav
Veteran
Posts: 2045
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [PR #3822] Strange behavior of copyObject

Post by carlopav »

wmayer wrote: Sun Aug 30, 2020 11:00 am Merged.
thanks!
If not, doesn't it make sense to make sure that a part container or body object always have its own origin?
+1 for me too.
follow my experiments on BIM modelling for architecture design
vocx
Veteran
Posts: 5205
Joined: Thu Oct 18, 2018 9:18 pm

Re: [PR #3822] Strange behavior of copyObject

Post by vocx »

wmayer wrote: Sun Aug 30, 2020 11:00 am ...
If not, doesn't it make sense to make sure that a part container or body object always have its own origin?
If I remember correctly, in one of his branches, realthunder decided to make the creation of the origin optional because in that case the number of document objects is reduced, saving memory and disk size.

Now, what would the use case be? I am not entirely sure, but I remember that it was discussed at some point in one of the assembly threads (assembly 3 or assembly4); maybe it was in the context of discontiguous Bodies, that is, multiple solids in a single Body.
Always add the important information to your posts if you need help. Also see Tutorials and Video tutorials.
To support the documentation effort, and code development, your donation is appreciated: liberapay.com/FreeCAD.
chrisb
Veteran
Posts: 43701
Joined: Tue Mar 17, 2015 9:14 am

Re: [PR #3822] Strange behavior of copyObject

Post by chrisb »

If it was just for saving space on disk, origins having the default placement (0,0,0), 0° could be omitted on write and restored on load. I don't think the additional code needed to do the same for Parts and Bodies in memory during runtime would justify the saving. Each such object would need a proxy to handle calls to the origin.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
carlopav
Veteran
Posts: 2045
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [PR #3822] Strange behavior of copyObject

Post by carlopav »

I compiled the branch and tested the fix trying to copy an App::Part object.

if I run App.ActiveDocument.copyObject(obj) from the console, only the App::Part object is copied, but it does not have any orgin.
If I copy with Ctrl+C I can deselect every dependency, resulting again in an App::Part object without the origin.
If I use Draft move command with Copy option enabled (App.ActiveDocument.copyObject(obj, with_dependencies=False, return_all=False)) this result again in an App::Part without origin.

So:

- I think the origin should always be copied, if not the error Can't find Origin for "Unnamed#Part002" is prompted.

- We can decide if Draft move with copy enabled should be modified to:
. - always copy dependencies (maybe not);
. - always copy dependencies when an App::Part object is processed (maybe yes);
. - add another option so user can copy or deep copy objects (too confused);
. - add a copy command (like the BIM Workbench copy command) where the user can chose if he wants to copy or deep copy and leave the move command like it is now (maybe the better option to me).


What do you think?
follow my experiments on BIM modelling for architecture design
realthunder
Veteran
Posts: 2164
Joined: Tue Jan 03, 2017 10:55 am

Re: [PR #3822] Strange behavior of copyObject

Post by realthunder »

I wonder whether there is any sensible use case where a part container (or body object) doesn't have an origin or shares it with another container (the PR makes it more difficult but not impossible)?
If not, doesn't it make sense to make sure that a part container or body object always have its own origin?
Added a PR to auto create Origin if copied without dependency.

carlopav wrote: Sun Aug 30, 2020 5:12 pm - We can decide if Draft move with copy enabled should be modified to:
. - always copy dependencies (maybe not);
. - always copy dependencies when an App::Part object is processed (maybe yes);
. - add another option so user can copy or deep copy objects (too confused);
. - add a copy command (like the BIM Workbench copy command) where the user can chose if he wants to copy or deep copy and leave the move command like it is now (maybe the better option to me).
There is really no way to decide this for the user. Why not reuse the object selection dialog when copy the object, using command "Std_DuplicateSelection".
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
carlopav
Veteran
Posts: 2045
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [PR #3822] Strange behavior of copyObject

Post by carlopav »

realthunder wrote: Mon Aug 31, 2020 1:08 am There is really no way to decide this for the user. Why not reuse the object selection dialog when copy the object, using command "Std_DuplicateSelection".
That's true, but also you have to consider that while drafting the user use copy a lot more than when modelling and could be bothered by that behaviour. So maybe I'd prefer to have a default (do not copy dependencies?) and leave the "Std_DuplicateSelection" as an option.

Could it be like that?
follow my experiments on BIM modelling for architecture design
realthunder
Veteran
Posts: 2164
Joined: Tue Jan 03, 2017 10:55 am

Re: [PR #3822] Strange behavior of copyObject

Post by realthunder »

carlopav wrote: Mon Aug 31, 2020 5:42 am That's true, but also you have to consider that while drafting the user use copy a lot more than when modelling and could be bothered by that behaviour. So maybe I'd prefer to have a default (do not copy dependencies?) and leave the "Std_DuplicateSelection" as an option.

Could it be like that?
You may also consider use 'with_dependencies' as the default behavior. When App::Link is found in the dependencies, copyObject() with_dependency will not include the linked object, but it will duplicate the link itself. In case you do want to duplicate everything including the linked object, you can manually call App.getDependentObjects(obj) to obtain the full dependency list, and pass the returned list of objects to copyObject().
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
carlopav
Veteran
Posts: 2045
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [PR #3822] Strange behavior of copyObject

Post by carlopav »

realthunder wrote: Mon Aug 31, 2020 9:28 am You may also consider use 'with_dependencies' as the default behavior.
It's good to me, but I'm not sure Yorik could agree: at the moment the default is without dependency and I remember some discussion where he preferred it to stay like this.
yorik wrote:ping
EDIT: Perhaps we can leave like this the Move with copy, but create another Copy command that works in the way you suggest!

When App::Link is found in the dependencies, copyObject() with_dependency will not include the linked object, but it will duplicate the link itself. In case you do want to duplicate everything including the linked object, you can manually call App.getDependentObjects(obj) to obtain the full dependency list, and pass the returned list of objects to copyObject().
That's something interesting that can substitute some duplicated Draft python api probably. I have to investigate it, thanks for the tip!
follow my experiments on BIM modelling for architecture design
Post Reply