cloneTree doesn't handle attachments

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.

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) ?

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"))

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…

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.

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.

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 ?

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.

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.

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.

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.

final (?) version of the patch.

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

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

committed as r447