Page MenuHome
Differential D16181

Fix T101130 (part 1) : simplify recalcData_nla()
ClosedPublic
Actions

Authored by Thibault de Villèle (thibaulltt) on Oct 7 2022, 12:47 PM.

Details

Reviewers
Sybren A. Stüvel (sybren)
Commits
rB962b64769018: Cleanup: NLA transforms, simplify `recalcData_nla()`
Summary

N.B. : this does not modify the functionality of the transform system,
but merely sets the stage for an easier comprehension of the NLA transform
stack application.

This diff adds a function to separate the strip modification from the
recomputation of the NLA data. Since we're going to modify the function
later, it is better to have the NlaStrip-modifying logic to another
function to facilitate code review.

Also, this adds a couple BKE functions for the NlaStrip, in order to find
the previous/next strips in the strip's track.

The next revision will focus on actually fixing T101130.

Diff Detail

Repository
rB Blender

Event Timeline

Thibault de Villèle (thibaulltt) requested review of this revision.Oct 7 2022, 12:47 PM
Thibault de Villèle (thibaulltt) created this revision.
Harbormaster completed remote builds in B24174: Diff 56583.Oct 7 2022, 12:47 PM
Thibault de Villèle (thibaulltt) edited the summary of this revision. (Show Details)Oct 7 2022, 12:48 PM
Thibault de Villèle (thibaulltt) updated this revision to Diff 56584.Oct 7 2022, 12:50 PM
  • Fix formatting for the modified files (oops)
Harbormaster completed remote builds in B24175: Diff 56584.Oct 7 2022, 12:50 PM
Thibault de Villèle (thibaulltt) mentioned this in T101130: Regression: Scaling of NLA Strip Via S Hotkey Not Working.Oct 7 2022, 1:14 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Oct 7 2022, 2:42 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/blenkernel/intern/nla.c
1286–1287

In that case, I'd say return NULL; to be explicit. Same in the function below.

source/blender/editors/transform/transform_convert_nla.c
69–74

Excellent commenting here!

BUT, for now I would just put the original code in here, together with the original comment.

/* blablabla */
RNA_float_set(&strip_ptr, "frame_start", tdn->h1[0]);
RNA_float_set(&strip_ptr, "frame_end", tdn->h2[0]);

RNA_float_set(&strip_ptr, "frame_start", tdn->h1[0]);
RNA_float_set(&strip_ptr, "frame_end", tdn->h2[0]);

That way it's super easy to see this is a non-functional change. I do agree that what you did is an improvement, but it's easier to comprehend if it's separated from this patch.

This revision now requires changes to proceed.Oct 7 2022, 2:42 PM
Thibault de Villèle (thibaulltt) marked 2 inline comments as done.Oct 10 2022, 10:26 AM

New update to the patch incoming in a couple minutes ...

It fixes the non-explicit null return statements, and restores old code to the transform_convert_nla.c file to make it more obvious this is a non-functional change.
I still kept my comment and the frame_start_ui property change (commented) right below to prepare for the next diff :)

Thibault de Villèle (thibaulltt) updated this revision to Diff 56663.Oct 10 2022, 10:27 AM
  • More explicit NULL returns in BKE_nla
  • Old code from recalcData_nla() restored (non-functional change)
Harbormaster completed remote builds in B24196: Diff 56663.Oct 10 2022, 10:27 AM
Sybren A. Stüvel (sybren) accepted this revision.Oct 10 2022, 4:53 PM

Much better :)

Some comments can be removed, because either they just say what the code already expresses clearly, or they're documenting the history of the code. I'll tackle those when landing the patch.

source/blender/blenkernel/intern/nla.c
1286

This comment can be removed, as it just puts the while() condition into words. I'll do that when landing the patch.

source/blender/editors/transform/transform_convert_nla.c
69

This doesn't need to describe the history, that's what we have Git for. I can remove it when landing the patch.

This revision is now accepted and ready to land.Oct 10 2022, 4:53 PM
Sybren A. Stüvel (sybren) added a comment.Oct 10 2022, 5:19 PM
In D16181#441417, @Thibault de Villèle (thibaulltt) wrote:

I still kept my comment and the frame_start_ui property change (commented) right below to prepare for the next diff :)

Right now it looks like it'll be an addition to that function, instead of a replacement of the earlier code. Instead of having to explain that as well, I'll remove the comment when landing the patch. The simpler the patch, the easier it is to understand. I'll make sure that the commit message does contain a "foreshadowing" of upcoming changes.

Sybren A. Stüvel (sybren) updated this revision to Diff 56684.Oct 10 2022, 5:20 PM
  • Remove some comments.
Harbormaster completed remote builds in B24207: Diff 56684.Oct 10 2022, 5:20 PM
Closed by commit rB962b64769018: Cleanup: NLA transforms, simplify `recalcData_nla()` (authored by Thibault de Villèle (thibaulltt), committed by Sybren A. Stüvel (sybren)). · Explain WhyOct 10 2022, 5:31 PM
This revision was automatically updated to reflect the committed changes.
Sybren A. Stüvel (sybren) added a commit: rB962b64769018: Cleanup: NLA transforms, simplify `recalcData_nla()`.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
18 lines
intern/
28 lines
editors/
transform/
43 lines

Diff 56685

View Options

source/blender/blenkernel/BKE_nla.h

Loading...
View Options

source/blender/blenkernel/intern/nla.c

Loading...
View Options

source/blender/editors/transform/transform_convert_nla.c

Loading...