Regression: Scaling of NLA Strip Via S Hotkey Not Working #101130

New Issue

System Information
Operating system: Win 10 x64
Graphics card: 3080 TI

Blender Version
Broken: 3.3.1, caused by bd00324c26
Worked: 3.2.0, e05e1e3691, master, 2022-06-08 10:22

Short description of error
Hotkey S seems to not be affecting the 'playback scale' value when scaling an NLA strip.

Exact steps for others to reproduce the error

Open attached file
Select NLA strip,
press {key S 2} enter.
The scale value (in the Sidebar) is unchanged, and if you play the animation, it loops twice instead of being played 2x slower.

If you scale manually in the N sidebar, there are no issues.

**System Information** Operating system: Win 10 x64 Graphics card: 3080 TI **Blender Version** Broken: 3.3.1, caused by bd00324c26 Worked: 3.2.0, e05e1e369187, master, 2022-06-08 10:22 **Short description of error** Hotkey S seems to not be affecting the 'playback scale' value when scaling an NLA strip. **Exact steps for others to reproduce the error** - Open attached file - Select NLA strip, - press {key S 2} enter. The scale value (in the Sidebar) is unchanged, and if you play the animation, it loops twice instead of being played 2x slower. ![image.png](https://archive.blender.org/developer/F13500496/image.png) If you scale manually in the N sidebar, there are no issues. [NLA_scale_issue.blend](https://archive.blender.org/developer/F13500665/NLA_scale_issue.blend)

Added subscriber: @CHARLES-MURRAY

#102754 was marked as duplicate of this issue

#101305 was marked as duplicate of this issue

Added subscriber: @mano-wii

Changed status from 'Needs Triage' to: 'Confirmed'

I can confirm the problem.
It was introduced sometime between 4c3b984b3d and 0e9367fc29

investigating...

I can confirm the problem. It was introduced sometime between 4c3b984b3daa and 0e9367fc29bc investigating...

Added subscribers: @dr.sybren, @BClark, @thibaulltt

Good catch, thank you

Added subscriber: @item412

Right now the scale hot key changes the Strip Start/End time instead of Scale being mapped to the Animated Clip Scale because we changed the end time to no longer scale the clip.

I can see why this is working that way and also I can see why it is confusing. It is being looked at.

Right now the scale hot key changes the Strip Start/End time instead of Scale being mapped to the Animated Clip Scale because we changed the end time to no longer scale the clip. I can see why this is working that way and also I can see why it is confusing. It is being looked at.

In #101130#1423267, @BClark wrote:
...we changed the end time to no longer scale the clip.

I wonder what was the reason for changing behavior of Start/End sliders? Because scaling strips to particular frames is exactly what I was using those sliders for.

> In #101130#1423267, @BClark wrote: >...we changed the end time to no longer scale the clip. I wonder what was the reason for changing behavior of Start/End sliders? Because scaling strips to particular frames is exactly what I was using those sliders for.

The main reason is that "END" frame shouldn't have been changing the scale of the animation. When someone wanted to adjust the end of a clip, the would type in the end value that changed the Action Clip Scale attribute, changing the speed of the animation without them knowing it.

So now Start/End are just that, a start and end for the strip instead of scaling it, start moves the strip to the correct start frame end adjusts the end of the clip to be the global length you want it without scale. What isn't expected was that the S key scale didn't get reconnected to drive the Action Clip Scale value.

The main reason is that "END" frame shouldn't have been changing the scale of the animation. When someone wanted to adjust the end of a clip, the would type in the end value that changed the Action Clip Scale attribute, changing the speed of the animation without them knowing it. So now Start/End are just that, a start and end for the strip instead of scaling it, start moves the strip to the correct start frame end adjusts the end of the clip to be the global length you want it without scale. What isn't expected was that the S key scale didn't get reconnected to drive the Action Clip Scale value.

In #101130#1423615, @BClark wrote:
... When someone wanted to adjust the end of a clip, the would type in the end value that changed the Action Clip Scale attribute, changing the speed of the animation without them knowing it.

But after the patch if you adjust End frame to make strip longer, you adding frames to clip where's no keyframes.
Sorry for the offtopic, but the logic of latest changes escapes me and it makes job of copying strip structure from one object to another more cumbersome.

> In #101130#1423615, @BClark wrote: > ... When someone wanted to adjust the end of a clip, the would type in the end value that changed the Action Clip Scale attribute, changing the speed of the animation without them knowing it. But after the patch if you adjust End frame to make strip longer, you adding frames to clip where's no keyframes. Sorry for the offtopic, but the logic of latest changes escapes me and it makes job of copying strip structure from one object to another more cumbersome.

Yes the end frame of the strip will be longer and if you don't have keys, it holds or expands the strip. END frame should not have == scale of the clip and change the animation length.

The number of people that the first time they used the NLA and wanted the clip to HOLD the time for longer or to add keys into the clip but didn't have auto sync on or want to TRIM the clip length, would change that end frame thinking it was a "trim" and not a scale, and then would end up messing up their animation.

In all video editing tools, adjusting the end of the clip is a trim operation, not a scale/time change operation.

Yes the end frame of the strip will be longer and if you don't have keys, it holds or expands the strip. END frame should not have == scale of the clip and change the animation length. The number of people that the first time they used the NLA and wanted the clip to HOLD the time for longer or to add keys into the clip but didn't have auto sync on or want to TRIM the clip length, would change that end frame thinking it was a "trim" and not a scale, and then would end up messing up their animation. In all video editing tools, adjusting the end of the clip is a trim operation, not a scale/time change operation.

You still have Action Strip Scale and You should have once fixed Scale tool to scale a clip to fit.

Ideal would be that we have two modes and some visual handles/ controls over editing/adjusting the end of the clip so that one default end of clip manipulator would drag the end frame. and holding a modifier key or changing the mode the end or start drag would scale it.

The other thing that this fixes is allowing us to create a Trim tool that will be able to correctly trim a looped clip.

You still have Action Strip Scale and You should have once fixed Scale tool to scale a clip to fit. Ideal would be that we have two modes and some visual handles/ controls over editing/adjusting the end of the clip so that one default end of clip manipulator would drag the end frame. and holding a modifier key or changing the mode the end or start drag would scale it. The other thing that this fixes is allowing us to create a Trim tool that will be able to correctly trim a looped clip.

can you talk about this more or make a video on what you are talking about here "copying strip structure from one object to another more cumbersome."

In #101130#1423640, @BClark wrote:
Ideal would be that we have two modes and some visual handles/ controls over editing/adjusting the end of the clip so that one default end of clip manipulator would drag the end frame. and holding a modifier key or changing the mode the end or start drag would scale it.

That would be much welcomed!

In #101130#1423641, @BClark wrote:
can you talk about this more or make a video on what you are talking about here "copying strip structure from one object to another more cumbersome."

It's just my way of working.
Since I can't directly copy/paste strips from one object to another, i have to reproduce same strip structure manually.
When i lay out a row of strips with different scales, i can memorize END frames of strips in that row from other object, instead of constantly selecting back and forth to copy scale (which often looks something like 1.34473)

> In #101130#1423640, @BClark wrote: > Ideal would be that we have two modes and some visual handles/ controls over editing/adjusting the end of the clip so that one default end of clip manipulator would drag the end frame. and holding a modifier key or changing the mode the end or start drag would scale it. That would be much welcomed! > In #101130#1423641, @BClark wrote: > can you talk about this more or make a video on what you are talking about here "copying strip structure from one object to another more cumbersome." It's just my way of working. Since I can't directly copy/paste strips from one object to another, i have to reproduce same strip structure manually. When i lay out a row of strips with different scales, i can memorize END frames of strips in that row from other object, instead of constantly selecting back and forth to copy scale (which often looks something like 1.34473)

100% the not being able to rebuild/transfer or duplicate Edits across other objects is a huge pain. Thank you that makes much more sense and it is on my list of stuff to get fixed but so are lots of things.

So it would be less of an issue if we had a way to rebuild an edit /duplicate it and set objects /actions instead of you having to by hand rebuild it.

100% the not being able to rebuild/transfer or duplicate Edits across other objects is a huge pain. Thank you that makes much more sense and it is on my list of stuff to get fixed but so are lots of things. So it would be less of an issue if we had a way to rebuild an edit /duplicate it and set objects /actions instead of you having to by hand rebuild it.

https://www.reddit.com/r/blenderhelp/comments/4wt1xr/copying_and_pasting_nla_action_strips/ some workaround ideas until it is something built in? Have you looked at those?

In #101130#1423676, @BClark wrote:
https://www.reddit.com/r/blenderhelp/comments/4wt1xr/copying_and_pasting_nla_action_strips/ some workaround ideas until it is something built in? Have you looked at those?

I think I saw this. Seems like it's applicable only to identical armatures. In my case animation timing is the same, but armatures are different.
It would be great to have ability to copy/paste strips as containers and just assign proper actions to them after.

> In #101130#1423676, @BClark wrote: > https://www.reddit.com/r/blenderhelp/comments/4wt1xr/copying_and_pasting_nla_action_strips/ some workaround ideas until it is something built in? Have you looked at those? I think I saw this. Seems like it's applicable only to identical armatures. In my case animation timing is the same, but armatures are different. It would be great to have ability to copy/paste strips as containers and just assign proper actions to them after.

Yeah it is for identical armatures but you could duplicate the armature, the clear it of bones, join the different armature into it then swap out the action clip actions. and keep the strips. ugly but something to think about for now.

Thank you for the discussion

Yeah it is for identical armatures but you could duplicate the armature, the clear it of bones, join the different armature into it then swap out the action clip actions. and keep the strips. ugly but something to think about for now. Thank you for the discussion

Please keep the comments on a bug strictly on the topic of that bug. This is not a general discussion forum, and having too much noise only slows down the bugfixing.

Hi all, I was a bit late to the party. I'm here to outline exactly how this patch will be resolved as well as define what went wrong.

What went wrong ?

Commit bd00324c26 introduced a new way to manage the frame_start and frame_end properties of a NlaStrip. This meant all code accessing those properties would have been based on the earlier behavior of the properties and thus prone to error (as the old workflow itself wasn't all that well defined).
The operations of the NLA Editor were nearly all changed ... except I only took into account the code inside the source/editors/space_nla folder. Any code using the NLA structures/data from outside those folders would not have been updated, and this is the root of the problem for this task.

Given those initial conditions, bugs were inevitable. And this is no exception ! Let's review why this particular bug happened.

The keymap for Blender is actually defined as Python files (the one of interest here is release/scripts/presets/keyconfig/keymap_data/blender_default.py at line 2603).
Those Python files actually map the keypress input by the user to RNA actions. In this case, we need to have a look at the transform.transform operation.

"Oh god no, not the transform system" -- Any Blender developer

After following a trail of loose function pointers and some very unsafe memory reinterpretations, we end up in the file source/blender/editors/transform/transform_convert_nla.c at line 389, in the recalcData_nla() function.

N.B. : This function is a bit of a behemoth, it does many things not related to the actual type of the transform operation (defined as flags of the eTfmMode enum)

The problem stems from the fact the function uses the frame_start and frame_end properties as they were previously behaving. Furthermore, the code was actually modified to overcome a limitation of the RNA properties of the NlaStrip that existed beforehand (see #33852 for the original bug report and more discussion on this).

What will be done ?

As @dr.sybren mentioned in a private chat with @BClark, Nathaniel and myself the best course of action would be the following :

Create a first "cleanup" patch, which does not change the functionality of the NLA transform operations but only moves the RNA code out of the recalcData_nla() function. EDIT : this is now uploaded under D16181.
Create a second patch aimed at actually resolving the issue, with another function encapsulating all RNA operations for the scale attribute.

This needs to be performed in order to make the function actually more readable (it's already at 200+ lines without any special handling for different transform modes). This will also help with code review, as the recalcData_nla() function will thus only serve as a dispatcher to dedicated transform functions for the NLA structures, and the case-by-case nature of all different transform types will be handled in separate functions.

I will update the comment with the dedicated Differential revisions for both steps when they will be published.

Hi all, I was a bit late to the party. I'm here to outline exactly how this patch will be resolved as well as define what went wrong. **What went wrong ?** - Commit bd00324c26 introduced a new way to manage the `frame_start` and `frame_end` properties of a `NlaStrip`. This meant all code accessing those properties would have been based on the earlier behavior of the properties and thus prone to error (as the old workflow itself wasn't all that well defined). - The operations of the NLA Editor were nearly all changed ... except I only took into account the code inside the `source/editors/space_nla` folder. Any code using the NLA structures/data from outside those folders would not have been updated, and this is the root of the problem for this task. Given those initial conditions, bugs were inevitable. And this is no exception ! Let's review why this particular bug happened. 1. The keymap for Blender is actually defined as Python files (the one of interest here is `release/scripts/presets/keyconfig/keymap_data/blender_default.py` at line 2603). 2. Those Python files actually map the keypress input by the user to RNA actions. In this case, we need to have a look at the `transform.transform` operation. > "Oh god no, not the transform system" -- Any Blender developer 3. After following a trail of loose function pointers and some ***very*** unsafe memory reinterpretations, we end up in the file `source/blender/editors/transform/transform_convert_nla.c` at line 389, in the `recalcData_nla()` function. - **N.B.** : This function is a bit of a behemoth, it does many things not related to the actual type of the transform operation (defined as flags of the `eTfmMode` enum) 4. The problem stems from the fact the function uses the `frame_start` and `frame_end` properties as they were previously behaving. Furthermore, the code was actually modified to overcome a limitation of the RNA properties of the `NlaStrip` that existed beforehand (see #33852 for the original bug report and more discussion on this). **What will be done ?** As @dr.sybren mentioned in a private chat with @BClark, Nathaniel and myself the best course of action would be the following : 1. Create a first "cleanup" patch, which does not change the functionality of the NLA transform operations but only moves the RNA code out of the `recalcData_nla()` function. **EDIT** : this is now uploaded under [D16181](https://archive.blender.org/developer/D16181). 2. Create a second patch aimed at actually resolving the issue, with another function encapsulating all RNA operations for the scale attribute. This needs to be performed in order to make the function actually more readable (it's already at 200+ lines without any special handling for different transform modes). This will also help with code review, as the `recalcData_nla()` function will thus only serve as a dispatcher to dedicated transform functions for the NLA structures, and the case-by-case nature of all different transform types will be handled in separate functions. I will update the comment with the dedicated Differential revisions for both steps when they will be published.

This issue was referenced by 962b647690

This issue was referenced by 962b64769018414616d50d28914a4ce4b65b490b

Changed status from 'Confirmed' to: 'Resolved'

Changed status from 'Resolved' to: 'Confirmed'

Silly Phabricator saw a pre-fix cleanup as enough to close this task. Tsssk tssk.

Added subscribers: @HectorDeAnda, @PratikPB2123, @TheRedWaxPolice

This issue was referenced by 51759e6595

This issue was referenced by 51759e659579eb48181306ae1574d4dfdd4a4ed9

Changed status from 'Confirmed' to: 'Resolved'

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Regression: Scaling of NLA Strip Via S Hotkey Not Working #101130