Ticket #84 (closed defect: fixed)

Opened 2 years ago

Last modified 1 year ago

cloneTree doesn't handle attachments

Reported by: allenb Assigned to: cneumann
Priority: major Milestone: 2.0 Beta
Component: System Version: 2.0
Keywords: Cc:
Completion: 2006/12/08

Description

There is a problem with OSG::cloneTree's handling of attachments. It doesn't do anything with them at all. :)

This can be seen in the OSGNodeTest.cpp unit test.

Attachments

cloneTree_01.diff (124.6 kB) - added by cneumann on 11/06/06 12:30:41.
final (?) version of the patch.

Change History

10/08/06 07:08:55 changed by cneumann

There was some discussion on the core list about this, I tried to summarize/reproduce it here:

Carsten: Should cloneTree be fixed to clone attachments or is the test bogus ?

Dirk: the idea is for cloneTree to clone attachments, too, so that's the part that needs to be fixed. The test is just for not forgetting about it.

Gerrit: yes and no, 1.x also did the attachment cloning only for deepClone. My idea would be more to make it optional for cloneTree so that the user can decide if it is needed or not.

(follow-up: ↓ 3 ) 10/08/06 07:30:19 changed by cneumann

Ok, making it optional is nice. If Attachments are the only case for which the behavior of cloneTree should be customizable would something like this be ok ?

#ifdef OSG_1_COMPAT
NodePtr OSG::cloneTree(NodePtrConstArg pRootNode, bool cloneAttachments = false)
#else
NodePtr OSG::cloneTree(NodePtrConstArg pRootNode, bool cloneAttachments = true)
#endif

Or do we need a more general interface here, similar to the share argument of deepCloneTree (can't think of what it should control though right now) ?

(in reply to: ↑ 2 ; follow-up: ↓ 4 ) 10/08/06 08:53:48 changed by anonymous

Replying to cneumann:

Ok, making it optional is nice. If Attachments are the only case for which the behavior of cloneTree should be customizable would something like this be ok ? {{{ #ifdef OSG_1_COMPAT NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, bool cloneAttachments = false) #else NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, bool cloneAttachments = true) #endif }}} Or do we need a more general interface here, similar to the share argument of deepCloneTree (can't think of what it should control though right now) ?

What would cloneAttachments=true mean? Would we do a deep clone of the attachments or just make the new nodes hook up links to the attachments?

I think we need something more like:

NodePtr OSG::cloneTree(NodePtrConstArg pRootNode, std::vector<std::string> notShared=list_of("attachments"))

(in reply to: ↑ 3 ; follow-up: ↓ 5 ) 10/08/06 09:15:57 changed by cneumann

Replying to anonymous:

Replying to cneumann:

Ok, making it optional is nice. If Attachments are the only case for which the behavior of cloneTree should be customizable would something like this be ok ? {{{ #ifdef OSG_1_COMPAT NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, bool cloneAttachments = false) #else NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, bool cloneAttachments = true) #endif }}} Or do we need a more general interface here, similar to the share argument of deepCloneTree (can't think of what it should control though right now) ?

What would cloneAttachments=true mean? Would we do a deep clone of the attachments or just make the new nodes hook up links to the attachments?

If cloneAttachments==true attachments should be shared, to be in spirit with the behavior of cloneTree, while for cloneAttachments==false the 1.x behavior of simply ignoring attachments (i.e. the cloned tree will not have any attachments on nodes - since the cores are shared, they retain their attachments) should be reproduced - one could see this as "bug compatible" mode.

I think we need something more like: {{{ NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, std::vector<std::string> notShared=list_of("attachments")) }}}

I'm sorry and I know I started it, but now you are not explaining the intended semantics…

(in reply to: ↑ 4 ; follow-up: ↓ 6 ) 10/08/06 11:22:29 changed by allenb

Replying to cneumann:

Replying to anonymous:

Replying to cneumann:

Ok, making it optional is nice. If Attachments are the only case for which the behavior of cloneTree should be customizable would something like this be ok ? {{{ #ifdef OSG_1_COMPAT NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, bool cloneAttachments = false) #else NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, bool cloneAttachments = true) #endif }}} Or do we need a more general interface here, similar to the share argument of deepCloneTree (can't think of what it should control though right now) ?

What would cloneAttachments=true mean? Would we do a deep clone of the attachments or just make the new nodes hook up links to the attachments?

If cloneAttachments==true attachments should be shared, to be in spirit with the behavior of cloneTree, while for cloneAttachments==false the 1.x behavior of simply ignoring attachments (i.e. the cloned tree will not have any attachments on nodes - since the cores are shared, they retain their attachments) should be reproduced - one could see this as "bug compatible" mode.

But what if you want to include attachments but include a deep copy of them. For example if you clone a tree that has names attached you may want to change the names in the cloned tree but not have those changes affect the original version of the tree.

We need more explicit control over these things:

  1. What to ignore totally
  2. What to clone by copying a reference to the original
  3. What to copy by making a new copy of the data referenced and pointing at that

I think we need something more like: {{{ NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, std::vector<std::string> notShared=list_of("attachments")) }}}

I'm sorry and I know I started it, but now you are not explaining the intended semantics…

The semantics here would be to clone everything by default so that every reference found is copied into the the new nodes. Then the list of types in notShared would be deep copied instead ("not shared"). Still not perfect but it would allow more control.

Side comment: This is a complex issue and I don't know if we are ever going to get something that will cover all the use cases. :(

(in reply to: ↑ 5 ) 10/08/06 11:49:21 changed by cneumann

Replying to allenb:

But what if you want to include attachments but include a deep copy of them. For example if you clone a tree that has names attached you may want to change the names in the cloned tree but not have those changes affect the original version of the tree.

Since you have to locate the places with the name attachments anyway, you might as well not clone them and add new ones to the cloned tree or replace the shared ones before changing their value.

We need more explicit control over these things: 1. What to ignore totally 2. What to clone by copying a reference to the original 3. What to copy by making a new copy of the data referenced and pointing at that

I think we need something more like: {{{ NodePtr? OSG::cloneTree(NodePtrConstArg? pRootNode, std::vector<std::string> notShared=list_of("attachments")) }}}

I'm sorry and I know I started it, but now you are not explaining the intended semantics…

The semantics here would be to clone everything by default so that every reference found is copied into the the new nodes. Then the list of types in notShared would be deep copied instead ("not shared"). Still not perfect but it would allow more control. Side comment: This is a complex issue and I don't know if we are ever going to get something that will cover all the use cases. :(

Yes, it is complex, so we should not try to cover all cases, but instead find the common ones (maybe by asking the users for input ?) and make a good interface for those. For odd or exotic clone behavior a hand-crafted traversal is needed, but I think that's acceptable.

(follow-up: ↓ 8 ) 10/23/06 10:24:00 changed by cneumann

Ok, I'd like to propose the following, which covers most cases, I hope:

#ifdef OSG_1_COMPAT
cloneTree(NodePtr root, const std::vector<std::string>& clone = <empty>, const std::vector<std::string>& ignore = <"FieldContainerAttachment">);
#else
cloneTree(NodePtr root, const std::vector<std::string>& clone = <empty>, const std::vector<std::string>& ignore = <empty>);
#endif

clone is a list of types that are deep cloned, i.e. new objects with the same values are created, and ignore is a list of types that is well ignored. The strange <empty> and <"FieldContainerAttachment"> stuff is only to indicate the default values I want to use, got to figure out the proper syntax for that.

deepCloneTree(NodePtr root, const std::vector<std::string>& share = <empty>, const std::vector<std::string>& ignore = <empty>);

share retains its current meaning (share values instead of cloning) and ignore as above indicates types that are not copied at all. Docs will contain a big warning that if ignore is set the resulting tree may be unusable.

Comments ?

(in reply to: ↑ 7 ; follow-up: ↓ 9 ) 10/23/06 11:10:14 changed by anonymous

Looks good to me. One nitpick: I'm a little worried about efficieny problems with comparing strings, I'd really like to see them mapped to ids before running the actual traversal.

I was unsure about the usefulness of the ignore, but actually I like it, as it would us to use the cloneTree also as a filter for type-based subtrees.

So overall: I'd say go for it.

(in reply to: ↑ 8 ) 10/23/06 11:40:55 changed by cneumann

  • owner changed from unassigned to cneumann.

Replying to anonymous:

Looks good to me. One nitpick: I'm a little worried about efficieny problems with comparing strings, I'd really like to see them mapped to ids before running the actual traversal.

yes, I was considering this too, but since most of the heavy lifting is actually done inside FieldDescription, I'd be interested in what Gerrit has to say about switching over to type ids.

I was unsure about the usefulness of the ignore, but actually I like it, as it would us to use the cloneTree also as a filter for type-based subtrees. So overall: I'd say go for it.

I'll see what I can come up with and post a patch here.

10/29/06 14:12:19 changed by cneumann

  • owner changed from cneumann to anonymous.
  • status changed from new to assigned.

attached a partial patch.

I'm a bit stuck at this point, because the original cloneTree/deepCloneTree code is broken for the same reason: There are specializations of FieldDescription≠ for Ptr fields, but they live in OSGFieldContainerFields.cpp and OSGAttachmentMapFields.cpp and since these are .cpp files these specializations are basically invisible to the compiler and never used. It falls back to the primary template which unconditionally asserts for the relevant functions: cloneValues, shareValues.

10/31/06 03:42:48 changed by cneumann

  • owner changed from anonymous to cneumann.
  • status changed from assigned to new.

10/31/06 03:42:54 changed by cneumann

  • status changed from new to assigned.

(follow-up: ↓ 14 ) 11/01/06 15:53:07 changed by cneumann

I updated the patch and more or less figured out why the FieldDescription magic actually works, contrary to what I ignorantly claimed in my previous comment. The patch itself is feature complete, but hardly tested.

TODO:

  • test it
  • remove old deepClone for FieldContainer and infrastructure in FieldDescriptionBase et al.

11/06/06 12:30:41 changed by cneumann

  • attachment cloneTree_01.diff added.

final (?) version of the patch.

(in reply to: ↑ 13 ) 11/06/06 12:35:04 changed by cneumann

  • estimated_completion changed.

Replying to cneumann:

I updated the patch and more or less figured out why the FieldDescription magic actually works, contrary to what I ignorantly claimed in my previous comment. The patch itself is feature complete, but hardly tested. TODO: * test it * remove old deepClone for FieldContainer and infrastructure in FieldDescriptionBase et al.

updated the patch once more. Completed the above TODO items and added default arguments, so that old code continues to work.

Comments and further testing greatly appriciated ;)

12/01/06 07:07:47 changed by cneumann

  • estimated_completion set to 2006/12/08.

I'll commit this on 2006/12/08 unless somebody object until then.

12/08/06 13:34:23 changed by cneumann

  • status changed from assigned to closed.
  • resolution set to fixed.

committed as r447