Maniphest T82234

NLA: Action Track Hold Forward Inconsistency
Confirmed, NormalDESIGN

Assigned To
None
Authored By
Wayde Moss (GuiltyGhost)
Oct 29 2020, 7:58 PM
Tags
  • Animation & Rigging
Subscribers
Brad Clark (RiggingDojo)
Wayde Moss (GuiltyGhost)

Description

Bug Report: T66946: NLA strip unexpectedly auto-switching from Hold to Hold Forward

Problem: For the Action Track (non pushed action), extrapolation None and Hold is respected but Hold_Forward is not. It's a source of confusion.

Solution: Originally proposed by angavrilov. We treat extrapolation Hold_Forward as bounded on the left and infinite on the right.

FModifiers note: It's important to note that the Action Track does not evaluate the same as normal strips which is intentional. FModifiers will affect the track's boundaries so keyframes don't necessarily define the Action Track's evaluation bounds. So adding a default cyclic modifier to channels effectively makes the Action Track's evaluation bounds infinite. Even extrapolation None in this case appears to not work properly. This behavior is still intentional even though it appears broken. The user-side solution is to restrict the modifier's frame range.

Revisions and Commits

Event Timeline

Wayde Moss (GuiltyGhost) created this task.Oct 29 2020, 7:58 PM
Wayde Moss (GuiltyGhost) added a project: Animation & Rigging.
Sybren A. Stüvel (sybren) moved this task from Backlog to Design on the Animation & Rigging board.Oct 30 2020, 11:17 AM
Wayde Moss (GuiltyGhost) changed the task status from Needs Triage to Confirmed.Oct 30 2020, 9:22 PM
Wayde Moss (GuiltyGhost) added a comment.EditedDec 23 2020, 11:19 PM

Patch that fixes it: Relative to D9696: Nla Refactor: Split animsys_evaluate_nla() A proper patch will be made later to avoid having to apply the fix twice (once before the refactor and after the refactor).

diff --git a/source/blender/blenkernel/intern/anim_sys.c b/source/blender/blenkernel/intern/anim_sys.c
index f7799357602..eec92271e45 100644
--- a/source/blender/blenkernel/intern/anim_sys.c
+++ b/source/blender/blenkernel/intern/anim_sys.c
@@ -864,8 +864,18 @@ NlaEvalStrip *nlastrips_ctime_get_strip(ListBase *list,
   /* loop over strips, checking if they fall within the range */
   for (strip = strips->first; strip; strip = strip->next) {
     /* check if current time occurs within this strip  */
-    if (IN_RANGE_INCL(ctime, strip->start, strip->end) ||
-        (strip->flag & NLASTRIP_FLAG_NO_TIME_MAP)) {
+
+    bool in_range = IN_RANGE_INCL(ctime, strip->start, strip->end);
+    if (strip->flag & NLASTRIP_FLAG_NO_TIME_MAP) {
+      if (ELEM(strip->extendmode, NLASTRIP_EXTEND_HOLD)) {
+        in_range = true;
+      }
+      else if (ELEM(strip->extendmode, NLASTRIP_EXTEND_HOLD_FORWARD)) {
+        in_range = ctime >= strip->start;
+      }
+    }
+
+    if (in_range) {
       /* this strip is active, so try to use it */
       estrip = strip;
       side = NES_TIME_WITHIN;
@@ -2297,16 +2307,12 @@ static void animsys_create_action_track_strip(const AnimData *adt,
   r_action_strip->extendmode = adt->act_extendmode;
   r_action_strip->influence = adt->act_influence;
 
-  /* NOTE: must set this, or else the default setting overrides,
-   * and this setting doesn't work. */
-  r_action_strip->flag |= NLASTRIP_FLAG_USR_INFLUENCE;
-
-  /* Unless extendmode is Nothing (might be useful for flattening NLA evaluation), disable range.
-   * Extendmode Nothing and Hold will behave as normal. Hold Forward will behave just like Hold.
-   */
-  if (r_action_strip->extendmode != NLASTRIP_EXTEND_NOTHING) {
-    r_action_strip->flag |= NLASTRIP_FLAG_NO_TIME_MAP;
-  }
+  /* Must set NLASTRIP_FLAG_USR_INFLUENCE, or else the default setting overrides, and influence
+   * doesn't work.
+   *
+   * Must set NLASTRIP_FLAG_NO_TIME_MAP, so Action Track evaluation extends beyond its keyframe
+   * bounds. */
+  r_action_strip->flag |= NLASTRIP_FLAG_USR_INFLUENCE | NLASTRIP_FLAG_NO_TIME_MAP;
 
   const bool tweaking = (adt->flag & ADT_NLA_EDIT_ON) != 0;
   const bool soloing = (adt->flag & ADT_NLA_SOLO_TRACK) != 0;
Wayde Moss (GuiltyGhost) mentioned this in T45854: Hold forward is reseted in NLA.Feb 24 2021, 10:03 PM
Brad Clark (RiggingDojo) added a subscriber: Brad Clark (RiggingDojo).Dec 13 2021, 9:27 PM

Documenting what is "needed" to get this patch moving forward to the finish line.

"- The patch description should be expanded, to explain better what the patch does from a user's perspective.

  • The new code in nlastrips_ctime_get_strip() should be extracted into its own function, so that it can have a clear name & single responsibility.

Only once the former is done can I really say anything about how it's implemented."