Aspects, States, and Properties

[mxgrey]

This pull request addresses #643, #627, and some conversations that were had offline. It's a doozy of a pull request, but I think it provides a lot of valuable improvements and features with minimal impact on the API (besides the renaming of Addon to Aspect).

Due to the renaming, the diff that's available on Github is going to be virtually unusable. That being the case, I'm going to provide individual links that highlight important parts of this pull request.

Air-tight (albeit complex) inheritance structures

First I'd like to start with a simplified diff which illustrates the primary value of this pull request

The key thing to note is that we eliminate a huge amount of code duplication between these two classes. EndEffector and ShapeNode no longer store their own relative transform property because now FixedFrame stores this information as part of its FixedFrame::Aspect. Moreover, EndEffector and ShapeNode don't need to define Node::State or Node::Properties for themselves, because all of their states and properties -- from the beginning to the end of their inheritance structure -- are stored within Aspects. This allows them to each inherit a class called CompositeNode which will wrap their Composite::State and Composite::Properties up into a Node::State and Node::Properties.

Essentially, every class that has information which could be labeled as "State" or "Properties" will store that information with one or more Aspects. An Aspect can embed its information within the object that owns it (completely eliminating any overhead for the object to access it), or the Aspect instance may hold onto the information itself (allowing for more flexible memory usage). This decision is up to the developer/user to determine which approach is best suited for any given Aspect.

The bulk of the implementation for these features can be found in dart/common/EmbeddedAspect.h and dart/common/detail/EmbeddedAspect.h.

Direct access to members of Composite State/Properties

Up until now, when you have a Composite::State (formerly known as AddonManager::State) it was completely opaque. You had no way of accessing or modifying its contents; you could only pull it out of one Composite and pass it to another Composite. Now, however, you can do the following:

Composite::Properties properties;
properties.getOrCreate<BodyNode>().mName = "Body";
if(SoftBodyNode::AspectProperties* softProperties = properties.get<SoftBodyNode>())
  softProperties->mDampCoeff = 0.3;

This allows you to dynamically construct and access individual Aspects of the Composite State. Identical features are available for Composite Properties. get<T>() will return a pointer which is nullptr if the Aspect didn't exist, whereas getOrCreate<T>() will return a reference, since the information will be created if it didn't exist already.

The implementation for these features can be found in dart/common/detail/CompositeData.h.

Seamless integration of both internal Aspects and user-added Aspects

Because of the new system, any Aspects (formerly known as Addons) that user adds to a Skeleton, BodyNode, Joint, or Node will get sucked into the standard State and Properties of the BodyNode/Joint/Node that it's added to, as well as the Skeleton in which that object exists. This is in contrast to the prior approach where the was an ExtendedProperties class that mangled together the basic Properties and the Composite::Properties.

The new system is completely unified---functionally, semantically, and mechanically---instead of having different information handled in different ways and then getting mangled together at the last second so that it can be delivered to the user as a single object.

Template Madness

I think the biggest catch to these changes is the ever growing usage of templates. As much as I may adore templates myself, I do recognize that not everyone is very comfortable with them, and they can result in very complex (read: confusing) inheritance structures when CRTP is involved. This template-based implementation is high performance and very flexible, but it will probably be confusing for reviewers and future developers. With that in mind, I encourage people to post any questions they run into while reviewing the implementation. Even if you can manager to answer the question yourself, it would still be good for me to know the question so that I can address it specifically when it comes time for me to get serious about documenting this implementation.

API Changes

I tried my hardest to keep the API changes minimal (with the exception of renaming Addon to Aspect and AddonManager to Composite). The biggest difference you'll see is in the SomeClass::Properties constructors. Only the constructors. The actual member variables of those classes should remain virtually unchanged, but their constructors won't quite work the way they used to, especially BodyNode::Properties. An easy fix for this is to allow it to perform a default construction and then fill in its member variables after construction.

---

This change is

[mxgrey]

Review status: 94 of 185 files reviewed at latest revision, 10 unresolved discussions.

---

dart/common/Composite.h, line 199 [r2] (raw file):
Done.

---

dart/common/detail/EmbeddedAspect.h, line 79 [r1] (raw file):
Done.

---

dart/common/detail/EmbeddedAspect.h, line 152 [r1] (raw file):
Done.

---

dart/common/detail/EmbeddedAspect.h, line 240 [r1] (raw file):
Done.

---

dart/common/detail/SpecializedForAspect.h, line 57 [r1] (raw file):
Done.

---

dart/dynamics/Skeleton.h, line 72 [r1] (raw file):
Done.

---

dart/common/StlHelpers.h, line 44 [r1] (raw file):
Done.

---

Comments from Reviewable

[jslee02]

Review status: 94 of 185 files reviewed at latest revision, 3 unresolved discussions.

---

dart/common/detail/EmbeddedAspect.h, line 152 [r1] (raw file):
I expected to use the function here, but it has almost no difference. 👍

---

Comments from Reviewable

[jslee02]

Reviewed 87 of 211 files at r1, 1 of 1 files at r2, 7 of 11 files at r3, 1 of 1 files at r4.
Review status: 173 of 185 files reviewed at latest revision, 19 unresolved discussions.

---

dart/dynamics/FixedFrame.h, line 59 [r4] (raw file):
Could we move this under detail directory?

---

dart/dynamics/ScrewJoint.cpp, line 171 [r4] (raw file):
Why don't we check the validity here?

---

dart/dynamics/ShapeFrame.h, line 106 [r4] (raw file):
It seems the places of the definitions of properties structs are not consistent. What is the convention of them in terms of visibility? Should they​ be in detail namespace and under detail directory?

---

dart/dynamics/ShapeNode.h, line 59 [r4] (raw file):
The same. I'm a bit inclined​ to moving all the definitions in detail namespace should be under detail directory.

---

dart/dynamics/SimpleFrame.h, line 163 [r4] (raw file):
virtual is not necessary with override. I'm thinking to remove all the virtual keywords when override is used together​ as a wholesale, though.

---

dart/dynamics/SoftBodyNode.h, line 310 [r4] (raw file):
Can be removed

---

dart/dynamics/SoftMeshShape.cpp, line 94 [r4] (raw file):
common::make_unique<aiMesh>())?

---

dart/dynamics/detail/BodyNodeAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/BodyNodeAspect.h, line 59 [r4] (raw file):
Could add newline

---

dart/dynamics/detail/EulerJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/JointAspect.h, line 37 [r4] (raw file):
Please rename to DART_DYNAMICS_DETAIL_JOINTASPECT_H_.

---

dart/dynamics/detail/MultiDofJointAspect.h, line 37 [r4] (raw file):
Please rename to DART_DYNAMICS_DETAIL_MULTIDOFJOINTASPECT_H_

---

dart/dynamics/detail/PlanarJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/PrismaticJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/RevoluteJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/ScrewJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/SingleDofJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/SoftBodyNodeAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

dart/dynamics/detail/UniversalJointAspect.h, line 37 [r4] (raw file):
Please rename PROPERTIES to ASPECT.

---

Comments from Reviewable

[jslee02]

Reviewed 10 of 211 files at r1, 2 of 11 files at r3.
Review status: all files reviewed at latest revision, 20 unresolved discussions.

---

dart/dynamics/FixedJacobianNode.cpp, line 215 [r4] (raw file):
Please remove one line.

---

Comments from Reviewable

[mxgrey]

dart/dynamics/ScrewJoint.cpp, line 171 [r4] (raw file):
It seems that the line was lost during a merge conflict resolution. I'll add it back in.

---

Comments from Reviewable

[mxgrey]

dart/dynamics/ShapeFrame.h, line 106 [r4] (raw file):
For the sake of consistency, that might be preferable. We can move these Properties structs to a file named detail/ShapeNodeAspect.h.

---

Comments from Reviewable

[jslee02]

Reviewed 7 of 211 files at r1, 23 of 23 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

dart/dynamics/ShapeNode.h, line 59 [r4] (raw file):
Has not addressed yet.

---

dart/dynamics/detail/JointAspect.h, line 37 [r4] (raw file):
Has not addressed yet.

---

Comments from Reviewable

[jslee02]

Reviewed 4 of 5 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.

---

dart/dynamics/ShapeNode.h, line 59 [r4] (raw file):
Done.

---

dart/dynamics/detail/JointAspect.h, line 37 [r4] (raw file):
Done.

---

Comments from Reviewable

[jslee02]

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

---

I'm happy with the improved API especially in terms of that it eliminated many boilerplate code that was used for managing properties and state, and became more flexible to construct and access to. The code reviewing was like taking very hard C++ exam of something like a Meta Programming class though. 😿

I believe this PR is ready to merge unless anyone has an objection. 👍

[mxgrey]

I snuck in one last commit that removed some hardcoded file paths that I was using 😅

[jslee02]

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jslee02]

@mkoval Any follow-up comment?

Edit: I confirmed that @mkoval is happy with this PR offline. Merging.