critiques.md

Critiques of MatrixNode

• Feels a little too heavy for a "node" class. My generic in-head idea is to provide a lightweight abstraction over existing stuff in order to compliment it
• Doesn't seem to allow for scaling ranges which some animations use, though may be impractical to implement anyway

NodeTransforms:

• Feels hard to read - why do we need links etc?
• NodeTransforms::NodeTransforms constructors - whats itterative do?
• Also constructors copy vectors, pretty bad
• mMatrixList is list of MatrixNode pointers - bad for cache locality
• pop_Link moves entire vector for every erase; this is called from 0...length when destroying matrix, so you'll end up with too many memcpys in a large collection
• const MatrixF* GetLinkGlobal(S32 id) uses a signed integer for index, so underflow is possible

SUGGESTION

Maybe design it more as a flat utility class, e.g.

template<class NodeType> MatrixSet
{
	Vector<NodeType> *mNodes; // node lists which contain parent -> child stuff etc
	MatrixF* mTransforms; // local transforms list; perhaps could even be abstract to support stuff like dual quaterions and such
	// + baked list? unless we just do that once somewhere else

	MatrixSet(NodeType* inNodes, MatrixF* inTransforms) :
	mNodes(inNodes), mTransforms(inTransforms)
	{

	}

	// Some util funcs (though prob dont include too much from MatrixF)

	QuatF getLocalRotation(U32 nodeIdx)
	{
		...
	}

	Point3F getForwardVector(U32 nodeIdx)
	{
		...
	}

	// world space / local space stuff: could use cache or 
	// do recursive parent lookup
	// (depends how dynamic it should be and what one expects the 
	//  user to do)

	Point3F toWorldSpace(U32 nodeIdx, Point3F nodeLocalPos)
	{
		...
	}

	Point3F toLocalSpace(U32 nodeIdx, Point3F worldPos)
	{
		...
	}

	// need to constrain something maybe?

	void constrainNodeAnglesBasic(U32 nodeIdx, EulerF angles)
	{
		...
	}

	void enumerateChildNodes(S32 nodeIdx, T& func)
	{
		for(NodeType& n : mNodes)
		{
			if (n.parentIdx == nodeIdx)
			{
				func(n, &n - mNodes.begin());
			}
		}
	}

	void sortChildNodes()
	{
		...
	}

	void enumerateChildNodesRecursive(S32 rootNodeIdx, T& func)
	{
		Vector<U32> recurseList;

		enumerateChildNodes(rootNodeIdx, [&recurseList, func](NodeType& n, U32 nodeIdx){
			func(n, nodeIdx);
			recurseList.push_back(nodeIdx);
		});

		// Recurse
		for (U32 recurseIdx : recurseList)
		{
			enumerateChildNodesRecursive(recurseIdx, func);
		}
	}
};

Then maybe add in other utility stuff on top like:

// Bake current transforms!
template<class NodeType> void BakeTransformList(MatrixSet<NodeType> &list, MatrixF &worldXfm, Vector<MatrixF> &outTransforms)
{
	outTransforms.setSize(list.mNodes->size());

	// Bit contrived as transform list should be sorted in terms of parentIdx
	// (so can just do a straight up iterate instead)
	list.enumerateChildNodesRecursive(-1, [&worldXfm](NodeType& node, U32 nodeIdx){
		if (node.parentIdx == -1)
		{
			outTransforms[nodeIdx] = worldXfm;
		}
		else
		{
			outTransforms[nodeIdx] = outTransforms[node.parentIdx].mul(list.mTransforms[nodeIdx]);
		}

		// Perhaps add on extra stuff here if we want to also do IK solving
		// OR just leave it to another util function
	});
}

Other plus points:

• Can be separated out from core shape code
• Can be easily tested independently

Useful references from other engines for ideas on USEFUL utility functions

• Unity Transform class ( https://docs.unity3d.com/ScriptReference/Transform.html )
• Unreal Transform class ( https://docs.unrealengine.com/4.27/en-US/API/Runtime/Core/Math/FTransform/ )