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

Closed
opened 2022-09-16 21:18:42 +02:00 by cmto · 30 comments

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.
    image.png

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

NLA_scale_issue.blend

**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)
Author

Added subscriber: @CHARLES-MURRAY

Added subscriber: @CHARLES-MURRAY

#102754 was marked as duplicate of this issue

#102754 was marked as duplicate of this issue

#101305 was marked as duplicate of this issue

#101305 was marked as duplicate of this issue

Added subscriber: @mano-wii

Added subscriber: @mano-wii

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

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

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

Good catch, thank you

Good catch, thank you
Pratik Borhade changed title from Scaling of NLA Strip Via S Hotkey Not Working to Regression: Scaling of NLA Strip Via S Hotkey Not Working 2022-09-17 08:16:18 +02:00

Added subscriber: @item412

Added subscriber: @item412
Member

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.
Member

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.
Member

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.
Member

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.
Member

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."

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)
Member

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.
Member

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?

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.
Member

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.

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.
Thibault de Villèle self-assigned this 2022-10-06 18:25:37 +02:00

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

  1. 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)
  1. 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.
  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.

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 'Confirmed' to: 'Resolved'

Changed status from 'Resolved' to: 'Confirmed'

Changed status from 'Resolved' to: 'Confirmed'

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

Silly Phabricator saw a pre-fix cleanup as enough to close this task. Tsssk tssk.
Member
Added subscribers: @HectorDeAnda, @PratikPB2123, @TheRedWaxPolice
Thibault de Villèle was unassigned by Sybren A. Stüvel 2022-12-06 19:14:38 +01:00
Sybren A. Stüvel self-assigned this 2022-12-06 19:14:38 +01:00

This issue was referenced by 51759e6595

This issue was referenced by 51759e659579eb48181306ae1574d4dfdd4a4ed9
Member

Changed status from 'Confirmed' to: 'Resolved'

Changed status from 'Confirmed' to: 'Resolved'
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
9 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#101130
No description provided.