Bugfix [#24163] Unable to animate INSIDE a group node in the

compositor

This commit fixes the original bug reported here by adding some
methods to move the relevant F-Curves (and drivers) over to the new
Node-Tree's (i.e. group-node) AnimData. Animated nodes which
subsequently get grouped will still be able to animate as a result of
this commit.

TODO's:
- Ungrouping now will not yet merge the animation back (or at least
copy it)
- Buttons for nodes freshly grouped do not correctly show animated
status indicators for some reason, yet normal animation does

6 files changed, 231 insertions, 4 deletions

diff --git a/source/blender/blenkernel/BKE_action.h b/source/blender/blenkernel/BKE_action.h
index 4d3f000c863..1c75387ba5d 100644
--- a/source/blender/blenkernel/BKE_action.h
+++ b/source/blender/blenkernel/BKE_action.h
@@ -117,6 +117,8 @@ void action_groups_remove_channel(struct bAction *act, struct FCurve *fcu);
 /* Find a group with the given name */
 struct bActionGroup *action_groups_find_named(struct bAction *act, const char name[]);
 
+/* Clear all 'temp' flags on all groups */
+void action_groups_clear_tempflags(struct bAction *act);
 
 /* Pose API ----------------- */	
 	
diff --git a/source/blender/blenkernel/BKE_animsys.h b/source/blender/blenkernel/BKE_animsys.h
index 3952b6066d3..500210d2fcd 100644
--- a/source/blender/blenkernel/BKE_animsys.h
+++ b/source/blender/blenkernel/BKE_animsys.h
@@ -100,6 +100,14 @@ void BKE_animdata_fix_paths_rename(struct ID *owner_id, struct AnimData *adt, co
 /* Fix all the paths for the entire database... */
 void BKE_all_animdata_fix_paths_rename(char *prefix, char *oldName, char *newName);
 
+/* -------------------------------------- */
+
+/* Move animation data from src to destination if it's paths are based on basepaths */
+void BKE_animdata_separate_by_basepath(struct ID *srcID, struct ID *dstID, struct ListBase *basepaths);
+
+/* Move F-Curves from src to destination if it's path is based on basepath */
+void action_move_fcurves_by_basepath(struct bAction *srcAct, struct bAction *dstAct, const char basepath[]);
+
 /* ************************************* */
 /* Batch AnimData API */
 
diff --git a/source/blender/blenkernel/intern/action.c b/source/blender/blenkernel/intern/action.c
index 227f2eadf4c..fdd51dd207f 100644
--- a/source/blender/blenkernel/intern/action.c
+++ b/source/blender/blenkernel/intern/action.c
@@ -96,7 +96,6 @@ void make_local_action(bAction *act)
 	if (act->id.us==1) {
 		act->id.lib= 0;
 		act->id.flag= LIB_LOCAL;
-		//make_local_action_channels(act);
 		new_id(0, (ID *)act, 0);
 		return;
 	}
@@ -376,6 +375,20 @@ bActionGroup *action_groups_find_named (bAction *act, const char name[])
 	return BLI_findstring(&act->groups, name, offsetof(bActionGroup, name));
 }
 
+/* Clear all 'temp' flags on all groups */
+void action_groups_clear_tempflags (bAction *act)
+{
+	bActionGroup *agrp;
+	
+	/* sanity checks */
+	if (ELEM(NULL, act, act->groups.first))
+		return;
+		
+	/* flag clearing loop */
+	for (agrp = act->groups.first; agrp; agrp = agrp->next)
+		agrp->flag &= ~AGRP_TEMP;
+}
+
 /* *************** Pose channels *************** */
 
 /* usually used within a loop, so we got a N^2 slowdown */
diff --git a/source/blender/blenkernel/intern/anim_sys.c b/source/blender/blenkernel/intern/anim_sys.c
index 41c6969cdce..e8a9851b0d9 100644
--- a/source/blender/blenkernel/intern/anim_sys.c
+++ b/source/blender/blenkernel/intern/anim_sys.c
@@ -276,6 +276,176 @@ void BKE_animdata_make_local(AnimData *adt)
 		make_local_strips(&nlt->strips);
 }
 
+/* Sub-ID Regrouping ------------------------------------------- */
+
+/* helper heuristic for determining if a path is compatible with the basepath 
+ * < path: (str) full RNA-path from some data (usually an F-Curve) to compare
+ * < basepath: (str) shorter path fragment to look for
+ * > returns (bool) whether there is a match
+ */
+static short animpath_matches_basepath (const char path[], const char basepath[])
+{
+	/* we need start of path to be basepath */
+	return (path && basepath) && (strstr(path, basepath) == path);
+}
+
+/* Move F-Curves in src action to dst action, setting up all the necessary groups 
+ * for this to happen, but only if the F-Curves being moved have the appropriate 
+ * "base path". 
+ *	- This is used when data moves from one datablock to another, causing the
+ *	  F-Curves to need to be moved over too
+ */
+void action_move_fcurves_by_basepath (bAction *srcAct, bAction *dstAct, const char basepath[])
+{
+	FCurve *fcu, *fcn=NULL;
+	
+	/* sanity checks */
+	if ELEM3(NULL, srcAct, dstAct, basepath) {
+		if (G.f & G_DEBUG) {
+			printf("ERROR: action_partition_fcurves_by_basepath(%p, %p, %p) has insufficient info to work with\n",
+					srcAct, dstAct, basepath);
+		}
+		return;
+	}
+		
+	/* clear 'temp' flags on all groups in src, as we'll be needing them later 
+	 * to identify groups that we've managed to empty out here
+	 */
+	action_groups_clear_tempflags(srcAct);
+	
+	/* iterate over all src F-Curves, moving over the ones that need to be moved */
+	for (fcu = srcAct->curves.first; fcu; fcu = fcn) {
+		/* store next pointer in case we move stuff */
+		fcn = fcu->next;
+		
+		/* should F-Curve be moved over?
+		 *	- we only need the start of the path to match basepath
+		 */
+		if (animpath_matches_basepath(fcu->rna_path, basepath)) {			
+			bActionGroup *agrp = NULL;
+			
+			/* if grouped... */
+			if (fcu->grp) {
+				/* make sure there will be a matching group on the other side for the migrants */
+				agrp = action_groups_find_named(dstAct, fcu->grp->name);
+				
+				if (agrp == NULL) {
+					/* add a new one with a similar name (usually will be the same though) */
+					agrp = action_groups_add_new(dstAct, fcu->grp->name);
+				}
+				
+				/* old groups should be tagged with 'temp' flags so they can be removed later
+				 * if we remove everything from them
+				 */
+				fcu->grp->flag |= AGRP_TEMP;
+			}
+			
+			/* perform the migration now */
+			action_groups_remove_channel(srcAct, fcu);
+			
+			if (agrp)
+				action_groups_add_channel(dstAct, agrp, fcu);
+			else
+				BLI_addtail(&dstAct->curves, fcu);
+		}
+	}
+	
+	/* cleanup groups (if present) */
+	if (srcAct->groups.first) {
+		bActionGroup *agrp, *grp=NULL;
+		
+		for (agrp = srcAct->groups.first; agrp; agrp = grp) {
+			grp = agrp->next;
+			
+			/* only tagged groups need to be considered - clearing these tags or removing them */
+			if (agrp->flag & AGRP_TEMP) {
+				/* if group is empty and tagged, then we can remove as this operation
+				 * moved out all the channels that were formerly here
+				 */
+				if (agrp->channels.first == NULL)
+					BLI_freelinkN(&srcAct->groups, agrp);
+				else
+					agrp->flag &= ~AGRP_TEMP;
+			}
+		}
+	}
+}
+
+/* Transfer the animation data from srcID to dstID where the srcID
+ * animation data is based off "basepath", creating new AnimData and
+ * associated data as necessary
+ */
+void BKE_animdata_separate_by_basepath (ID *srcID, ID *dstID, ListBase *basepaths)
+{
+	AnimData *srcAdt=NULL, *dstAdt=NULL;
+	LinkData *ld;
+	
+	/* sanity checks */
+	if ELEM(NULL, srcID, dstID) {
+		if (G.f & G_DEBUG)
+			printf("ERROR: no source or destination ID to separate AnimData with\n");
+		return;
+	}
+	
+	/* get animdata from src, and create for destination (if needed) */
+	srcAdt = BKE_animdata_from_id(srcID);
+	dstAdt = BKE_id_add_animdata(dstID);
+	
+	if ELEM(NULL, srcAdt, dstAdt) {
+		if (G.f & G_DEBUG)
+			printf("ERROR: no AnimData for this pair of ID's\n");
+		return;
+	}
+	
+	/* active action */
+	if (srcAdt->action) {
+		/* set up an action if necessary, and name it in a similar way so that it can be easily found again */
+		if (dstAdt->action == NULL) {
+			dstAdt->action = add_empty_action(srcAdt->action->id.name+2);
+		}
+		else if (dstAdt->action == srcAdt->action) {
+			printf("Argh! Source and Destination share animation! ('%s' and '%s' both use '%s') Making new empty action\n",
+				srcID, dstID, srcAdt->action);
+			
+			// TODO: review this...
+			id_us_min(dstAdt->action);
+			dstAdt->action = add_empty_action(dstAdt->action->id.name+2);
+		}
+			
+		/* loop over base paths, trying to fix for each one... */
+		for (ld = basepaths->first; ld; ld = ld->next) {
+			const char *basepath = (const char *)ld->data;
+			action_move_fcurves_by_basepath(srcAdt->action, dstAdt->action, basepath);
+		}
+	}
+	
+	/* drivers */
+	if (srcAdt->drivers.first) {
+		FCurve *fcu, *fcn=NULL;
+		
+		/* check each driver against all the base paths to see if any should go */
+		for (fcu = srcAdt->drivers.first; fcu; fcu = fcn) {
+			fcn = fcu->next;
+			
+			/* try each basepath in turn, but stop on the first one which works */
+			for (ld = basepaths->first; ld; ld = ld->next) {
+				const char *basepath = (const char *)ld->data;
+				
+				if (animpath_matches_basepath(fcu->rna_path, basepath)) {
+					/* just need to change lists */
+					BLI_remlink(&srcAdt->drivers, fcu);
+					BLI_addtail(&dstAdt->drivers, fcu);
+					
+					// TODO: add depsgraph flushing calls?
+					
+					/* can stop now, as moved already */
+					break;
+				}
+			}
+		}
+	}
+}
+
 /* Path Validation -------------------------------------------- */
 
 /* Check if a given RNA Path is valid, by tracing it from the given ID, and seeing if we can resolve it */
diff --git a/source/blender/blenkernel/intern/node.c b/source/blender/blenkernel/intern/node.c
index 0513593a0e0..f85def22566 100644
--- a/source/blender/blenkernel/intern/node.c
+++ b/source/blender/blenkernel/intern/node.c
@@ -36,11 +36,13 @@
 #include <string.h>
 
 #include "DNA_anim_types.h"
+#include "DNA_action_types.h"
 
 #include "RNA_access.h"
 
+#include "BKE_animsys.h"
+#include "BKE_action.h"
 #include "BKE_fcurve.h"
-#include "BKE_animsys.h" /* BKE_free_animdata only */
 
 
 #include "PIL_time.h"
@@ -460,6 +462,7 @@ bNode *nodeMakeGroupFromSelected(bNodeTree *ntree)
 	bNode *node, *gnode, *nextn;
 	bNodeSocket *sock;
 	bNodeTree *ngroup;
+	ListBase anim_basepaths = {NULL, NULL};
 	float min[2], max[2];
 	int totnode=0;
 	
@@ -502,11 +505,27 @@ bNode *nodeMakeGroupFromSelected(bNodeTree *ntree)
 	for(node= ntree->nodes.first; node; node= nextn) {
 		nextn= node->next;
 		if(node->flag & NODE_SELECT) {
+			/* keep track of this node's RNA "base" path (the part of the pat identifying the node) 
+			 * if the old nodetree has animation data which potentially covers this node
+			 */
+			if (ntree->adt) {
+				PointerRNA ptr;
+				char *path;
+				
+				RNA_pointer_create(&ntree->id, &RNA_Node, node, &ptr);
+				path = RNA_path_from_ID_to_struct(&ptr);
+				
+				if (path)
+					BLI_addtail(&anim_basepaths, BLI_genericNodeN(path));
+			}
+			
+			/* change node-collection membership */
 			BLI_remlink(&ntree->nodes, node);
 			BLI_addtail(&ngroup->nodes, node);
+			
 			node->locx-= 0.5f*(min[0]+max[0]);
 			node->locy-= 0.5f*(min[1]+max[1]);
-
+			
 			/* set socket own_index to zero since it can still have a value
 			 * from being in a group before, otherwise it doesn't get a unique
 			 * index in group_verify_own_indices */
@@ -526,6 +545,21 @@ bNode *nodeMakeGroupFromSelected(bNodeTree *ntree)
 		}
 	}
 	
+	/* move animation data over */
+	if (ntree->adt) {
+		LinkData *ld, *ldn=NULL;
+		
+		BKE_animdata_separate_by_basepath(&ntree->id, &ngroup->id, &anim_basepaths);
+		
+		/* paths + their wrappers need to be freed */
+		for (ld = anim_basepaths.first; ld; ld = ld->next) {
+			ldn = ld->next;
+			
+			MEM_freeN(ld->data);
+			BLI_freelinkN(&anim_basepaths, ld);
+		}
+	}
+	
 	/* now we can make own group typeinfo */
 	ntreeMakeOwnType(ngroup);
 	
diff --git a/source/blender/makesrna/intern/rna_animation.c b/source/blender/makesrna/intern/rna_animation.c
index b3af8e6b57d..90c072bcfcb 100644
--- a/source/blender/makesrna/intern/rna_animation.c
+++ b/source/blender/makesrna/intern/rna_animation.c
@@ -161,7 +161,7 @@ static StructRNA *rna_KeyingSetInfo_register(bContext *C, ReportList *reports, v
 {
 	KeyingSetInfo dummyksi = {0};
 	KeyingSetInfo *ksi;
-	PointerRNA dummyptr = {0};
+	PointerRNA dummyptr = {{0}};
 	int have_function[3];
 
 	/* setup dummy type info to store static properties in */