Anim: Separate keying flags #115525

Merged
Christoph Lendenfeld merged 31 commits from ChrisLend/blender:autokey_separate_flags into main 2024-01-19 16:26:19 +01:00

User facing changes

Splits the flag ..._FLAG_INSERTNEEDED between autokey and
manual keying. The fact that this flag was shared between the two
systems has been the cause of issues in the past. It wouldn't
let you insert a keyframe even though you explicitly used an operator
to do so.

In order to be clearer what options are used where, the user preferences
have been reordered
image

By default "Only Insert Needed" will be enabled for auto-keying, but not for manual keying.
The versioning code will enable both if it was enabled previously.

Test build: https://builder.blender.org/download/patch/PR115525/

Code side changes

The keying system has flags that define the behavior
when keys are inserted. Some of those flags were shared
between keying and auto-keying. Some were only used for
auto-keying.
To clarify that, prefix flags that used exclusively in one or the other
system with AUTOKEY/MANUALKEY

Also the flag name on the user preferences and the tool settings was renamed.
Previously it was called autokey_flag. To indicated that it is not only used
for autokeying, rename it keying_flag.

Fixes: #73773


Part of: #113278: Anim: Keyframing Rework

# User facing changes Splits the flag `..._FLAG_INSERTNEEDED` between autokey and manual keying. The fact that this flag was shared between the two systems has been the cause of issues in the past. It wouldn't let you insert a keyframe even though you explicitly used an operator to do so. In order to be clearer what options are used where, the user preferences have been reordered ![image](/attachments/73ae4d8e-d9fa-4778-9035-697d3515d537) By default "Only Insert Needed" will be enabled for auto-keying, but not for manual keying. The versioning code will enable both if it was enabled previously. Test build: https://builder.blender.org/download/patch/PR115525/ # Code side changes The keying system has flags that define the behavior when keys are inserted. Some of those flags were shared between keying and auto-keying. Some were only used for auto-keying. To clarify that, prefix flags that used exclusively in one or the other system with `AUTOKEY`/`MANUALKEY` Also the flag name on the user preferences and the tool settings was renamed. Previously it was called `autokey_flag`. To indicated that it is not only used for autokeying, rename it `keying_flag`. Fixes: #73773 ---- Part of: [#113278: Anim: Keyframing Rework](https://projects.blender.org/blender/blender/issues/113278)
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2023-11-28 16:05:35 +01:00
Christoph Lendenfeld added 12 commits 2023-11-28 16:05:52 +01:00
Christoph Lendenfeld added 1 commit 2023-11-28 16:29:52 +01:00
Christoph Lendenfeld added 1 commit 2023-12-01 10:50:31 +01:00
Christoph Lendenfeld changed title from WIP: Anim: Separate keying flags to Anim: Separate keying flags 2023-12-14 10:16:32 +01:00
Christoph Lendenfeld added 2 commits 2023-12-14 10:45:49 +01:00
Christoph Lendenfeld added 1 commit 2023-12-14 10:57:53 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
7fec5fa8be
move versioning code to correct file
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115525) when ready.
Christoph Lendenfeld added 1 commit 2024-01-05 11:44:46 +01:00
Christoph Lendenfeld added 2 commits 2024-01-05 13:50:05 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
29cce7b710
fix failing unit tests
First-time contributor

After testing the functionality of the new 'only insert needed' implementation, I stumbled on the following issues:

  • with only insert needed turned off, the custom properties are not keying the whole keying set (which was always the case, so i wonder if this behaviour needs to be changed).
  • manual 'only insert needed' seems to conflict when a keying set is activated. (gives an error that excludes other channels that were not keyed)
  • when resetting transformations (ALT + S/R/G) 'only insert needed' seems to be ignored.
After testing the functionality of the new 'only insert needed' implementation, I stumbled on the following issues: - with only insert needed turned off, the custom properties are not keying the whole keying set (which was always the case, so i wonder if this behaviour needs to be changed). - manual 'only insert needed' seems to conflict when a keying set is activated. (gives an error that excludes other channels that were not keyed) - when resetting transformations (ALT + S/R/G) 'only insert needed' seems to be ignored.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115525) when ready.
Christoph Lendenfeld added 2 commits 2024-01-16 13:18:42 +01:00
Christoph Lendenfeld added 1 commit 2024-01-16 16:17:17 +01:00
Author
Member
  • when resetting transformations (ALT + S/R/G) 'only insert needed' seems to be ignored.

@Rikstopher I know you showed me this issue on the call, but I can't reproduce this now. For you, Blender created keyframes on Location+Rotation+Scale when you hit ALT+G. For me this creates only location keyframes, doesn't matter if "Only Insert Needed" is enabled or not

> - when resetting transformations (ALT + S/R/G) 'only insert needed' seems to be ignored. @Rikstopher I know you showed me this issue on the call, but I can't reproduce this now. For you, Blender created keyframes on Location+Rotation+Scale when you hit ALT+G. For me this creates only location keyframes, doesn't matter if "Only Insert Needed" is enabled or not
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-01-16 18:20:51 +01:00
Contributor

I've tested this as well, functionality is very good. But UX has little issues.

Questions about functionality:

  1. When manual keying with "Only Insert Needed" and you hit I for the first time, meaning you have no prior keyframes, it still keyframes every transform on all three axis even if you modified only one. Is that expected? Because for example if I moved default cube only on X axis and hit I, I am expecting that only X location gets keyframed, and not Y,Z and definately not RotScale

  2. When auto-keying with "Only Insert Needed" and manually change value from properties or Item tab in sidebar its not keyframed. Is that a bug or expected? If I have loc+scale keyframes and I keep changing them it works fine, but for rotation, which is not keyframed yet, entering new value in the field doesnt work, but rotating with R in 3D viewport does keyframe it. (I checked, Only Insert Available isn't enabled)


UX feedback:

  1. When Only Insert Needed is enabled for manual keying you get this error when hovering over property and hitting I

Failed to insert keys on F-Curve with path 'location[0]', ensure that it is not locked or sampled, and try removing F-Modifiers

This error while descriptive is very annoying, especially when I was animating fast it just kept popping in my face and spamming my Info/console. Also, why is that an error at all? When I enable it, or lock the channel I expect Blender to know that it was my conscious choice and I dont want to be asked about it every time. There should be no error or text popup at all in my opinion.

  1. Descriptions when hovering on items in preferences is very basic and kinda just says same thing title does in different wording. I propose this titles:

Only Insert Needed Insert keyframes only for properties that were modified (i.e. they have different value than last keyframe)
Only Insert Available Insert Keyframes only for properties that already have F-Curve (i.e. they were manually keyframed)

I've tested this as well, functionality is very good. But UX has little issues. Questions about functionality: 1. When manual keying with "Only Insert Needed" and you hit I for the first time, meaning you have no prior keyframes, it still keyframes every transform on all three axis even if you modified only one. Is that expected? Because for example if I moved default cube only on X axis and hit I, I am expecting that only X location gets keyframed, and not Y,Z and definately not RotScale 2. When auto-keying with "Only Insert Needed" and manually change value from properties or Item tab in sidebar its not keyframed. Is that a bug or expected? If I have loc+scale keyframes and I keep changing them it works fine, but for rotation, which is not keyframed yet, entering new value in the field doesnt work, but rotating with R in 3D viewport does keyframe it. (I checked, Only Insert Available isn't enabled) ------ UX feedback: 1. When Only Insert Needed is enabled for manual keying you get this error when hovering over property and hitting I `Failed to insert keys on F-Curve with path 'location[0]', ensure that it is not locked or sampled, and try removing F-Modifiers` This error while descriptive is very annoying, especially when I was animating fast it just kept popping in my face and spamming my Info/console. Also, why is that an error at all? When I enable it, or lock the channel I expect Blender to know that it was my conscious choice and I dont want to be asked about it every time. There should be no error or text popup at all in my opinion. 2. Descriptions when hovering on items in preferences is very basic and kinda just says same thing title does in different wording. I propose this titles: Only Insert Needed `Insert keyframes only for properties that were modified (i.e. they have different value than last keyframe)` Only Insert Available `Insert Keyframes only for properties that already have F-Curve (i.e. they were manually keyframed)`
Author
Member

I've tested this as well, functionality is very good. But UX has little issues.

Questions about functionality:

  1. When manual keying with "Only Insert Needed" and you hit I for the first time, meaning you have no prior keyframes, it still keyframes every transform on all three axis even if you modified only one. Is that expected? Because for example if I moved default cube only on X axis and hit I, I am expecting that only X location gets keyframed, and not Y,Z and definately not RotScale

That is a known limitation. Basically without a keyframe Blender doesn't know if anything has changed so the behavior is to add keys at first.

  1. When auto-keying with "Only Insert Needed" and manually change value from properties or Item tab in sidebar its not keyframed. Is that a bug or expected? If I have loc+scale keyframes and I keep changing them it works fine, but for rotation, which is not keyframed yet, entering new value in the field doesnt work, but rotating with R in 3D viewport does keyframe it. (I checked, Only Insert Available isn't enabled)

Hm this seems to have been the case since at least 3.3
Thanks for pointing that out, I wasn't aware that was the case. I'll put it on the list for a future PR.
The question is how this should behave. I imagine creating keyframes on every property you change can be quite annoying (thinking about modifying shader node values for example)

UX feedback:

  1. When Only Insert Needed is enabled for manual keying you get this error when hovering over property and hitting I

Failed to insert keys on F-Curve with path 'location[0]', ensure that it is not locked or sampled, and try removing F-Modifiers

This error while descriptive is very annoying, especially when I was animating fast it just kept popping in my face and spamming my Info/console. Also, why is that an error at all? When I enable it, or lock the channel I expect Blender to know that it was my conscious choice and I dont want to be asked about it every time. There should be no error or text popup at all in my opinion.

I agree on this. It would also greatly simplify the keyframing code if we simplified/removed the messages. But this has to be discussed with more people. Providing users with feedback on why something didn't work is a good thing in theory.

Edit: can you elaborate on that, I am not able to reproduce that with the current build

  1. Descriptions when hovering on items in preferences is very basic and kinda just says same thing title does in different wording. I propose this titles:

Only Insert Needed Insert keyframes only for properties that were modified (i.e. they have different value than last keyframe)
Only Insert Available Insert Keyframes only for properties that already have F-Curve (i.e. they were manually keyframed)

I will make a separate PR for those but I agree that this would need better descriptions.

> I've tested this as well, functionality is very good. But UX has little issues. > > Questions about functionality: > > 1. When manual keying with "Only Insert Needed" and you hit I for the first time, meaning you have no prior keyframes, it still keyframes every transform on all three axis even if you modified only one. Is that expected? Because for example if I moved default cube only on X axis and hit I, I am expecting that only X location gets keyframed, and not Y,Z and definately not RotScale That is a known limitation. Basically without a keyframe Blender doesn't know if anything has changed so the behavior is to add keys at first. > 2. When auto-keying with "Only Insert Needed" and manually change value from properties or Item tab in sidebar its not keyframed. Is that a bug or expected? If I have loc+scale keyframes and I keep changing them it works fine, but for rotation, which is not keyframed yet, entering new value in the field doesnt work, but rotating with R in 3D viewport does keyframe it. (I checked, Only Insert Available isn't enabled) Hm this seems to have been the case since at least 3.3 Thanks for pointing that out, I wasn't aware that was the case. I'll put it on the list for a future PR. The question is how this should behave. I imagine creating keyframes on every property you change can be quite annoying (thinking about modifying shader node values for example) > UX feedback: > > 1. When Only Insert Needed is enabled for manual keying you get this error when hovering over property and hitting I > > `Failed to insert keys on F-Curve with path 'location[0]', ensure that it is not locked or sampled, and try removing F-Modifiers` > > This error while descriptive is very annoying, especially when I was animating fast it just kept popping in my face and spamming my Info/console. Also, why is that an error at all? When I enable it, or lock the channel I expect Blender to know that it was my conscious choice and I dont want to be asked about it every time. There should be no error or text popup at all in my opinion. I agree on this. It would also greatly simplify the keyframing code if we simplified/removed the messages. But this has to be discussed with more people. Providing users with feedback on why something didn't work is a good thing in theory. Edit: can you elaborate on that, I am not able to reproduce that with the current build > 2. Descriptions when hovering on items in preferences is very basic and kinda just says same thing title does in different wording. I propose this titles: > > Only Insert Needed `Insert keyframes only for properties that were modified (i.e. they have different value than last keyframe)` > Only Insert Available `Insert Keyframes only for properties that already have F-Curve (i.e. they were manually keyframed)` I will make a separate PR for those but I agree that this would need better descriptions.
Contributor

The question is how this should behave. I imagine creating keyframes on every property you change can be quite annoying (thinking about modifying shader node values for example)

I think it should behave exactly like that, but Only Insert Available should be turned on by default to avoid that behavior. That way logic becomes clear and you always know whats gonna happen when you adjust the value.

Only Insert Available enabled - No autokey keyframes without existing F-curve
Only Insert Available disabled - Autokey keyframes everything.

Edit: can you elaborate on that, I am not able to reproduce that with the current build

To get an error:

  1. Enable Only Insert Available for Manual Keying
  2. Keyframe default cube location
  3. On different frame move default cube only on X axis
  4. Hover over the Location property in sidebar or Properties panel and press I

error also appears twice, once for each channel that you didnt modify. It makes it seem like you did something wrong, when in fact you're doing correct thing.

> The question is how this should behave. I imagine creating keyframes on every property you change can be quite annoying (thinking about modifying shader node values for example) I think it should behave exactly like that, but Only Insert Available should be turned on by default to avoid that behavior. That way logic becomes clear and you always know whats gonna happen when you adjust the value. Only Insert Available enabled - No autokey keyframes without existing F-curve Only Insert Available disabled - Autokey keyframes everything. > Edit: can you elaborate on that, I am not able to reproduce that with the current build To get an error: 1. Enable Only Insert Available for Manual Keying 2. Keyframe default cube location 3. On different frame move default cube only on X axis 4. Hover over the Location property in sidebar or Properties panel and press I error also appears twice, once for each channel that you didnt modify. It makes it seem like you did something wrong, when in fact you're doing correct thing.
Nathan Vegdahl requested changes 2024-01-18 13:06:40 +01:00
Nathan Vegdahl left a comment
Member

Generally looks good to me. Just a couple of confusing points left over from the old code that we can maybe also clean up here.

Also, seeing the [manual | auto] toggle buttons layout here, I'm now less sure if it's the way to go even though I suggested it. It think it's a bit challenging to lay these options out in a clear way.

In any case, I'm fine with this UI layout for this PR, because the main thing here is that the options have been split out appropriately between manual and auto. We can do a follow up PR to try to lay the options out well in the UI—I don't think it should be a blocker for this, since it probably involves the other options as well.

Generally looks good to me. Just a couple of confusing points left over from the old code that we can maybe also clean up here. Also, seeing the `[manual | auto]` toggle buttons layout here, I'm now less sure if it's the way to go even though I suggested it. It think it's a bit challenging to lay these options out in a clear way. In any case, I'm fine with this UI layout for this PR, because the main thing here is that the options have been split out appropriately between manual and auto. We can do a follow up PR to try to lay the options out well in the UI—I don't think it should be a blocker for this, since it probably involves the other options as well.
@ -136,3 +136,3 @@
/** Check if a flag is set for auto-key-framing (per scene takes precedence). */
bool is_autokey_flag(const Scene *scene, eKeyInsert_Flag flag);
bool is_autokey_flag(const Scene *scene, eKeying_Flag flag);
Member

Should we change this function's name to not reference autokeying? is_autokey_flag makes me think that it's specific to flags that only apply to autokeying. But reading further into the PR it looks like it's used in non-autokeying contexts for non-autokeying-specific flags as well.

But maybe I'm confused.

Should we change this function's name to not reference autokeying? `is_autokey_flag` makes me think that it's specific to flags that only apply to autokeying. But reading further into the PR it looks like it's used in non-autokeying contexts for non-autokeying-specific flags as well. But maybe I'm confused.
Author
Member

That's a good point. I've renamed the function to is_keying_flag
and moved the implementation to keyframing.cc

That's a good point. I've renamed the function to `is_keying_flag` and moved the implementation to `keyframing.cc`
nathanvegdahl marked this conversation as resolved
@ -1616,7 +1616,7 @@ typedef struct ToolSettings {
/* Auto-Keying Mode. */
Member

Should this comment change now that autokey_flag has been changed to keying_flag?

Should this comment change now that `autokey_flag` has been changed to `keying_flag`?
Author
Member

Yep, changed

Yep, changed
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld added 3 commits 2024-01-18 16:43:14 +01:00
Christoph Lendenfeld added 1 commit 2024-01-18 16:47:42 +01:00
Author
Member

@nathanvegdahl I think the GUI will make more sense once we add "Only Insert Available" for manual keying as well

@nathanvegdahl I think the GUI will make more sense once we add "Only Insert Available" for manual keying as well
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-01-18 16:50:27 +01:00
Nathan Vegdahl requested changes 2024-01-19 11:18:43 +01:00
Nathan Vegdahl left a comment
Member

Found one more thing on my second time through (mainly just want to confirm whether it's intentional or not).

Found one more thing on my second time through (mainly just want to confirm whether it's intentional or not).
@ -959,2 +959,2 @@
/** Flags for autokeying. */
short autokey_flag;
/** Flags for inserting keyframes. */
short keying_flag;
Member

If I understand DNA correctly, this rename means (from DNA's perspective) that autokey_flag has disappeared and a separate unrelated field keying_flag has been added. Which means the flags in autokey_flag from old files will simply be discarded by DNA on load.

That's totally fine if we're okay with the flags from older files getting lost. But if we want to migrate them over we either need to:

  • Keep autokey_flag alongside keying_flag, and manually copy the flags over in the versioning code, OR
  • Add the rename to dna_rename_defs.h so that DNA knows it's a rename, and thus will copy the data over for us.
If I understand DNA correctly, this rename means (from DNA's perspective) that `autokey_flag` has disappeared and a separate unrelated field `keying_flag` has been added. Which means the flags in `autokey_flag` from old files will simply be discarded by DNA on load. That's totally fine if we're okay with the flags from older files getting lost. But if we want to migrate them over we either need to: - Keep `autokey_flag` alongside `keying_flag`, and manually copy the flags over in the versioning code, OR - Add the rename to dna_rename_defs.h so that DNA knows it's a rename, and thus will copy the data over for us.
Author
Member

thanks for pointing that out. Wasn't aware such a file existed
tested it now and the user preferences seem to version up properly

thanks for pointing that out. Wasn't aware such a file existed tested it now and the user preferences seem to version up properly
Member

Yeah, I also wasn't aware of it until relatively recently, when working on the bone collections code. Lots of hidden things in Blender like this...

Yeah, I also wasn't aware of it until relatively recently, when working on the bone collections code. Lots of hidden things in Blender like this...
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-01-19 12:14:12 +01:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-01-19 12:14:54 +01:00
Nathan Vegdahl approved these changes 2024-01-19 12:23:45 +01:00
Nathan Vegdahl left a comment
Member

Looks good to me!

Looks good to me!
Author
Member

the outstanding issues raised by artists are not caused by this PR, so I will go ahead and land this.
Raised issues will be noted and fixed in upcoming PRs

the outstanding issues raised by artists are not caused by this PR, so I will go ahead and land this. Raised issues will be noted and fixed in upcoming PRs
Christoph Lendenfeld added 2 commits 2024-01-19 16:13:06 +01:00
Christoph Lendenfeld merged commit 5e28601d69 into main 2024-01-19 16:26:19 +01:00
Christoph Lendenfeld deleted branch autokey_separate_flags 2024-01-19 16:26:21 +01:00
Sign in to join this conversation.
No reviewers
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
5 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#115525
No description provided.