Anim: Separate keying flags #115525
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#115525
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ChrisLend/blender:autokey_separate_flags"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
User facing changes
Splits the flag
..._FLAG_INSERTNEEDED
between autokey andmanual 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
![image](/blender/blender/attachments/73ae4d8e-d9fa-4778-9035-697d3515d537)
have been reordered
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 usedfor autokeying, rename it
keying_flag
.Fixes: #73773
Part of: #113278: Anim: Keyframing Rework
WIP: Anim: Separate keying flagsto Anim: Separate keying flags@blender-bot package
Package build started. Download here when ready.
After testing the functionality of the new 'only insert needed' implementation, I stumbled on the following issues:
@blender-bot package
Package build started. Download here when ready.
@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
I've tested this as well, functionality is very good. But UX has little issues.
Questions about functionality:
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
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:
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.
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)
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.
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)
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
I will make a separate PR for those but I agree that this would need better descriptions.
Christoph Lendenfeld referenced this pull request2024-01-18 11:10:41 +01:00
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.
To get an error:
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.
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);
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.
That's a good point. I've renamed the function to
is_keying_flag
and moved the implementation to
keyframing.cc
@ -1616,7 +1616,7 @@ typedef struct ToolSettings {
/* Auto-Keying Mode. */
Should this comment change now that
autokey_flag
has been changed tokeying_flag
?Yep, changed
@nathanvegdahl I think the GUI will make more sense once we add "Only Insert Available" for manual keying as well
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;
If I understand DNA correctly, this rename means (from DNA's perspective) that
autokey_flag
has disappeared and a separate unrelated fieldkeying_flag
has been added. Which means the flags inautokey_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:
autokey_flag
alongsidekeying_flag
, and manually copy the flags over in the versioning code, ORthanks for pointing that out. Wasn't aware such a file existed
tested it now and the user preferences seem to version up properly
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...
Looks good to me!
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