[Preliminary] Nodes and Addons

[mxgrey]

This is a preliminary pull request for facilitating review and discussion

Current Status: Everything is implemented except the SoftBodyNode Addon. More unit tests should be created to ensure that everything works as expected.

This pull request brings a tremendous amount of additional functionality to the Node class and introduces the Addon class.

The Node class exists primarily to allow users to embed custom classes into the kinematic/dynamic structure of a Skeleton. Nodes are objects that attach to BodyNodes, and they are allowed to contain a State structure and a Properties structure. Nodes are also cloneable. The State of a Node will be saved when the State of its Skeleton is saved, and same for the Node's Properties. There is no limit to how many instances of a single Node type you can attach to a single BodyNode.

The Addon class allows the user to embed extra data into any class that inherits common::AddonManager. An Addon can contain State and/or Properties, and Addons are cloneable. Unlike Nodes, you can only add a single instance of a particular Addon type to an AddonManager.

In order to deal with the huge amount of boilerplate code that is needed to produce Nodes and Addons, I took some creative liberties with CRTP and macro patterns. I think my CRTP implementations should be fairly good (even if they don't look very pretty), but my use of macros could probably use some heavy critiquing.

I try not to use macros too much since I consider them a rather gross legacy of C, but they were the only way I could think of to keep the code reasonably DRY. I'm pretty sure we can't avoid using macros if we want DRYness, but I would be very open to any suggestions as far as improving the naming conventions or style that the macros are written with.

Note: This pull request already includes the changes of #530

[jslee02]

If this function is for the legality check of removing from the manager, I think it should to be determined by its manager rather than by itself. If the decision is tied to the addon, we cannot create addons of the addon type from two managers, where one is general manager (or specialized manager but not for the addon) and the other one is a specialized manager for the addon, expecting one is specialized and the other is not specialized.

[mxgrey]

That was my original plan, but I had a very hard time thinking of a clean way to implement that. I found it much easier to have that information embedded in the Addon rather than trying to get it into the SpecializedAddonManager.

It should be noted that this function provides the Addon with a pointer to its manager, so the Addon can determine whether it's required for this particular type of manager. So if we have a FooAddon that's "required" only for a FooManager but we attach it to a GeneralManager, then the function FooAddon::isOptional(AddonManager* mgr) can dynamic_cast the mgr argument to a FooManager*. If the dynamic_cast fails, then FooAddon::isOptional will return true, if the cast succeeds then it will return false.

[jslee02]

Actually, I'm not sure if we really need this function. (If there wouldn't be any additional use case,) It's currently used in AddonManager and SpecializedManager to check the legality of removing from the managers. In AddonManager, we might don't need it since all the addons in AddonManager are not specialized. For SpecializedAddonManager, we can check the legality in more simple way. For example, erase() function can be implemented as:

template <class SpecAddon>
template <class T>
void SpecializedAddonManager<SpecAddon>::erase()
{
  if (typeid( T ).name() == mSpecAddonIterator->first.name())
  {
    std::cout << "Attempting to erase specialized addon, which is not allowed.\n";
    return;
  }

  AddonManager::erase<T>();
//  _erase(type<T>());
}

[mxgrey]

The thing is, not all specialized addons are required. For example, for ShapeNode, we will have a VisualData and a CollisionData specialized addon, but neither of them are required to be there, because their absence just means that the ShapeNode doesn't want to visualize or doesn't want to collide. On the other hand, RevoluteJoint has a RevoluteJointAddon which contains axis properties, and that addon is required because the dynamics and kinematics computations can't work without it.

A specialized addon just allows us to have constant-time access to it, and does not imply that the addon is required.

[jslee02]

It makes sense, but I wonder if there is a use case that the optionality should be checked from addon instead of from the specific manager we want to check. We can check it from FooManager and GeneralManager for FooAddon for the above case.

Even though there are use cases, isOptional() doesn't seem to work as we expect. For example, AddonWithProtectedPropertiesInSkeleton::isOptional() would always return true regardless the passed in manager type if true is passed in for the template parameter OptionalT.

[mxgrey]

As far as whether I expect the behavior of (1) or (2) it will depend on whether we have MandatoryAddon also specialize the addon. I think we should, because why not? If an addon is going to be mandatory, we may as well specialize it as well, because clearly we expect it to exist.

[jslee02]

It sounds like the map can have any addons regardless of each addon is optional or mandatory. Hm, I'm more confused, haha. Here is my understanding. Please let me know which one is different from you.

• Unspecialized addons are always optional.
• Specialized addons can be one of optional and mandatory.

In that sense, it seems the mandatory addons should be a subset of specialized addons. So I stored the optionality value within specialized addon map (that is stored in the basic addon manager) in the code, and worried the attemptions to set the optionality when the associated addon hasn't added yet.

[mxgrey]

I don't really agree with the subset relationship. Here's how I think it should work:

• A manager that is specialized for an addon type will offer zero-overhead access to that addon
• If an addon is mandatory for a certain manager type, then there must be no possible way to remove the addon from that manager

These are two completely independent concepts. The only point at which they overlap is that if an addon is mandatory for a certain manager type, then we may as well also specialize the manager for that addon.

[jslee02]

I see your point. What I'm not clear is why we open the case that an addon is mandatory but not specialized. Wouldn't it be too strong restriction if we enforce the user to use specialized manager for every mandatory addons so that the user can always take the advantage of the zero-overhaed access to that addon?

[mxgrey]

Right, I agree. I think the concepts of specialized and mandatory are inherently independent, but if an addon is important enough to be mandatory, then we may as well have the API also specialize it, especially since it would be as easy as having MandatoryAddon virtually inherit SpecializedAddonManager.

[jslee02]

If the current master branch is merged into this, the AppVeyor can successfully build this PR thanks to #595. I created DART dependency binaries for Visual Studio 2015.

[jslee02]

Is there a special reason why this class is not inherited from AddonWithProtectedState?

[jslee02]

It seems this pull request is ready to merge. There are several items discussed in this pull request that might need some changes in code, but we save them for later pull requests to make it easier to review and evaluate.

DART now have very useful features of flexible data structures, which is another big improvement you made. Nice job @mxgrey ! Merging now.

[mkoval]

👍 Glad this got merged!

I have one follow-up question from the discussion: What is the purpose of a required Addon when we could simply put those attributes in the AddonManager?

Using @mxgrey's example:

On the other hand, RevoluteJoint has a RevoluteJointAddon which contains axis properties, and that addon is required because the dynamics and kinematics computations can't work without it.

Why not just put these attributes in the RevoluteJoint class? Is the idea to use AddonManager to generate the Property and State boilerplate?

[mxgrey]

Is the idea to use AddonManager to generate the Property and State boilerplate?

This is exactly right. And not just generate the boilerplate: By storing joint-type-dependent property data in an Addon, it will naturally get sucked into the Skeleton::ExtendedProperties. This way, we don't need to do any special handling on a per-joint-type basis in order to take a comprehensive snapshot of all the joint property data in a Skeleton. This is especially important for handling the properties and states of user-created custom joint types.

[mkoval]

👍 Very nice!