Page MenuHome

Nla Refactor: Split animsys_evaluate_nla()
ClosedPublic

Authored by Wayde Moss (GuiltyGhost) on Dec 1 2020, 1:06 PM.

Details

Summary

XXX anim_sys.c) nlatrack_find_tweaked() is a temporary work around.
If anyone has any insight into this problem, help is appreciated.


Refactors animsys_evaluate_nla() into 2 versions:
animsys_evaluate_nla_for_keyframing(), animsys_evaluate_nla_for_flush()
to make it clear what data is being calculated and why.

Dummy strip creation has been refactored to two separate functions,
animsys_create_tweak_strip() and animsys_create_action_track_strip().
Both are evaluated differently from other strips and eachother. There's
no need to interweave them. A future patch D8296, generally requires
both strips.

Split from D9247: Refactor: NLA Prep for D8296
No intended functional changes.
Depends on D9695: Nla Refactor: Better blend function parm naming to be buildable.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D9696_2 (branched from master)
Build Status
Buildable 12171
Build 12171: arc lint + arc unit

Event Timeline

Wayde Moss (GuiltyGhost) requested review of this revision.Dec 1 2020, 1:06 PM
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)Dec 1 2020, 1:08 PM
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)
Sybren A. Stüvel (sybren) requested changes to this revision.Dec 3 2020, 2:40 PM

I can't check all the code changes for correctness, so I'll assume that you've done proper testing. What I can say is that this split-up certainly makes it easier to read :)

source/blender/blenkernel/intern/anim_sys.c
2249

can be const

2308

It's clearer to have non-inverted booleans, so:

const bool tweaking = (adt->flag & ADT_NLA_EDIT_ON) != 0;

This then makes the rest of the code follow naturally:

const bool soloing = ... 
const bool actionstrip_evaluated = r_action_strip->act && !soloing && !tweaking;
This revision now requires changes to proceed.Dec 3 2020, 2:40 PM
  • apply review note: apply const and simplify condition
Wayde Moss (GuiltyGhost) marked 2 inline comments as done.Dec 9 2020, 9:04 AM
  • rebase
  • fix IS_EQF() so right operand is literal float to prevent warnings/errors for other compilers
  • fix warnings (again)
Sybren A. Stüvel (sybren) requested changes to this revision.Jan 8 2021, 11:40 AM

The patch looks pretty good. There are two bits of code that could use some readability improvements, for those I've added some inline notes.

source/blender/blenkernel/intern/anim_sys.c
2320–2325

The code doesn't seem to be in line with the comment. From the comment, I would expect something like this:

if ((nlt->flag & NLATRACK_DISABLED) && !(contains active strip)) {
  return false;
}

As the code is now, I suspect that (adt->flag & ADT_NLA_EDIT_ON) and nlt->index != adt->act_track->index are part of the "contains active strip" aspect. If this is the case, the fact that it's split up into separate if-statements doesn't make it easier to read.

2349–2355

The first condition can be flipped to reduce the cognitive complexity:

if (adt->action == NULL) {
  return false;
}
if ((adt->flag & (ADT_NLA_SOLO_TRACK | ADT_NLA_EDIT_ON)) == 0 && !any_strip_evaluated) {
  /* Evaluate as if there isn't any NLA data. */
  return true;
}
return false;

You could even take it further and simplify the condition a bit as well. I'll leave it up to you which one you like best.

if (adt->action == NULL) {
  return false;
}
if (any_strip_evaluated) {
  return true;
}

/* Evaluate as if there isn't any NLA data. */
return (adt->flag & (ADT_NLA_SOLO_TRACK | ADT_NLA_EDIT_ON)) == 0;

The comment is a bit vague to me, so I don't know if I placed it over the correct part of the condition. I'll leave that up to you.

This revision now requires changes to proceed.Jan 8 2021, 11:40 AM
Wayde Moss (GuiltyGhost) marked 2 inline comments as done.
  • apply review notes. The simplifications does help alot, thanks.

Just some very minor comments about comments. It's fine to tweak them and then commit to master.
The only important note is creating a task to resolve the supposed-to-be-temporary situation. For some giggles, do a search for the words "for now" in the source, and check how old they are.

source/blender/blenkernel/intern/anim_sys.c
2371

For such temporary things, create a task that addresses the issue. It'll also give you a space to attach test files, describe test steps etc. That way it's not forgotten, and someone else might actually pick it up and fix it for you :)

Be sure to mention this patch in that task, so that the two get linked together. That way the eventual commit can be tracked down to this patch, and then to the task.

2381

Only use ... when there is actually something that follows it. Here it doesn't add much info beyond a normal full stop.

2388

Is this an unexpected condition? If so, describe that. If not, it's fine to remove the comment, because it's clear that there was no disabled track in that case.

This revision is now accepted and ready to land.Jan 14 2021, 1:06 PM
Wayde Moss (GuiltyGhost) marked 3 inline comments as done.Jan 15 2021, 12:27 AM