Details

Reviewers

Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Commits

rBcbeeca8167f8: NLA: Extract ..get_inverted_upper_snapshot()

Summary

Extracts nlasnapshot_blend_get_inverted_upper_snapshot() from BKE_animsys_nla_remap_keyframe_values()

This introduces a new struct member: NlaEvalChannelSnapshot->remap_domain and marks which values of blended_snapshot are processed for remapping/used-for-inverting. Effectively, it marks which values have successfully been reampped and can be further used for remapping.
nlasnapshot_blend_get_inverted_upper_snapshot(): the output snapshot r_upper_snapshot has each channel's remap_domain written to which effectively marks the successfully remapped values. The only reason a value is not in the remap domain is if inversion failed or it wasn't marked to be remapped.
nlasnapshot_enable_all_remap_domain() is unused in this patch but seemed generally useful when using snapshots for remapping. D8867: Feature: NLA Merge Strips would be able to make use of it. Though I suppose that means that D8867 should also introduce it?
nlasnapshot_blend_get_inverted_upper_snapshot() has a variant nlasnapshot_blend() from D10220: NLA: Extract nlasnapshot_blend(), but this patch doesn't depend on it at all. A third variant will later be added nlasnapshot_blend_get_inverted_lower_snapshot(). Altogether, these three functions allow solving for any of (lower_snapshot, upper_snapshot, blended_snapshot) given the other two. The function ...lower_snapshot() will also similarly process the remap domain of the blended and lower snapshot.
added assertions within nlasnapshot_blend() and nlasnapshot_blend_get_inverted_upper_snapshot() to future proof branches dealing with blendmode and mixmodes. (suggested by sybren)

No user functional changes

Diff Detail

Repository

rB Blender

Branch

arcpatch-D10222_3 (branched from master)

Build Status

Buildable 13554
Build 13554: arc lint + arc unit

Event Timeline

Wayde Moss (GuiltyGhost) requested review of this revision.Jan 27 2021, 6:49 AM

Wayde Moss (GuiltyGhost) created this revision.

Harbormaster completed remote builds in B12410: Diff 33197.Jan 27 2021, 6:49 AM

Wayde Moss (GuiltyGhost) retitled this revision from NLA: Extract ...inverted_upper_snapshot() to NLA: Extract nlasnapshot_blend_get_inverted_upper_snapshot().Jan 27 2021, 7:28 AM

Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)

Wayde Moss (GuiltyGhost) added reviewers: Sybren A. Stüvel (sybren), Animation & Rigging.

Wayde Moss (GuiltyGhost) added a project: Animation & Rigging.

rebase and minor cleanup

Harbormaster completed remote builds in B12593: Diff 33518.Feb 4 2021, 12:07 AM

Sybren A. Stüvel (sybren) requested changes to this revision.Feb 5 2021, 12:37 PM

Sybren A. Stüvel (sybren) added inline comments.

source/blender/blenkernel/intern/anim_sys.c
2932–2933	You may want to add some `BLI_assert()` calls here, then, just so that you can get a warning when this assumption fails.
2943	When only one item of an enum gets handled in an `if`-statement, and the rest is shoved into an `else`, it feels fragile to me. There are no guarantees that the `else` clause is always correct for future additional items, and there will be no warnings from the compiler that new cases have been added. I would prefer either a `switch` with an explicit `case` for every enum item, or a `BLI_assert(ELEM(mode, ....))` in the `else` that checks that the blend/mix mode is actually as expected.
3021	Why the `{`? If you want to make this a separate block, would it make sense to actually make it a separate function instead?
source/blender/blenkernel/nla_private.h
82–84	I don't quite understand this comment. "further inverted" implies that the marked strips have already been inverted. However, inverting "further" (i.e. double negation) would simply mean restoring back to the original working of the NLA strip, which is possible for all strips, so no mask would be necessary. It's unclear whether the mask indicates "can be inverted" (in which case the variable name is correct) or "has been inverted" (in which case the variable name is ambiguous).

This revision now requires changes to proceed.Feb 5 2021, 12:37 PM

applied review notes:
assertions on else() cases for future proofing
renamed invertible to remap_domain for clarity, thanks
should now work when blended_snapshot and output r_upper_snapshot are different pointers. Currently they're the same so nothing broke. This means that we now explicitly copy over the disabled remap_domain bits from blended_snapshot to r_upper_snapshot. They're of the form:

if (!BLI_BITMAP_TEST_BOOL(blended_necs->remap_domain.ptr, j)) {
   BLI_BITMAP_DISABLE(result_necs->remap_domain.ptr, j);
   continue;
}

Harbormaster completed remote builds in B12740: Diff 33742.Feb 9 2021, 6:52 AM

Wayde Moss (GuiltyGhost) added inline comments.Feb 9 2021, 6:54 AM

source/blender/blenkernel/intern/anim_sys.c
2932–2933	That's a poorly explained comment, sorry. This new one should be better. In the `remap()` function, the `lower_snapshot` will generally have N channels and we fill `blended_snapshot` with only one channel, the one we're solving for. Thus it's more that we assume the caller only wants to process channels where the `blended_necs` exist. So there shouldn't be a `BLI_assert()`.
3021	This was just a habit for blocks of code that could be a function but aren't since they're only used once and not expected to be reused anytime soon. Given that, I'd rather not make it a separate function until I have more use cases.
source/blender/blenkernel/nla_private.h
82–84	It's used for both meanings. When removing the effect of an upper layer, the result snapshot stores whether the value has successfully been inverted and thus the value is valid. The input `blended_snapshot` and output `lower_snapshot` are the same in this case (for a call to a future function `nlasnapshot_blend_get_inverted_lower_snapshot()`). When we remove the effect of another upper layer, those values that were successfully inverted are still valid values that can still be used. If not invertible, then we can't solve for the lower stack values and must skip those. We cannot use the result for keyframing because there was no solution. I suppose a better name would be `remap_domain`.

rebase and minor cleanup

Harbormaster completed remote builds in B12942: Diff 34078.Feb 17 2021, 7:24 AM

Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)Feb 17 2021, 7:40 AM

Overlooked remap_domain writing, now writes remap_domain on output even when remapping succeeds. This didn't have any effect for the user as the input blended_snapshot and output upper_snapshot were always the same.

Harbormaster completed remote builds in B12963: Diff 34112.Feb 17 2021, 10:27 PM

Wayde Moss (GuiltyGhost) mentioned this in D10504: NLA: Keyframe Remap Through Upper Strips.Feb 22 2021, 10:13 PM

Sybren A. Stüvel (sybren) added inline comments.Mar 1 2021, 5:04 PM

source/blender/blenkernel/intern/anim_sys.c
3021	Reuse is only one reason to put code into a separate function. A change in abstraction level is another one, and the ability to have a clear name for a function (instead of an anonymous block) is also an advantage.

remove unnecessary braces

Harbormaster completed remote builds in B13272: Diff 34786.Mar 5 2021, 5:42 AM

Wayde Moss (GuiltyGhost) marked an inline comment as done.Mar 5 2021, 5:43 AM

nlasnapshot_enable_all_remap_domain() is unused in this patch but seemed generally useful when using snapshots for remapping. D8867: Feature: NLA Merge Strips would be able to make use of it. Though I suppose that means that D8867 should also introduce it?

Yes, introducing it in the same patch that uses it is a good idea. Otherwise there is no way to know (except for very carefully studying every line) whether the code is actually working.

source/blender/blenkernel/intern/anim_sys.c
2947–2976	Is this code really depending on the fact that this function is getting the inverted upper snapshot? Or could it be extracted into a separate function, which then could potentially also be reused in other places? Same question for the `else`-block too. This function now has higher cognitive complexity than I'm comfortable with.

This revision now requires changes to proceed.Mar 5 2021, 10:12 AM

Ok, I'll remove the extra function.

Is this what you had in mind?

D10222_sybren_suggested_changes.diff17 KBDownload

Or were you looking for a simplification that doesn't depend on NlaEvalChannelSnapshots. Note any simplification would still require value masks to know which values should be blended and which should be directly copied from the lower stack.

For re-usability, I'm not aware of any existing areas or potential bugfixes that would make use of the extra level of granularity. I have considered T64487: 2.80 Pose Breakdowner doesn't work in Add NLA Strips but the solution there would likely work on the nla strip blending level (which would be introduced in D10504: NLA: Keyframe Remap Through Upper Strips). If there's an area you have in mind, let me know.

Edit: 3/12
I'm not sure about the change since it adds a level of granularity that's unlikely to be used anywhere.

The current levels of granularity for blending are: plain values (nla_blend_values() and variants), snapshots (nlasnapshot_blend() and variants), and then finally strips (nlasnapshot_blend_strip() and variants). The plain values level is the lowest and simplest level. Snapshots adds support for blending many values at once. Finally, strips add support for actions, animated influence, animated time, repeat, scale, strip types, etc.

Yes, much better!

I'm not sure about the change since it adds a level of granularity that's unlikely to be used anywhere.

I don't quite understand what you mean here.

remove unnecessary extra function
Apply Sybren's review notes: simplify nlasnapshot_blend() by extracting nlaevalchan_blendOrcombine() for NlaEvalChannelSnapshot blending/combining. Same goes for the invert variation.

I don't quite understand what you mean here

I've compromised by implementing the suggestion. Let me know if an explanation is still desired.

Harbormaster completed remote builds in B13531: Diff 35234.Mar 16 2021, 3:35 AM

Wayde Moss (GuiltyGhost) marked an inline comment as done.Mar 16 2021, 3:37 AM

There are a whole bunch of compiler warnings:

.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c: In function ‘nlaevalchan_assert_nonNull’:
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:1656:64: warning: unused parameter ‘necs’ [-Wunused-parameter]
 1656 | static void nlaevalchan_assert_nonNull(NlaEvalChannelSnapshot *necs)
      |                                        ~~~~~~~~~~~~~~~~~~~~~~~~^~~~
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c: In function ‘nlaevalchan_assert_blendOrcombine_compatible’:
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:1662:82: warning: unused parameter ‘lower_necs’ [-Wunused-parameter]
 1662 | static void nlaevalchan_assert_blendOrcombine_compatible(NlaEvalChannelSnapshot *lower_necs,
      |                                                          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:1663:82: warning: unused parameter ‘upper_necs’ [-Wunused-parameter]
 1663 |                                                          NlaEvalChannelSnapshot *upper_necs,
      |                                                          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:1664:82: warning: unused parameter ‘blended_necs’ [-Wunused-parameter]
 1664 |                                                          NlaEvalChannelSnapshot *blended_necs)
      |                                                          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c: In function ‘nlasnapshot_blend’:
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:2862:15: warning: unused variable ‘length’ [-Wunused-variable]
 2862 |     const int length = nec->base_snapshot.length;
      |               ^~~~~~
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:2859:14: warning: unused variable ‘zero_upper_influence’ [-Wunused-variable]
 2859 |   const bool zero_upper_influence = IS_EQF(upper_influence, 0.0f);
      |              ^~~~~~~~~~~~~~~~~~~~
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c: In function ‘nlasnapshot_blend_get_inverted_upper_snapshot’:
.../blender-git/blender/source/blender/blenkernel/intern/anim_sys.c:2898:15: warning: unused variable ‘length’ [-Wunused-variable]
 2898 |     const int length = nec->base_snapshot.length;
      |               ^~~~~~

fix warnings

Harbormaster completed remote builds in B13553: Diff 35280.Mar 17 2021, 12:49 AM

remove unrelated formatting change from last patch update

Harbormaster completed remote builds in B13554: Diff 35281.Mar 17 2021, 12:57 AM

LGTM

This revision is now accepted and ready to land.Apr 20 2021, 2:35 PM

Closed by commit rBcbeeca8167f8: NLA: Extract ..get_inverted_upper_snapshot() (authored by Wayde Moss (GuiltyGhost)). · Explain WhyMay 12 2021, 4:12 AM

This revision was automatically updated to reflect the committed changes.

Wayde Moss (GuiltyGhost) added a commit: rBcbeeca8167f8: NLA: Extract ..get_inverted_upper_snapshot().

NLA: Extract nlasnapshot_blend_get_inverted_upper_snapshot()
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 35281

source/blender/blenkernel/intern/anim_sys.c

source/blender/blenkernel/nla_private.h

NLA: Extract nlasnapshot_blend_get_inverted_upper_snapshot()ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 35281

source/blender/blenkernel/intern/anim_sys.c

source/blender/blenkernel/nla_private.h

NLA: Extract nlasnapshot_blend_get_inverted_upper_snapshot()
ClosedPublic
Actions

Revision Contents
Changeset List