VisualNode & CollisionNode

[mxgrey]

I think it would be appropriate to begin a conversation about the VisualNode/CollisionNode concept which is intended to replace the current method of using purely the Shape class to provide visualization and collision information. I don't anticipate this being finished any time before version 5.1, but I think it's worth opening the conversation now to give us some time to share ideas and flesh out a design.

To review, the current way Shapes are used is: (i) The Entity class has a std::vector of std::shared_ptr<Shape> which it uses for visualization (ii) The BodyNode class has a std::vector of std::shared_ptr<Shape> which it uses to check collisions

The nice thing about this scheme is that it allows us to share shape information between multiple Entities/BodyNodes which saves on memory. The downside is that Shape has several features, such as Shape::setLocalTransform and Shape::setColor, which could result in destructive interference if two Entities are sharing the Shape but want that shape to have a different local transform or a different color.

Another issue with the current scheme is it does not allow a single BodyNode to have different parts with different physical properties, like coefficients of friction and restitution. A real physical body will have different friction properties on different parts, and different restitution properties on different parts, but DART cannot currently model that. Making friction and restitution properties part of a CollisionNode and allowing a BodyNode to have multiple CollisionNodes would address this issue.

Design suggestions

I think there is an interest in having the VisualNode and CollisionNode classes inherit the Frame class. This makes sense, because we want these Node classes to have a local transform property, and it would be convenient to have a reference frame tied to the Shapes. However, there is a bit of a conflict here: if we make a VisualNode responsible for visualization and we make VisualNode a Frame, then this means that we might need to remove visualization features from Entities, because classes that are derived directly from Entity (rather than from Frame) are expected to leaf nodes in the kinematic tree, so it would not make sense for a raw Entity to have child VisualNodes (since this would contradict the idea that the Entity is a leaf).

So I have two suggestions to offer for how to resolve this:

(1) We remove all visualization features from Entity. This would make the raw Entity class far less valuable, but maybe that's okay. Instead of creating an Entity to render an arbitrary (and non-physical) Shape in a scene, the user would have to create a SimpleFrame (which would inherit VisualNode) and fill it with some Shapes.

(2) We put all the visualization API into Entity. We would also create a VisualNode class that inherits the Frame class, but its API for visualizing shapes will be entirely derived from Entity. This means that everything that inherits Entity will be able to contain its own Shapes, and the visualization of all those Shapes could be manipulated by the user. My main concern here is this could result in confusion: The BodyNode class would be able to contain visualization Shapes, but it would also have VisualNodes that can contain visualization Shapes, so the user might be confused about which to use when trying to manipulate visualizations.

Of these two options, I think I would prefer (1).

Visual vs. Collision Node

The purposes of the VisualNode and the CollisionNode would be slightly different: VisualNode would be an interface for manipulating how things get rendered, while CollisionNode would contain some physical property data which describe how collisions should be handled.

@psigen brought up how this type of separation between visualization and collision can be troublesome, and he requested that the concepts of the two classes be merged into a single ShapeNode class. Indeed, at the core of the two classes there is just a std::vector of std::shared_ptr<Shape> and a local transform, but the ShapeNodes being used for visualization would still need to be kept separate from the ShapeNodes being used for collision, because you do not always want to render everything that's used for collision checking, and you don't always want to collision check everything that's used for visualization. So rather than a ShapeNode class that has all the features of both, I would propose just having a way to implicitly convert a VisualNode into a CollisionNode and a CollisionNode into a VisualNode. An implicit conversion should address the main source of inconvenience that arises from keeping visualization and collision data separate.

[mkoval]

The proposed design makes sense to me. I agree that we should keep CollisionNode and VisualNode separate. There is no reason to, e.g. require coefficients of friction on a VisualNode or color on a CollisionNode. I do think it is important to have a method for converting CollisionNodes to VisualNodes for debugging purposes (i.e. visualizing the collision geometry to figure out why two BodyNodes are in collision). However, I see no reason to make the conversion implicit.

I don't understand the implications of (1) versus (2). I don't understand why Entity must be "a leaf node in the kinematic tree." This seems like an artificial limitation. It is also not true of `BodyNode: a BodyNode is an Entity, but many BodyNodes have children.

I am already quite confused about option (2), so I agree that I prefer option (1). :smile:

@psiegen Should share his thoughts on the CollisionNode versus VisualNode debate. I know he feels strongly about this.

[psigen]

Thanks for starting this thread @mxgrey. I need some time to put together a response, I do have some opinions on this. Will try to have something tomorrow.

[mxgrey]

I don't understand the implications of (1) versus (2). I don't understand why Entity must be "a leaf node in the kinematic tree."

The auto-updating and Frame semantics for forward kinematics works by propagating update notifications downstream through the kinematic tree and having a kinematic tree built from Frames. The expectation is that an Entity will always descend from a parent Frame. A Frame is always an Entity, but an Entity is not necessarily a Frame (for example, there is an experimental branch of DART where we have Point, Velocity, and Acceleration classes which are Entities but not Frames). In order to know its Frame of reference, an Entity must have a Frame as its parent in the tree. It would not make sense for something to have an abstract Entity as its parent.

It is also not true of `BodyNode: a BodyNode is an Entity, but many BodyNodes have children.

But a BodyNode is a Frame, so it does not need to be a leaf in the kinematic tree.

Consider this: Would it make sense to give a child Frame to a Point, a Velocity, or an Acceleration?

[psigen]

Thanks very much for opening this discussion @mxgrey.

For the most part, I agree with (1). But I would like to point out some observations and use-cases that I think are useful to consider, and a counter-proposal that I think meets all of our needs more fully.

Apologies in advance for the extensive brain dump that follows. You can skip to the pseudocode at the end if it gets too dry.

Issues/ambiguities in existing proposal

(1) There is nothing special about 'Visual' and 'Collision' vs other uses of geometry.

Besides being used for collision and rendering, geometry has a variety of other uses:

• Distance field computation. CHOMP approximates the robot using a set of spheres to evaluate signed distance. TrajOpt does the same using convex hulls. These shapes are not used for collision checks or rendering.
• Hitbox geometry for interaction or sensor-simulation (e.g. FLIR heat signature, WiFi permeability). These may have additional metadata, e.g. event handlers, and are not used for collision checks or rendering.
• Physics-simulation at varying levels of fidelity. E.g. a quasi-static simulation requires different geometry properties that a dynamical simulator.

My point here is that we cannot easily bin the properties associated with geometry into Collision and Visual categories.

(2) The current contents of 'Collision' and 'Visual' nodes are undefined/overfit.

The current Collision node includes some properties (e.g. coefficients of restitution) that only make sense for certain types of physical simulation. Here are two examples that break this assumption:

• Geometry that is used for collision checking during motion planning, but is not used in the physics simulation. Why do we need to specify coefficients of restitution or friction on these shapes?
• Alternative simulation modes; e.g. quasi-static simulation, will have properties that are not used by the dynamical simulator.

The current Visual node includes data that is useful for the current rendering pipeline. However, in the future, we may need additional data (e.g. GLSL displacement shaders or dynamic UV maps).

(4) It is useful for the same Node to have multiple types of metadata attached.

It is often useful for a single ShapeNode to have some combination of information. Here are a few examples:

• Moving and cloning. If the shape node is moved, deleted, or cloned, all relevant metadata moves with it.
• GUI Interaction. Interacting with a ShapeNode in a GUI, e.g. to delete it, will remove the associated meta-data.
• Debugging. It is possible to render any type of geometry, e.g. collision or distance field geometry, by adding visual metadata to it.
• Implementing functionality. Some functions might operate on multiple pieces of metadata, and it is convenient to pass them a ShapeNode without enforcing any arbitrary conventions.

(5) The other place where this distinction occurs: between RigidBodyNodes and SoftBodyNodes, already seems to stand out as being weirdly artificial.

I mean, doesn't that seem incredibly arbitrary to you guys as well? It immediately begs the question of why soft bodies have their own getter functions, and if there are other weird body types that will need special handling at the API level.

Requirements

I think we all agree that the following use-cases should be supported:

• There are multiple uses for geometry. The two most important uses are rendering and collision-checking. There are also other uses such as physics-simulation and optimization. Note that collision-checking and physics-simulation are not the same thing, do not always involve the same set of properties, and may vary independently.
• Most uses of geometry (at least as far as I can tell) involve a tuple of [shape, transform, metadata], where metadata is some arbitrary structure specific to the functional use.
• We will need to efficiently be able to query all ShapeNodes that also have a given type of information, such as rendering information or collision information.

Use cases that I care about

• Load a shape and use it for: (1) just for collision checking, (2) just for visualization, or (3) both.
• Visualize the exact shape that is being used for collision checking to debug why planning is failing. If I change the shape being used for collision checking, then it should also change in the visualization.
• I want to quickly query the following things with (roughly) equal speed:
  • only render geometry
  • only collision geometry
  • only the convex geometry for Trajopt
  • only the spheres used for CHOMP
• I have a set of convexified geometry for Trajopt. It should not have useless parameters like coefficients of restitution or collision masks, any more than render meshes should have to. There's nothing special about 'visual' vs. 'for Trajopt'.
• I want to visualize any collision shapes that are colliding and color them red.
• I want to color my Trajopt geometry with a hot-cold colormap based on the magnitude of their signed distance field evaluation.

Proposed structure

In light of the above, I've been considering the following:

First, above all else, let's avoid calls like getVisualNodes(), getCollisionNodes().

As outlined above, there are many use cases that don't fit neatly into these categories. These will often occur in user-code, at which point it is impossible to add analogous calls like getTrajoptNodes() and getPaddedNodes(). Instead, let's try to make an API that efficiently supports the query of 'Get all the things that have X kind of metadata', and let it support the user coming up with some new metadata they want to bundle with the ShapeNode.

As for the rest, I think it's best explained by some pseudocode:

// Define our info structures.
struct RenderInfo {
    Color mDiffuseColor;
    Color mSpecularColor;
    double mTransparency;
};

struct CollisionInfo {
    std::vector<CollisionGroup> mMyGroups;
    std::vector<CollisionGroup> mCheckedGroups;
};

struct DynamicsInfo {
    double mRestitution;
    double mRollingFriction;
    double mFriction;
};

// Create a ShapeNode from a geometry!
auto shape = MeshShape::load("/path/to/geometry.stl");
Transform T_0 = Transform.translate(0., 1., 0.);
ShapeNode node(shape, T_0);

// Create some info (make_shared()-style constructor forwarding).
// This is optional syntactic-sugar to make it easy to populate ShapeNodes.
node.create<RenderInfo>(
    diffuseColor,
    specularColor,
    transparencyScalar
));
node.create<CollisionInfo>(
    myParentCollisionGroups,
    myCheckedCollisionGroups
));
node.create<DynamicsInfo>(
    restitutionCoeff, 0., 0.
));

// Modify the info in-place.
DynamicsInfo *const nodeDynamics = node.get<DynamicsInfo>();
nodeDynamics->restitutionCoeff += 3.0;

// Check and add some new custom info from an external source.
struct TrajoptInfo {
    double distanceCoeff;
};

node.has<TrajoptInfo>(); // returns false
TrajoptInfo* const nodeTrajopt = node.get<TrajoptInfo>(); // returns null

TrajoptInfo trajoptInfo(someCostScalar);
node.set<TrajoptInfo>(trajoptInfo);

// Query for things on the kinematic tree that have particular info.
std::vector<ShapeNode> trajoptChildren = node.getChildrenWith<TrajoptInfo>()
std::vector<ShapeNode> alltrajOpt = node.getSkeleton().getChildrenWith<CollisionInfo>()

Of course the API above is looks magical, but I have some sketch of how it could work.

1. Common metadata (CollisionInfo, DynamicsInfo, and RenderInfo) is handled by template specialization on the get/set/create/has methods to access an optionally populated pointer inside ShapeNode. This makes the most performance-critical operations very low overhead (one additional pointer dereference).

   template <>
   CollisionInfo *RenderInfo::get<>()
   {
      return mCollisionInfo;
   }

2. User-defined metadata is stored an internal std::unordered_map<std::type_index, void *> and returned by dynamic_cast<T> (or, if performance is critical, reinterpret_cast<T>). If this becomes a bottleneck, then we can convert this special case to (1).

   template <class T>
   T *RenderInfo::get()
   {
    auto it = mGenericInfo.find(typeinfo(T));
    if (it != mGenericInfo.end()) {
      return reinterpret_cast<T *>(mGenericInfo->second);
    } else {
      return nullptr;
    }
   }

3) Adding/creating infos propagates caching up the kinematic tree. This allows all the usual caching tricks to be able to efficiently query any subtree for all children that have a particular type of info.

This gets me the primary thing that I care about, which is that any metadata that we want to pair up with a ShapeNode is equally valid, and that multiple pieces of geometry metadata can be attached to a ShapeNode without conflict and with tight coupling such that they move with the ShapeNode.

It also leaves room for customized ShapeNode classes to specialize their own mechanisms for particular info. For example, a child ShapeNode class could override the get specialization and dynamically return a color based on the current position on the mesh, or a collision condition.

[mkoval]

:+1: for @psigen's suggestion. We had a long debate about this last night and settled on this as a good compromise.

[mxgrey]

You bring up some really important points about extensibility that I hadn't considered.

(5) The other place where this distinction occurs: between RigidBodyNodes and SoftBodyNodes, already seems to stand out as being weirdly artificial.

I agree with this. I've been gradually weaning out every case where SoftBodyNodes are handled separately from RigidBodyNodes. I think there are only two cases remaining; I just haven't had time to do the plumbing on those yet.

I definitely like the ShapeNode with templated metadata concept. I was going to complain that this makes it hard to organize the really important node types, like visualization and collision, but this:

Common metadata (CollisionInfo, DynamicsInfo, and RenderInfo) is handled by template specialization on the get/set/create/has methods to access an optionally populated pointer inside ShapeNode.

addressed that complaint for me. If we make it so that a ShapeNode will know its parent BodyNode, then it can inform its parent when new metadata is created (like when you turn on visualization for a ShapeNode that used to only do collisions), and the parent BodyNode can then add it to the vector of visualization nodes for easy access.

Adding/creating infos propagates caching up the kinematic tree. This allows all the usual caching tricks to be able to efficiently query any subtree for all children that have a particular type of info.

I'm not 100% clear on why we would do this. To me, it would make more sense to just offer a convenience function that will crawl through a subtree and return references to all the ShapeNodes in the subtree that match the info type. I don't understand what we would gain by caching that information further upstream in the kinematic tree. Caching it in the parent BodyNode seems sufficient to me, and caching it any more than that seems like it would just make restructuring needlessly more expensive.

Other than the caching (which I don't understand the use for), I'm completely on board with this design concept. It's obviously more complicated to implement than my original proposal, but I think the payoff will be well worth the effort.

[psigen]

Excellent! I'm glad it makes sense to you.

With regards to the caching, the use-case that @mkoval and I had in mind was that a collision checker or renderer might want to make a request like herb.getChildrenWith<CollisionInfo> that returns a safe iterator or similar of ShapeNodes that contained CollisionInfo. Since this request might be done over a large tree (or the whole skeleton if you are trying to set up for a collision test), it seemed like it might be useful to cache the information somewhere, so that re-querying would be cheap.

It's a separate issue from the rest of the architecture, and primarily a performance optimization for potentially sparse lookups (maybe you'd have a few collision shapes, but tons of render shapes, or vice versa). We aren't strongly tied to it or anything.

[mxgrey]

I guess the question is whether it would be better to:

(1) Have changes to the ShapeNodes be an O(1) operation while querying for a certain ShapeNode type is an O(N) operation, or

(2) Have changes to the ShapeNodes be an O(N) operation while querying for a certain ShapeNode type is O(1)

When you factor in the all the memory allocation required for option (2), I think option (1) becomes more appealing. Especially since querying for a certain ShapeNode type means you're almost certainly going to be doing an O(N) operation on the returned value anyway, so reducing the query to O(1) doesn't seem to me like it accomplishes much. Whereas changing something about a ShapeNode would be an O(1) operation if you don't need to perform an O(N) caching operation.

But yeah, other than that, I'm completely on board. So I guess now we'll want to consider the exact breakdown of the different InfoTypes that we want to have native to DART.

[mkoval]

I agree with @mxgrey's analysis. I think it's best to scrap the caching at the API level. We may need to revisit this inside the collision checker, e.g. by maintaining an indexing using add and remove signals, if the overhead proves significant.

To take a stab at figuring out the Info types we need:

• CollisionInfo to flag a ShapeNode for collision checking (if present) and, in the future, to store additional information about collision groups.
• DynamicsInfo to store friction coefficients and coefficients of restitution
• VisualInfo to store color

Here's a sketch in C++ish pseudocode:

class CollisionInfo {
  // collision group information, once we decide on an API
};

class DynamicsInfo {
  void setGravityMode(bool _gravityMode);
  bool getGravityMode() const;

  void setFrictionCoeff(double _coeff);
  double getFrictionCoeff() const;

  void setRestitutionCoeff(double _coeff);
  double getRestitutionCoeff() const;
};

class VisualInfo {
public:
  Eigen::Vector3d getRGB() const;
  void setRGB(const Eigen::Vector3d& _rgb);

  const Eigen::Vector4d& getRGBA() const;
  void setRGBA(const Eigen::Vector4d& _rgb);
};

class ShapeNode : public Frame {
  ConstShapePtr const &getShape() const;

  BodyNode *getParentBodyNode();
  const BodyNode *getParentBodyNode() const;

  const Eigen::Isometry3d& getLocalTransform() const;
  void setLocalTransform(const Eigen::Isometry3d& _Transform);

  Eigen::Vector3d getOffset() const;
  void setOffset(const Eigen::Vector3d& _offset);

  template <class T> T *getInfo();
  template <class T> const T *getInfo() const;
  // Specialized on CollisionInfo, DynamicsInfo, and VisualInfo for efficiency.
};

@mxgrey @psigen Thoughts?

[psigen]

LGTM.

I'll defer to you and @mxgrey on the appropriate containerization of the info structures (whether to do pure data objects, getter/setter accessors, inheritance: I think you guys have a better handle on what's best practice from a language-style and DART convention point-of-view.

[mkoval]

@psigen and I have been working on this in our fork. We have fully implemented ShapeNode and partially implemented the standard property classes CollisionShapeProperties DynamicsShapeProperties, and VisualShapeProperties. We have not yet migrated the rest of DART to query shape properties through the new API.

Our main blocker is that we are confused by the Entity and Frame classes and are not sure how to properly integrate ShapeNode into the kinematic tree of a Skeleton. Our initial plan was to extend Frame and let DART automatically do forward kinematics for us. However, this is not enough: ShapeNodes should be owned (and cloned) with the Skeleton they are attached to. Additionally, it would be ideal to attach a ShapeNode to any Frame in a Skeleton, instead of only BodyNodes.

We tried to mimic the BodyNode and SoftBodyNode management logic inside Skeleton, but it required duplicating a lot of code. We have a few questions about how to proceed:

1. How should non-BodyNode subclasses of Entity be managed by the Skeleton?
2. What happens if an Entity's parent Frame is deleted?
3. When extending Frame, why do we have to manually call the Entity constructor as well?

In general, it would be nice if all Frames were first-class citizens, instead of only BodyNodes and SoftBodyNodes. I suspect that you ran into several of these issues with the EndEffector class as well.

@mxgrey Any thoughts or suggestions? Thanks!

[mxgrey]

As a slight tangent, I think CollisionShapeProperties, DynamicsShapeProperties, and VisualShapeProperties might make more sense if named CollisionNodeProperties, DynamicsNodeProperties, and VisualNodeProperties, but that's minor.

I agree that it would be ideal if the ShapeNode inherited Frame somehow.

Additionally, it would be ideal to attach a ShapeNode to any Frame in a Skeleton, instead of only BodyNodes.

As of right now, the only Frames in a Skeleton are the BodyNodes, but of course the EndEffector class will be coming up soon, and that is also a Frame. It should be straightforward to have a common base class for BodyNode and EndEffector so that they can both have ShapeNodes. And both BodyNode and EndEffector can clone all their ShapeNodes during their cloning process.

How should non-BodyNode subclasses of Entity be managed by the Skeleton?

I think the ShapeNodes should actually be managed by the BodyNode/EndEffector that they belong to, not by the Skeleton that they are in.

What happens if an Entity's parent Frame is deleted?

When a parent frame is destroyed, it moves all of its child Entities into the World Frame.

When extending Frame, why do we have to manually call the Entity constructor as well?

This is an unfortunate side effect of virtual inheritance. I needed Frame to virtually inherit Entity in order to avoid the diamond problem when combining two different derivations of Entity into one class. I definitely agree that the current scheme is annoying and feels redundant, so I'll gladly put some thought into how we can safely set up some default constructors to avoid needing to explicitly call the entire inheritance hierarchy when constructing the most derived class. The hard part is doing it "safely".

In general, it would be nice if all Frames were first-class citizens, instead of only BodyNodes and SoftBodyNodes.

The Frame class is really only meant to be a common interface for handling kinematics computations (although it does also handle some caching and the auto-updating for forward kinematics). It's a pure abstract class, so I don't really feel it deserves anything past second-class status.

I guess the current scheme does make it difficult (or maybe closer to impossible) to render an arbitrary kinematic tree that is not part of a Skeleton, if we want to use the ShapeNode concept.

My current proposal would be:

(1) ShapeNode inherits Frame (2) We have a class that manages ShapeNodes (let's call it ShapeNodeMgr for now). ShapeNodeMgr would virtually inherit Frame. (3) BodyNode, EndEffector, and SimpleFrame will inherit ShapeNodeMgr

With this setup, your render interface can take (1) Skeletons, and (2) a list of arbitrary ShapeNodeMgrs. Passing a Skeleton into the render interface will tell the renderer to render the entire Skeleton (including BodyNodes and EndEffectors). Passing a list (presumably std::vector or std::unordered_set) of ShapeNodeMgrs will tell it to render whatever is renderable in each of those ShapeNodeMgrs.

This would allow an easy interface for rendering Skeletons while also being open to the possibility of rendering arbitrary kinematic trees which might or might not be tied to any Skeleton.

The more desirable scheme would probably be a way to just pass a root Frame into the renderer and tell the renderer to render everything that can be rendered in the whole kinematic tree, descending from that root Frame. I can't think of an obvious way to do this except to make the basic Frame class itself capable of managing ShapeNodes. Maybe that would be preferable? I'll need to sleep on it.

[psigen]

Just got a chance to catch up. As usual, opinionated comments will follow:

Minor comments (Let's get this out of the way before the data dump...)

As a slight tangent, I think CollisionShapeProperties, DynamicsShapeProperties, and VisualShapeProperties might make more sense if named CollisionNodeProperties, DynamicsNodeProperties, and VisualNodeProperties, but that's minor.

Makes sense, if we think of the ShapeNode as more of a Node than a Shape.

This is an unfortunate side effect of virtual inheritance. I needed Frame to virtually inherit Entity in order to avoid the diamond problem when combining two different derivations of Entity into one class. I definitely agree that the current scheme is annoying and feels redundant, so I'll gladly put some thought into how we can safely set up some default constructors to avoid needing to explicitly call the entire inheritance hierarchy when constructing the most derived class. The hard part is doing it "safely".

Is it at all helpful to go the other way: treat Frame as a mixin?

ShapeNode Attachment

It seems to make sense for ShapeNodes (and probably other things) to be able to be attached to any other Entity because there's nothing particularly special about BodyNodes or EndEffectors as far as an attachment point goes. In fact, if we wanted to make a scene graph loader/renderer, it is highly likely that the loaded meshes would be positioned according to a large kinematic tree of non-physical entities.

For example, a fridge model might contain a number of shelves that are all referencing a common "shelf origin", which is rigidly attached to the "fridge origin", which has another child at some fixed offset for the fridge body. Thus the ability to attach ShapeNodes to rigid offsets from other ShapeNodes or non-physical Frame objects seems like a useful property for replicating/loading/exporting conventional scene graph representations. It is also semantically useful, as re-positioning a shelf can be done from some meaningful origin, and repositioning all shelves can be done by simply adjusting the transform of the root "shelf_origin" Entity.

This is a trivial example, but consider a more complicated situation: a fridge model in multiple parts, each of which uses a visual mesh, a padded collision mesh, and multiple meshes used for physics which have different coefficients of friction (e.g. glass inserts in the shelves). It makes sense to be able to adjust all of the meshes associated with a particular part (like a shelf) without (1) having to create multiple BodyNodes and FreeJoints for these static parts or (2) having to iterate and apply a relative transform to every related mesh of a particular part.

Kinematic tree organization

It seems somewhat myopic to make a special manager for ShapeNodes, rather than something on Entity or BodyNode that just handles "things in the kinematic tree". Rather, it seems to me that EndEffectors, ShapeNodes, SimpleFrames, and any other future objects that might get attached to the kinematic tree could all share the same attachment, ownership, and inheritance semantics.

This is because we don't really know ahead of time what useful rigid kinematic chains we might want to construct for certain cases. Modular components like grippers will have a lot of the above custom kinematic attachments, and we will want these pieces to move/clone/attach in similar ways. Under the current proposal, it seems like EndEffectors and ShapeNodes will have wildly different attachment APIs and semantics, for what seems like no especially good reason.

Model 1: Skeleton Ownership

Previously, @mkoval and I were considering of the entire Skeleton as an arbitrary kinematic tree of Entities, which happen to have some special constrained transformations (Joints) for one special type of Entity (BodyNode), and some fixed transformations for attaching anything else like Markers (yes, I know they are going away), EndEffectors, and ShapeNodes to the tree.

Under that model it made sense for EndEffectors, ShapeNodes, and BodyNodes to all be managed by the Skeleton, because all of these are just Entities (i.e. things in the kinematic tree). While they may do any arbitrary amount of work under the hood (like BodyNode), there's no reason that other entities necessarily need to be aware of this.

Model 2: BodyNode Ownership

In considering the problem a bit more, that there is a fair amount of specialization that goes into BodyNodes that make them different than other Node types. The Joint is a very special type of transform, because it is also the unit of cache invalidation. When a joint is affected by anything, rigidly-attached or downstream parts get dirtied. So perhaps a useful distinction is to divide up the kinematic subtree into (1) a tree of BodyNodes (2) each of which has a rigid kinematic tree attached to it. This distinction actually shows up in ad-hoc fashion in OpenRAVE, with a GetRigidlyAttachedLinks() function being distinct from the GetLinks() function.

Structurally, this is closer to what the existing Skeleton is doing. The key downside is that BodyNodes can't be connected via a string of non-BodyNode Frame relationships. If you want to attach your arm via a chain of rigid vertebrae to your base, I think you have to pre-compute the whole transform and use that between your BodyNodes directly, while the geometry could be a daisy chain of ShapeNodes (under above spec).

However, this has some nice properties in that the rigid kinematic subtrees can more or less be 'owned' by the BodyNode, at least in the sense of being attached and moving with it as necessary. This is a pretty consistent semantic that might be useful/identical to how EndEffectors would move around, in addition to any future things that we might want to attach to the world.

[mkoval]

If I understand correctly, it sounds like @psigen's "Model 2" is similar to what @mxgrey is suggesting. That seems like a good compromise.

However, I agree with @psigen that the attachment of all Frame (other than BodyNode) should share the same ownership and clone semantics; e.g. EndEffector, ShapeNode, and SimpleFrame. Instead of ShapeNodeManager, we could implement a generic EntityManager class.

This is still very rough, but this is what I am thinking:

• Every BodyNode has a unique parent Joint and is owned by a Skeleton
• Every Joint have a unique parent BodyNode (or nullptr) and is owned by a Skeleton
• Add two new classes EntityManager and ManagedEntity:
  • Each ManagedEntity is owned by an EntityManager
  • BodyNode inherits from EntityManager
  • EndEffector, ShapeNode, and SimpleFrame inherit from EntityManager, ManagedEntity, and Frame
• Ownership semantics:
  • ManagedEntity is managed by a shared_ptr
  • Each ManagedEntity is owned by at most one EntityManager
• Cloning semantics:
  • ManagedFrame has a virtual clone() method
  • FrameManager has a clone() method that clones itself and its ManageFrames.

This is likely much more complicated than it needs to be. Hopefully @mxgrey and @psigen will talk some sense into me. :smile:

A few other notes:

• BodyNode is not a ManagedEntity and, thus, cannot be attached to other Entity types.
• It is possible to attach an arbitrary hierarchy of Frames to a BodyNode.
• Do we need a dedicated EntityManager class? Or can we add functionality to Frame?
• Some of the logic of EntityManager does already exists inside Frame.

[mxgrey]

I think I'm mostly in agreement, except for these things:

the attachment of all Frame (other than BodyNode) should share the same ownership and clone semantics; e.g. EndEffector, ShapeNode, and SimpleFrame

SimpleFrame is a special type of Entity called a Detachable. The idea behind the Detachable is that the user has the right to move it to a new parent Frame at any time without any consequences (in contrast to the BodyNode which requires some nontrivial Skeleton management when changing its parent). I think SimpleFrame should continue to be a free agent, able to carelessly change its parent Frame.

ManagedEntity is managed by a shared_ptr

At least for EndEffector, this would run into the same problem that we'd have if we tried to manage BodyNodes with shared_ptrs: an EndEffector depends strongly on its parent BodyNode, and cannot be attached to arbitrary non-BodyNode Frames. If the parent BodyNode of an EndEffector is destroyed while the user still holds onto a shared_ptr<EndEffector>, then they're hold onto a garbage EndEffector which will cause problems if they try to do anything with it. We'll need a custom smart pointer type (similar to JointPtr) for EndEffectors. I would also prefer to not be able to change the parent BodyNode of an EndEffector, but that's something that can be negotiated since it wouldn't be necessarily impossible; I just think it might end up confusing and possibly harmful.

I'd be okay with using shared_ptrs for ShapeNodes if we're going allow ShapeNodes to be attached to arbitrary Frames, because the ShapeNode could always fall back on the World Frame if its parent Frame gets destroyed.

But in summary, I don't believe there's a one-size-fits-all solution for the various types of attachments. I think it would be reasonable to offer a few different methods of management & ownership, but I don't believe we'll be able to fit everything into identical semantics.

[mkoval]

At least for EndEffector, this would run into the same problem that we'd have if we tried to manage BodyNodes with shared_ptrs: an EndEffector depends strongly on its parent BodyNode, and cannot be attached to arbitrary non-BodyNode Frames.

Why it is not possible to attach an EndEffector to an arbitrary Frame? For example, the hierarchy BodyNode → SimpleFrame → EndEffector seems reasonable (and potentially useful, since I could put the SimpleFrame in a convenient location for specifying multiple children). See below for a few examples.

Now for the larger discussion... (sorry for the wall of text)

The Issue

I think the crux of the issue is whether a Skeleton is:

1. a tree of BodyNodes with some other types of Frames attached
2. a tree of Frames, some of which are BodyNodes (others may be EndEffector, ShapeNode, or SimpleFrame)

In either case, BodyNodes are special because they define the kinematic structure (through their parent Joints) and are the source of inertia. As a result, implementing Featherstone efficiently requires bookkeeping at the Skeleton level that is not necessary for other Frames. This means that every BodyNode must be owned by exactly one Skeleton.

The Compromise

@pkv and I came up with a compromise between (1) and (2). We propose to add a new class, tentatively called RigidFrame that represents an inertia-less coordinate frame rigidly attached to a BodyNode. RigidFrame will serve as the base class for EndEffector, ShapeNode, and any other inertia-less frames in the kinematic tree. Since it has no inertia, RigidFrame can be ignored by dynamics calculations.

Each RigidFrame will have a BodyNode or another RigidFrame as a parent. This produces a structure where each BodyNode has a tree of RigidFrames attached to it. In both cases, RigidFrame will have efficient access to its parent BodyNode; even if it is several layers up in the tree.

RigidFrame will share its lifetime with its parent BodyNode. Ownership will be managed using the same mechanism as BodyNodePtr; i.e. keeping any RigidFrame in scope will keep its parent BodyNode (and all other RigidFrames attached to it) in scope. Since BodyNode shares its lifetime with its Skeleton, this will also keep the owning Skeleton in scope.

Example Use Cases

• I want to attach an EndEffector on HERB's right palm and visualize its position with a sphere. I construct a EndEffector attached to the hand BodyNode and offset it to line up with the palm. I attach a ShapeNode to the EndEffector with an identity transformation. Moving the EndEffector automatically moves the ShapeNode that I am using for visualization.
• Same as above, except that I want to attach an EndEffector to a refrigerator's door handle. I attach an EndEffector to the ShapeNode that represents the door handle and add a small relative translation to align it with the center of the handle.
• I have multiple ShapeNodes representing the geometry of the door handle; e.g. a high fidelity mesh for visualization and a low-fidelity mesh for collision checking during motion planing. I want to move all of them at the same time, so I attach them all to a RigidFrame and change its relative transformation.

Closing Thoughts

The RigidFrame proposal introduces a clear ownership semantic. Every RigidNode is a member of the Skeleton's kinematic tree. The RigidNode is owned by its parent BodyNode and shares lifetime with its Skeleton. Bookkeeping at the BodyNode or Skeleton level, if necessary, can be implemented in a signal handler that is called when a RigidNode change parent BodyNodes.

Other Frames, that do not inherit from RigidNode, do not change in behavior.

@mxgrey Thoughts?

[mxgrey]

Why it is not possible to attach an EndEffector to an arbitrary Frame?

One of the key purposes of EndEffector is to perform inverse kinematics on it. This means that there needs to be generalized coordinates leading up to it. I suppose in principle we could attach an EndEffector to an arbitrary chain and then associate it with whatever the closest BodyNode in its lineage is. The only potential issue here would be that getParentFrame() and getParentBodyNode() would refer to different things, which might be confusing, but I guess as long as we make it clear that this is the case, it could be fine. I'll give this some more consideration.

I think the crux of the issue is whether a Skeleton is:...

I think your short description of the central issue was perfectly accurate.

The Compromise

Overall, I think I like the sound of it. I'm not sure if I like the naming of "RigidFrame" (calling it Rigid makes me think it's unmovable), but I don't have a better alternative to offer at the moment.

The only thing I'm a bit iffy on is where SimpleFrame might fit into this. I think it would be good if SimpleFrame could offer some of the same interface as ShapeNode, because I'd like to be able to render SimpleFrames easily. So I'd like to suggest one small adjustment to The Compromise:

ShapeNode would be a more abstract class that does not require being attached to a BodyNode, but just offers an extensible interface for defining and handling geometric data. SimpleFrame can inherit ShapeNode so that a SimpleFrame is renderable and can be used for collision detection. SimpleFrame will keep its current user-centric ownership semantics.

Then we have something like BodyShapeNode which inherits both ShapeNode and RigidFrame, so that it offers the ShapeNode render/collision/etc interface while having the same ownership semantics as EndEffector.

Additionally, I'm thinking that BodyNode should also inherit RigidFrame so that the semantics for BodyShapeNode and EndEffector can simply be "attachable to a RigidFrame", but this is probably an unimportant implementation detail right now.

[psigen]

It sounds like we're getting close to convergence. :+1:

Overall, I think I like the sound of it. I'm not sure if I like the naming of "RigidFrame" (calling it Rigid makes me think it's unmovable), but I don't have a better alternative to offer at the moment.

Additionally, I'm thinking that BodyNode should also inherit RigidFrame so that the semantics for BodyShapeNode and EndEffector can simply be "attachable to a RigidFrame"

Might I suggest.... the almighty, unqualified Node class, base for all managed Frames.

ShapeNode would be a more abstract class that does not require being attached to a BodyNode Then we have something like BodyShapeNode which inherits both ShapeNode and RigidFrame,

We can continue the delineation between Nodes and Frames here. Consider a ShapeFrame has all the properties that you would like: it is a Frame that also contains shape properties, etc. Then we have the class ShapeNode which is the managed version, inheriting from Node and ShapeFrame, which has all the additional management properties and constraints.

In this way, we have two clear semantics, Frame objects are not managed, do not keep things in scope, while Nodes will.

• EndEffector, ShapeNode, and anything else that we would expect to be tightly coupled to the kinematic tree will inherit from Node (previously called RigidFrame).
• A Node can only represent a fixed relative transform to its parent. The exception is a BodyNode, which is heavily specially-cased through Featherstone.

I'm not 100% sure on the details behind implementing this, @mkoval can probably help with that.

[mxgrey]

I think I'm okay with this.

One detail to consider: ignoring the special case of BodyNode, would Nodes like ShapeNode and EndEffector be transferrable between parent BodyNodes, or would they be permanently tied to the BodyNode upon which they are created? I think I would prefer the latter, if only to make ShapeNodePtr and EndEffectorPtr much more straightforward to implement (and there could be a TemplatedNodePtr class that would handle both).

[mxgrey]

I don't know how far you guys have gotten in implementing the Node concept, but I thought of an implementation detail that has become very important while working on the EndEffector class (which, as we've discussed, will inherit Node and follow Node's ownership semantics).

I think there are two conflicting ownership semantics that could be tricky to consolidate:

1. A Node, such as ShapeNode or EndEffector should be stored within the Skeleton. The user should not have to maintain a reference to one in order for it to remain in existence. This means that the Skeleton (or parent BodyNode, doesn't matter) should also be responsible for cleaning it up if the user wants it deleted, presumably by calling some kind of remove() function.
2. A Node should not get deleted if a strong reference to it still exists somewhere.

But I think I came up with a compromise that should work reasonably well:

1. There will be a NodePtr that allows users to hold strong references to the various Node classes.
2. Calling a remove() function will stage the Node for removal, but it won't actually be deleted from the Skeleton until its reference count drops to zero.

By the way, once I've wrapped up the InverseKinematics and State implementations, I plan on jumping in on the Node and ShapeNode development with you guys, wherever you happen to be with it.

[mxgrey]

It turns out that I desperately need to create the Node class in order to finish off my work on the EndEffector class, and it doesn't look like you guys have implemented it yet in your ShapeNode branch. So I think I'll go ahead and implement the Node class in my own branch, and then later on I'll merge in your ShapeNode work and have your ShapeNode class inherit my Node class.

Be sure to stop me if you've already implemented the Node class somewhere, and I managed to overlook it.

[mkoval]

We haven't implemented Node yet. Everything you described sounds reasonable. Only one question: Will BodyNode and SoftBodyNode now inherit from Node?

[mxgrey]

My current plan is that, yes, BodyNode will have Node as a base class, and it can be treated as if it's a Node, but its actual underlying ownership semantics will remain unchanged. My logic is this:

We'll define Node as an object which is tied to a BodyNode and therefore depends on some BodyNode to exist. A BodyNode can still qualify as a Node because it is "tied" to itself and requires itself to exist in order for itself to exist.

The second sentence is clearly tautological, but I think that's okay.

My primary motive for making BodyNode a Node is because I want to have a class called JacobianNode that would represent a Node with an API for Jacobian-related functions. I want both BodyNode and EndEffector to inherit from that class so that I can have the InverseKinematics class simply operate on JacobianNodes, and then it can be completely agnostic about whether the thing it's operating on is a BodyNode, an EndEffector, or some other class that's inheriting from JacobianNode. I also want to have a strong InverseKinematicsPtr that holds a strong reference to the JacobianNode of the InverseKinematics module that it's holding. In order for that to work with minimal overhead, I need a single pointer class that is agnostic about whether it's holding a strong reference to an EndEffector or a BodyNode.

This means that I'll want to have a NodePtr class that can hold anything that inherits from Node, including BodyNode. So you'll be able to hold a strong reference to a BodyNode using either a NodePtr<BodyNode> or a BodyNodePtr. I need to keep the current BodyNodePtr class the way it is, because I believe the NodePtr class will need to use BodyNodePtr.

The NodePtr class will enable the InverseKinematicsPtr to hold a strong reference to its JacobianNode simply by holding onto a NodePtr<JacobianNode>.

It's kind of convoluted, and I haven't 100% fleshed out the implementation details, but I feel pretty sure that it will work. I'm going to try my best to have no more than two reference counts being performed by the NodePtr class (one count would be number of references for the BodyNode and the other count would be number of references for the Node), and I'm going to try to completely avoid virtual function calls so that we don't need to worry about dynamic resolution overhead in any of our smart pointer classes.

[mxgrey]

From earlier in this thread, the following issue was brought up:

When extending Frame, why do we have to manually call the Entity constructor as well?

I believe I've come up with an okay solution to this. You'll still have to call the Entity constructor, but it will simply be Entity(ConstructFrame) instead of needing to figure out what values to actually pass in. It will work like this:

Entity class will have some single-purpose enums and a constructor associated with each:

class Entity
{
public:
  enum ConstructFrame_t { ConstructFrame };
  enum ConstructAbstract_t { ConstructAbstract };

  // Normal constructor
  Entity( Frame* _refFrame, const std::string& _name, bool _quiet );

  // Constructor for extensions of the Frame class
  Entity( ConstructFrame_t );

  // Constructor for pure abstract classes that only need to call the Entity constructor as a formality
  Entity( ConstructAbstract_t );
}

Then when you construct your extension of the Frame class, all you need to do is this:

class DerivedFrame : public Frame
{
public:
  DerivedFrame( Frame* _refFrame, const std::string& _name )
    : Entity( ConstructFrame ),
      Frame( _refFrame, _name )
  { }
};

I can't think of any way to completely avoid calling the Entity constructor in the most derived class without either: (1) being killed by diamond of death problem or (2) running the risk of custom Entities being constructed incorrectly. My proposed approach should at least make the virtual inheritance of Entity much cleaner and less ambiguous.

[psigen]

@mkoval and I discussed the diamond of death going on here for some time, and I don't think we could come up with anything much better.

I think one idea that we had was to try to make Frame some sort of pure mixin class and have SimpleFrame and DetachableFrame and all of that do multiple inheritance from Entity and Frame.

@mkoval: do you recall anything else useful from this discussion?

[mxgrey]

@mkoval and I were talking last night, and we came to some conclusions about improvements for the design, so I thought I'd lay them out here for discussion.

(1) We realized that type erasure makes cloning difficult/impossible. Instead of using type erasure, we think we should have a base class, which I will tentatively refer to as the Addon class (but I'm open to better suggestions for names). The Addon class will have a pure virtual clone function. This didn't come up last night, but I think it should also have a virtual getState function to allow the Addons to have a recordable state. By default, it could just return a nullptr, which would indicate that it is stateless.

(2) We think this addon/plugin concept that we've planned for the ShapeFrame class would most likely benefit Nodes in general, so we'd like to shift the functionality from ShapeFrame to a base class, which we can tentatively call AddonManager.

(3) I'm thinking that an Addon instance should be strictly owned by its Manager, and so we should be using unique_ptr for Addons instead of shared_ptr. Addons can always share information between each other using shared_ptr, so this shouldn't really be a limitation.

Here's what I would propose for the API:

class Addon
{
public:
  class State { virtual ~State() = default; };
  // I'm not sure if there's anything else we should do with State. Maybe have a copy/clone function for this as well?

  virtual ~Addon() = default;
  virtual std::unique_ptr<Addon> clone(AddonManager* newManager) const = 0;
  virtual std::unique_ptr<State> getState() const;       // default returns a nullptr
  void setState(const std::unique_ptr<State>& newState); // does nothing by default

  // Should we have a string getType function for Addons?
}

class AddonManager
{
public:

  virtual ~AddonManager() = default;

  template <class T> has() const;
  template <class T> T* get();
  template <class T> const T* get() const;
  template <class T> void set(const std::unique_ptr<T>& addon);
  template <class T> void set(std::unique_ptr<T>&& addon);           // <- I'm not sure if this is the correct syntax, but this should be using move semantics
  template <class T, typename ...Args> T* construct(Args&&... args); // Addon constructor that uses perfect forwarding
  template <class T> erase();

  void cloneAddonsFrom(const AddonManager& otherManager);
}

I know that in your ShapeFrame implementation, you had function names like getProperties instead, but I feel like the templated nature of these functions makes get and set sufficient, because when they're used, they'll end up looking like node->get<CollisionData>() and node->set<VisualData>(vis). As long as none of the inheriting classes are crazy enough to define their own templated get/set functions, we should be okay.

I also think the Addon and AddonManager classes should go into the common namespace rather than dynamics.

That's all I can think of at the moment. If anyone has thoughts/feelings, please share.

[mxgrey]

One thing we haven't sorted out yet is how to do shortcutting for commonly used Addons (like collision and visual data) to avoid the hash table lookup. This was straightforward when there was no inheritance structure, but that's not the case anymore. I don't believe CRTP will work here, because we have a lot of abstraction happening. CRTP can only work if we (1) have only the most derived class inherit AddonManager, and (2) don't want to be able to get Addons from pointers to the base classes.

[psigen]

I think these modifications to the API make a lot of sense, but we do have to think a bit about the ramifications. I do particularly like the AddonManager moving to common and the get/set API.

• Do we need the explicit State object? If the purpose of the State is simply to have a recordable representation of the Addon, perhaps a serialize/deserialize API would be more appropriate. That might avoid having to create awkward friended State classes for every Addon.
• It seemed useful to apply Addon as a mixin to some existing class from another library that we are trying to attach to our nodes. Does it make sense to architect it as such?
• W.r.t. template shortcutting, is it possible to do something like this? http://stackoverflow.com/a/8775173 or http://stackoverflow.com/a/10601673
• I think we should have a getType or getName or getID function on Addon. It's useful for debugging/UI and we can always default it to "UnknownAddon" or "GenericAddon" so that it's not a hassle if someone doesn't need it.

[mxgrey]

Do we need the explicit State object? If the purpose of the State is simply to have a recordable representation of the Addon, perhaps a serialize/deserialize API would be more appropriate.

The main purpose of the State class is to facilitate serialization and deserialization for extensible types. All the Node states and their Addon states would get serialized into something like a BodyNode::FullState. But I haven't done serialization with inheritance structures this complex before, so there might be better ways to do it.

It seemed useful to apply Addon as a mixin to some existing class from another library that we are trying to attach to our nodes. Does it make sense to architect it as such.

I think this would be extremely useful, and I don't see any particular reason it wouldn't work.

W.r.t. template shortcutting, is it possible to do something like this?

As far as I can tell, that would only work if we crammed all the shortcuts into the implementation of the AddonManager, which means it isn't extensible for user-defined classes. Also, I think it would still prevent us from using the AddonManager as a base class.

According to this reference for insert, iterators to an unordered_map are invalidated if rehashing occurs. However, for a regular map, insert does not invalidate any iterators and erase only invalidates iterators that point to the erased entries. Other iterators are unaffected.

So here is what I propose: We use a std::map instead of a std::unordered_map. During construction, an inheriting class should fill its shortcut entries with a nullptr and then store an iterator for each of those entries. We never erase any entries from the std::map, instead we just fill the entry with a nullptr if the user asks to erase it. Then the shortcut functions can just use the iterators to the entries instead of needing to do a lookup. As long as the user doesn't have hundreds of addons in a single object, the use of std::map shouldn't be noticeably worse than std::unordered_map.

[mkoval]

So here is what I propose: We use a std::map instead of a std::unordered_map. During construction, an inheriting class should fill its shortcut entries with a nullptr and then store an iterator for each of those entries. We never erase any entries from the std::map, instead we just fill the entry with a nullptr if the user asks to erase it. Then the shortcut functions can just use the iterators to the entries instead of needing to do a lookup. As long as the user doesn't have hundreds of addons in a single object, the use of std::map shouldn't be noticeably worse than std::unordered_map.

:+1: This is very clever. The std::type_index class supports both comparison and hashing, so I don't see any harm in switching to std::map.

However, I'm still not sure how we will implement shortcuts in the derived class. It is not possible to make a template member function virtual. Therefore, we cannot specialize has, get, set, or construct in the derived class.

The main purpose of the State class is to facilitate serialization and deserialization for extensible types. All the Node states and their Addon states would get serialized into something like a BodyNode::FullState. But I haven't done serialization with inheritance structures this complex before, so there might be better ways to do it.

What API does the State class provide? I also wonder, like @psigen, if it would be better to have methods for serializing/deserializing from a binary buffer.

I think we should have a getType or getName or getID function on Addon. It's useful for debugging/UI and we can always default it to "UnknownAddon" or "GenericAddon" so that it's not a hassle if someone doesn't need it.

:+1: for this. This is invaluable for debugging; e.g. so I can dump a list of all of the addons associated attached to an object. I suggest using the same getType and getTypeStatic that we added to Joint: that API worked very well there.

[mxgrey]

It is not possible to make a template member function virtual. Therefore, we cannot specialize has, get, set, or construct in the derived class.

Right, the generic versions of the functions will still require lookups. We only get the benefit of shortcutting by calling specialized versions of the functions. I.e. instead of get<CollisionData>() you would need to call getCollisionData(). I suspect this is the best we're going to be able to do.

What API does the State class provide?

So far all I can think of is: virtual destructor and clone maybe, just to make sure we can correctly delete and copy them. Perhaps an optional getType() string.

I also wonder, like @psigen, if it would be better to have methods for serializing/deserializing from a binary buffer.

I'm open to this, but it's outside of my realm of experience. I would strongly prefer not to create another external dependency, but I would even more strongly prefer not to implement a heavy duty serializer inside of DART.

My plan was to have something like std::map< std::type_index, std::unique_ptr<Addon::State> > within an AddonManager::State class and let each Addon take care of downcasting the Addon::State to its appropriate type.

I imagine we'd use a node visitor pattern where getState() would traverse each Addon and allow them to deposit their State pointer into their spot on the map while setState(~) would traverse each Addon and allow them to withdraw their pointer. Addons that are stateless will just do nothing for both of these functions.

I suggest using the same getType and getTypeStatic that we added to Joint: that API worked very well there.

Sounds good to me :+1:

[mkoval]

Right, the generic versions of the functions will still require lookups. We only get the benefit of shortcutting by calling specialized versions of the functions. I.e. instead of get<CollisionData>() you would need to call getCollisionData(). I suspect this is the best we're going to be able to do.

I'd prefer to have the same API for both, but this seems like an okay compromise. I'll post if I come up with a brilliant solution. :smile:

So far all I can think of is: virtual destructor and clone maybe, just to make sure we can correctly delete and copy them. Perhaps an optional getType() string.

I was thinking about using State for (de)serialization, but forgot about cloning. You shouldn't have to go through a serialize-deserialize just to clone, so this makes sense to me. It seems like (de)serialization is a completely separate discussion.

[psigen]

I was thinking about using State for (de)serialization, but forgot about cloning. You shouldn't have to go through a serialize-deserialize just to clone, so this makes sense to me. It seems like (de)serialization is a completely separate discussion.

I'm not sure I follow. The Addon itself already has a clone() method, so we don't need to do anything for that. The only remaining function of the State seems to be (de)serialization. Am I missing a use case here?

[mxgrey]

You might want to make a copy of a state without passing it back into the Addon.

[psigen]

Ah ok, now I get it, thanks.

[psigen]

Quick question: Would it make any sense to not inherit AddonManager directly, but instead make it an accessible member of the Node class? E.g. node.addons.get<CollisionAddon>()?

I think this sidesteps the inheritance problem, and since there are relatively few actual functions on the AddonManager, we could potentially create wrapper get/set calls on Node to its member AddonManager, PIMPL-style.

[mxgrey]

What would be the problem with inheritance?

[psigen]

get<CollisionData>()/getCollisionData()?

[mxgrey]

Why would that be a problem?

My main concern with making it the AddonManager a member is that in order to do the specialized shortcutting, we need access to the map of Addons, which I think ought to be protected in the AddonManager. If the AddonManager is a member variable, there wouldn't be a way to make the map protected while still allowing the classes holding it to do the shortcutting. I suppose we could have a getAddonMap function that just returns a non-const reference to the map, but I think I'd prefer hiding the map from the API altogether. Inheriting the AddonManager lets us do shortcutting while keeping the map hidden from the outside.

[psigen]

@mkoval and I were discussing this a bit further and came up with the following:

Instead of trying to implement a virtual template specialization, we could make the get<>/set<> methods on AddonManager non-virtual. Within AddonManager, this would simply direct accesses through the map.

For performance-critical accesses, such as accessing the CollisionAddon on a ShapeNode, we would write a new non-virtual get<>/set<> implementation that template specializes between direct access (e.g. get<CollisionAddon>) and map accesses (e.g. get<FoobarAddon>).

This has two minor caveats, which might not be a big deal in practice:

1. Non-virtual implementations mean that access while casted to a parent class will not use the performant specialization. If you access a CollisionAddon on a ShapeNode, it will be direct, but if you cast that ShapeNode to a Node and attempt the access, it will resolve through the map.

   Since this access is primarily important for very specific addons, it seems reasonable that at the time, the same semantics that lets one know that the Addon will exist on the Node would probably also convey a sufficiently derived class to have the fast access.

2. Child classes that need to add additional specialization will need to reimplement parent specializations if they need them to remain performant. Suppose that we have a child class of ShapeNode called SoftShapeNode. If SoftShapeNode wants performant access to a new Addon called SoftParamsAddon, then it will also need to re-specialize the CollisionAddon.

   This re-specialization should be fairly trivial, however, since this implementation can simply call the ShapeNode specialization. If no additional specialization is required, then child class can simply inherit the non-virtual implementation of the parent class wholesale.

[mxgrey]

Template specialization sounds like the perfect solution to me :+1: :+1:

Non-virtual implementations mean that access while casted to a parent class will not use the performant specialization.

If someone is confident in the type of Node that they have and they want to maximize performance, they can always static_cast, or if they're not confident in the type and want to try to improve performance, they can dynamic_cast.

Child classes that need to add additional specialization will need to reimplement parent specializations

I wonder if the new C++11 way of using the using keyword on member functions would work here. If not, it would be pretty easy to write a macro to take care of this anyway.

I think I would still advocate having both a specialized version of get<CollisionData>() and an explicit getCollisionData() in the API to make it absolutely clear which types of Addons have specialized status. Then we can just have get<CollisionData>() simply call getCollisionData() or vice-versa.

[psigen]

I think I would still advocate having both a specialized version of get() and an explicit getCollisionData()

Just clarifying, no issues here, sounds fine. :smile:

As for naming:

• Addon
• Module
• Property
• Extra
• Component
• Feature

@mkoval, @mxgrey: Any of those sound good?

[mxgrey]

I think my favorites from that list would be

• Addon
• Extra
• Feature

I don't want to use Property, because just about every class already has its own Properties struct which has a very particular meaning. I'm using the word Module a lot to refer to the configurable IK stuff, so I'd rather not use that. Component sounds too generic to me, and doesn't express the idea that it's optional.

[mxgrey]

I plan on beginning the implementation of all of this tonight or tomorrow. If no one has any strong feelings to the contrary, I think I'll go with the term Addon, although I'm not totally sure if I should spell it like Addon or AddOn. Any preferences between those two?

[psigen]

My preference is fewer capital letters, since the derived classes will add more camel case words anyway.

:+1: for Addon. On Jul 29, 2015 6:49 PM, "Michael X. Grey" notifications@github.com wrote:

I plan on beginning the implementation of all of this tonight or tomorrow. If no one has any strong feelings to the contrary, I think I'll go with the term Addon, although I'm not totally sure if I should spell it like Addon or AddOn. Any preferences between those two?

— Reply to this email directly or view it on GitHub https://github.com/dartsim/dart/issues/394#issuecomment-126119365.

[mkoval]

:+1: for Addon.

[mxgrey]

Then Addon it will be.

[mxgrey]

I thought I'd quickly outline my current development plans.

The ShapeFrame / ShapeNode concept is going to clobber the API for visualization and collision shapes. It's very very hard for me to imagine a clean way of keeping the API backwards compatible. As two examples, Shape::setLocalTransform will be thrown out in favor of ShapeFrame::setRelativeTransform, and Entity will not be able to accept visualization shapes anymore.

I'd like to get most of the Addon and State features available in version 5.x, so my plan is to implement the mechanics of Addon and State without implementing ShapeFrame and ShapeNode yet. Then once the pull request for Addon and State is finished and merged (probably into v5.2), I'll take a blunt instrument to the API and implement ShapeFrame and ShapeNode which will kick off the next major version.

I know this is sort of ironic and backwards from our original intentions, but I think it makes the most sense from a development timeline perspective.

[psigen]

I think this is probably a reasonable course. Addon and State are core features that are almost certainly going to shape the API we want to use going forward.

I'd rather get them in completely first so we can start taking advantage of them architecturally wherever possible now in 5.x without worrying about the breaking API changes.

[mxgrey]

Anyone have opinions on where in the inheritance hierarchy the AddonManager should be placed? Entity? Frame? Node? Or at the further derived types, like BodyNode / ShapeFrame / EndEffector?