Page 2 of 3

Re: [PR #3822] Strange behavior of copyObject

Posted: Sun Aug 30, 2020 11:00 am
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?

Re: [PR #3822] Strange behavior of copyObject

Posted: Sun Aug 30, 2020 11:48 am
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.

Re: [PR #3822] Strange behavior of copyObject

Posted: Sun Aug 30, 2020 12:37 pm
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.

Re: [PR #3822] Strange behavior of copyObject

Posted: Sun Aug 30, 2020 3:00 pm
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.

Re: [PR #3822] Strange behavior of copyObject

Posted: Sun Aug 30, 2020 3:37 pm
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.

Re: [PR #3822] Strange behavior of copyObject

Posted: Sun Aug 30, 2020 5:12 pm
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?

Re: [PR #3822] Strange behavior of copyObject

Posted: Mon Aug 31, 2020 1:08 am
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".

Re: [PR #3822] Strange behavior of copyObject

Posted: Mon Aug 31, 2020 5:42 am
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?

Re: [PR #3822] Strange behavior of copyObject

Posted: Mon Aug 31, 2020 9:28 am
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().

Re: [PR #3822] Strange behavior of copyObject

Posted: Mon Aug 31, 2020 10:20 am
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!