Add Component::extractComponent and Component::removeComponent
#4174
Conversation
|
This is ready for review, perhaps @nickbianco ? |
There was a problem hiding this comment.
LGTM. Nice work @adamkewley, these will be super useful going forward.
I had one minor comment, but it's not blocking.
| std::unique_ptr<T> extractValueAtIndexVirtual(int index) override | ||
| { | ||
| SimTK::ClonePtr<T>* p = &objects.at(index); | ||
| std::unique_ptr<T> extracted{p->release()}; |
There was a problem hiding this comment.
You use SimTK::ClonePtr here, but not in the implementation in SimpleProperty. I'm 99% sure that's because there's no reason to use SimTK::ClonePtr for simple property types, but just double checking.
There was a problem hiding this comment.
The storage for an ObjectProperty is Array<ClonePtr<ObjectProperty>> whereas the storage for a SimpleProperty is Array<T>, so the SimpleProperty version needs to heap-allocate in order to satisfy the uniform API of the function returning unique_ptr<T>.
The reason it returns unique_ptr<T> is so that the interface is uniform for all Ts:
extractProperty<ContactGeometry>
extractProperty<int>
etc.
There was a problem hiding this comment.
Ah yes, I was looking at this backwards. Thanks for the clarification.
nickbianco
deleted the
feature_remove-or-extract-components-from-component
branch
Fixes issue N/A
This PR adds
Component::extractComponentandComponent::removeComponentto theComponentAPI.Abstractly, the motivation for doing this is that the
ComponentAPI currently only supports adding generic components viaComponent::addComponentbut has no generic way to remove components added via that method. Therefore, if downstream code wants to useComponentas a composite type (presumably intended, given the API design), that code can only use it to build trees, but can't easily edit or manipulate a built one.Concretely, OpenSim Creator has been patching opensim-core downstream with this diff so that it can provide some basic support for deleting things from an OpenSim model (link).
The
extractComponentvariant allows the caller to take ownership of an object in aComponent'sComponentSet. This is useful for (e.g.) moving a subcomponent from one owner to another one. OpenSim Creator uses it to support moving markers from bodies to the model's markerset at runtime (link) but it could be used, in principle, to move any subcomponent (i.e. stuff in the genericComponentSet) from any owner to any other one.Brief summary of changes
Component::removeComponentandComponent::extractComponentProperty<T>::extractComponentto thePropertyAPI, which is required because the<components>(ComponentSet) field of aComponentis stored as aProperty.Property<T>::removeComponentto internally useextractComponent: it has the same side-effect (removal of the component) but assumes the caller wants to immediately drop the componentProperty<T>::removeComponentVirtualand replaces it withProperty<T>::extractComponentVirtual. The side-effects are the same (component removal) but the extraction method also ensures that the caller can get access to the thing that was removed. This has a small penalty (of allocation, in the simple property case), but property removal is rare (only one usage in opensim-core) and will not be part of a simulation hot loop. It is unlikely that downstream code implements its ownProperty<T>specialization.Testing I've completed
Looking for feedback on...
CHANGELOG.md (choose one)
This change is