Anim: Deselect Keys before inserting new keys #121908

Merged
Christoph Lendenfeld merged 43 commits from ChrisLend/blender:deselect_keys_on_creation into main 2024-07-18 14:48:20 +02:00

This PR aims to solve the animator request to deselect keyframes when inserting new keys.
This has been discussed in the Animation & Rigging module meeting.
There is also an RCS post about that.
Doing this brings key creation in line with object creation, where only the newly created object is selected.
There has been a previous attempt to do a similar thing.

Changes

When inserting keys by pressing I in the viewport or choosing a keying set, all keys of the Action get deselected before inserting new keys.
New keys are selected by default.
Python RNA functions are NOT affected, meaning addons using those functions will not deselect any keys by default. The developer has to choose to do so.

TODO

  • Check autokeying works with Objects
  • Check autokeying works with Bones
  • Make it work in Dope Sheet and Graph Editor
  • Support for pose library operations
  • explore how to integrate that with Python
  • de-duplicate code
  • Cover with unit tests

Test Build

https://builder.blender.org/download/patch/PR121908/

Insights

  • Deselection happens outside of the key insertion core.
    • IMO this keeps it more flexible and doesn't burden the key insertion code with the knowledge of deselection.
    • It is needed for the case of having multiple things selected when their keys go to the same Action. Since the insertion code only deals with a single thing, having multiple selection would result in the selection of only the last selected in the list.
    • Python RNA functions don't change behavior, there needs to be an easy way to deselect things in Python though.
  • When multiple IDs reference the same Action, all keys get deselected regardless. With the new layered action this might not be something we want. We could only deselect keys of the ID for which keys are inserted?
  • Auto - Record is an edge case. Since you are in essence creating a bunch of keys in one go, I kept the selected for those.
  • With "Only Insert Needed" enabled, only the keys changed by the recent operation are selected. E.g. translate, then rotate will leave only the rotation keys selected.
  • Since interacting with RNA functions does NOT deselect other keys automatically, there is now a function on the action to do that. action.deselect_keys(). The developer can choose at which point this should be called.

Open Questions

  • Should this be a user preference? We need to reach out to a lot of users to find out if anyone has a strong need for the old behavior.
  • Should this behavior extend to Grease Pencil?
  • This does not yet change the behavior of CLIP_OT_keyframe_insert. Would need the opinion of someone familiar with that area to tell me if that needs changing
  • The autokeying behavior follows the behavior of regular keyframing. There is also an argument that it should NOT influence key selection at all. Which is better?

User Feedback

This PR aims to solve the animator request to deselect keyframes when inserting new keys. This has been discussed in the [Animation & Rigging](https://devtalk.blender.org/t/2024-05-02-animation-rigging-module-meeting/34493#patches-review-decision-time-5) module meeting. There is also an [RCS post](https://blender.community/c/rightclickselect/K0hbbc/?sorting=hot) about that. Doing this brings key creation in line with object creation, where only the newly created object is selected. There has been a [previous attempt](https://archive.blender.org/developer/D11623) to do a similar thing. ### Changes When inserting keys by pressing `I` in the viewport or choosing a keying set, all keys of the `Action` get deselected before inserting new keys. New keys are selected by default. Python RNA functions are **NOT** affected, meaning addons using those functions will not deselect any keys by default. The developer has to choose to do so. ### TODO * [x] Check autokeying works with Objects * [x] Check autokeying works with Bones * [x] Make it work in Dope Sheet and Graph Editor * [x] Support for pose library operations * [x] explore how to integrate that with Python * [x] de-duplicate code * [x] Cover with unit tests ### Test Build https://builder.blender.org/download/patch/PR121908/ ### Insights * Deselection happens outside of the key insertion core. * IMO this keeps it more flexible and doesn't burden the key insertion code with the knowledge of deselection. * It is needed for the case of having multiple things selected when their keys go to the same `Action`. Since the insertion code only deals with a single thing, having multiple selection would result in the selection of only the last selected in the list. * Python RNA functions don't change behavior, there needs to be an easy way to deselect things in Python though. * When multiple IDs reference the same `Action`, all keys get deselected regardless. With the new layered action this might not be something we want. We could only deselect keys of the ID for which keys are inserted? * Auto - Record is an edge case. Since you are in essence creating a bunch of keys in one go, I kept the selected for those. * With "Only Insert Needed" enabled, only the keys changed by the recent operation are selected. E.g. translate, then rotate will leave only the rotation keys selected. * Since interacting with RNA functions does NOT deselect other keys automatically, there is now a function on the action to do that. `action.deselect_keys()`. The developer can choose at which point this should be called. ### Open Questions * Should this be a user preference? We need to reach out to a lot of users to find out if anyone has a strong need for the old behavior. * Should this behavior extend to Grease Pencil? * This does not yet change the behavior of `CLIP_OT_keyframe_insert`. Would need the opinion of someone familiar with that area to tell me if that needs changing * The autokeying behavior follows the behavior of regular keyframing. There is also an argument that it should NOT influence key selection at all. Which is better? ---- User Feedback * https://devtalk.blender.org/t/2024-05-02-animation-rigging-module-meeting/34493/10
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2024-05-17 12:38:33 +02:00
Christoph Lendenfeld added 1 commit 2024-05-17 12:38:45 +02:00
Christoph Lendenfeld added 1 commit 2024-05-17 14:51:38 +02:00
Christoph Lendenfeld added 1 commit 2024-05-17 15:00:32 +02:00
Christoph Lendenfeld added 1 commit 2024-05-17 17:02:50 +02:00
Member

The main issue with inserted keys auto-selection is that if you are using a keying set like whole character or whole character (selected bones only ) you will easily end up in a situation where some channels are not showing in the dope-sheet or graph-editor depending on your view settings. Usually only show selected is on by default and custom properties may not appear in this selection state (i.e. rigify puts the properties on the gear at each limbs, but displays it at each limb bone). This will easily result in keyes that are not easily de-selectable. So imo having at least an option for disabling the auto-select behaviour is beneficial to most of the character animation workflows i am aware of. I'd put this as a flag in the dopesheet, near to the only show selected toggle if possible.

But the above issue could also be resolved by an upgrade of the select/de-select all operator by taking in account not only visible keys but all-keys upon deselection, which is imo the correct behaviour. In fact by using the alt-a command, as a new user, i'd never understand why some keys stays yellow if i am unselecting all.

The main issue with inserted keys auto-selection is that if you are using a keying set like `whole character` or `whole character (selected bones only )` you will easily end up in a situation where some channels are not showing in the dope-sheet or graph-editor depending on your view settings. Usually `only show selected` is on by default and custom properties may not appear in this selection state (i.e. rigify puts the properties on the gear at each limbs, but displays it at each limb bone). This will easily result in keyes that are not easily de-selectable. So imo having at least an option for disabling the auto-select behaviour is beneficial to most of the character animation workflows i am aware of. I'd put this as a flag in the dopesheet, near to the `only show selected` toggle if possible. But the above issue could also be resolved by an upgrade of the select/de-select all operator by taking in account not only visible keys but all-keys upon deselection, which is imo the correct behaviour. In fact by using the `alt-a` command, as a new user, i'd never understand why some keys stays yellow if i am unselecting all.
Christoph Lendenfeld added 2 commits 2024-05-21 10:13:05 +02:00
support bone auto keying
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
d6eae19523
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/PR121908) when ready.
Christoph Lendenfeld added 1 commit 2024-05-21 10:37:01 +02:00

Tested it today creating a quick animation, feels very natural 9/10 times a day you wanna edit keys you just created, to me it works well so far!

Tested it today creating a quick animation, feels very natural 9/10 times a day you wanna edit keys you just created, to me it works well so far!
Christoph Lendenfeld added 1 commit 2024-05-23 10:51:50 +02:00
Christoph Lendenfeld added 1 commit 2024-05-23 11:14:54 +02:00
Christoph Lendenfeld added 2 commits 2024-05-23 11:51:08 +02:00
Christoph Lendenfeld added 3 commits 2024-05-23 13:53:25 +02:00
Christoph Lendenfeld added 1 commit 2024-05-23 13:56:17 +02:00
Christoph Lendenfeld changed title from WIP: Anim: Deselect Keys before inserting new keys to Anim: Deselect Keys before inserting new keys 2024-05-23 15:08:48 +02:00

The main issue with inserted keys auto-selection is that if you are using a keying set like whole character or whole character (selected bones only ) you will easily end up in a situation where some channels are not showing in the dope-sheet or graph-editor depending on your view settings. Usually only show selected is on by default and custom properties may not appear in this selection state (i.e. rigify puts the properties on the gear at each limbs, but displays it at each limb bone). This will easily result in keyes that are not easily de-selectable. So imo having at least an option for disabling the auto-select behaviour is beneficial to most of the character animation workflows i am aware of. I'd put this as a flag in the dopesheet, near to the only show selected toggle if possible.

I think you're mixing up the selection of the objects/bones being animated, and the selection of the keys themselves. This PR is about the latter.

But the above issue could also be resolved by an upgrade of the select/de-select all operator by taking in account not only visible keys but all-keys upon deselection, which is imo the correct behaviour.

Hidden keys are already considered deselected.

In fact by using the alt-a command, as a new user, i'd never understand why some keys stays yellow if i am unselecting all.

This depends on where your mouse is when you press Alt+A. Above the channel list: deselects all channels. Above the keys: deselects the keys. In the 3D Viewport: deselects the objects/bones/vertices/whatever. If you're deselecting the keys, and Alt+A doesn't deselect all of them, please write a bug report about this (if you can do this consistently).

> The main issue with inserted keys auto-selection is that if you are using a keying set like `whole character` or `whole character (selected bones only )` you will easily end up in a situation where some channels are not showing in the dope-sheet or graph-editor depending on your view settings. Usually `only show selected` is on by default and custom properties may not appear in this selection state (i.e. rigify puts the properties on the gear at each limbs, but displays it at each limb bone). This will easily result in keyes that are not easily de-selectable. So imo having at least an option for disabling the auto-select behaviour is beneficial to most of the character animation workflows i am aware of. I'd put this as a flag in the dopesheet, near to the `only show selected` toggle if possible. I think you're mixing up the selection of the objects/bones being animated, and the selection of the keys themselves. This PR is about the latter. > But the above issue could also be resolved by an upgrade of the select/de-select all operator by taking in account not only visible keys but all-keys upon deselection, which is imo the correct behaviour. Hidden keys are already considered deselected. > In fact by using the `alt-a` command, as a new user, i'd never understand why some keys stays yellow if i am unselecting all. This depends on where your mouse is when you press Alt+A. Above the channel list: deselects all channels. Above the keys: deselects the keys. In the 3D Viewport: deselects the objects/bones/vertices/whatever. If you're deselecting the keys, and Alt+A doesn't deselect all of them, please write a bug report about this (if you can do this consistently).
Sybren A. Stüvel requested changes 2024-06-03 14:01:07 +02:00
Dismissed
@ -269,2 +269,4 @@
const AnimationEvalContext &anim_eval_context);
/**
* Deselect all keys within the actions the given objects hold.

"the actions the given objects hold" → "of the actions referenced by these objects"

"the actions the given objects hold" → "of the actions referenced by these objects"
ChrisLend marked this conversation as resolved
@ -105,0 +132,4 @@
if (!ch_bag) {
continue;
}
for (int i = 0; i < ch_bag->fcurve_array_num; i++) {

This could loop over for (FCurve *fcu : ch_bag->fcurves()) as you don't need to have the index anyway.

This could loop over `for (FCurve *fcu : ch_bag->fcurves())` as you don't need to have the index anyway.
ChrisLend marked this conversation as resolved
@ -105,0 +134,4 @@
}
for (int i = 0; i < ch_bag->fcurve_array_num; i++) {
FCurve *fcu = ch_bag->fcurve_array[i];
for (int j = 0; j < fcu->totvert; j++) {

I think deselecting all keys in an FCurve should be a function of the FCurve itself, and not of the Action. Could sit next to BKE_fcurve_has_selected_control_points() (and also I wonder why there it's suddently "control points", but that's beside the point).

I think deselecting all keys in an FCurve should be a function of the FCurve itself, and not of the Action. Could sit next to `BKE_fcurve_has_selected_control_points()` (and also I wonder why there it's suddently "control points", but that's beside the point).
ChrisLend marked this conversation as resolved
@ -105,0 +145,4 @@
void Action::deselect_keys()
{
if (this->is_empty()) {

I don't think the separation between empty / legacy / layered is strictly necessary here. Either the legacy or the layers list (or both) is going to be empty anyway.

I don't think the separation between empty / legacy / layered is strictly necessary here. Either the legacy or the layers list (or both) is going to be empty anyway.
ChrisLend marked this conversation as resolved
@ -1030,1 +1032,4 @@
void deselect_action_keys(Span<Object *> objects)
{
Map<bAction *, bool> deselected_actions;

Use a Set<bAction *> instead, as that's the data model you're actually implementing here. Same with the similar map in ANIM_animdata_deselect_action_keys.

This logic of constructing a set to avoid repeated deselections on the same Action is itself repeated quite a bit in the code. I think it would be nicer to have:

  • one function that takes a Span<Action *>, which then calls action.deselect_keys(); on unique Actions, and
  • the other functions just construct a vector (or whatever) of Actions and call that function.

That way the "find the Actions to deselect" and "ensure deselecting only happens once per Action" code is decoupled.

Use a `Set<bAction *>` instead, as that's the data model you're actually implementing here. Same with the similar map in `ANIM_animdata_deselect_action_keys`. This logic of constructing a set to avoid repeated deselections on the same Action is itself repeated quite a bit in the code. I think it would be nicer to have: - one function that takes a `Span<Action *>`, which then calls `action.deselect_keys();` on unique Actions, and - the other functions just construct a vector (or whatever) of Actions and call that function. That way the "find the Actions to deselect" and "ensure deselecting only happens once per Action" code is decoupled.
Author
Member

split out those functions and used a Span
good to know such a thing exists :)

split out those functions and used a `Span` good to know such a thing exists :)
ChrisLend marked this conversation as resolved
@ -1857,2 +1864,4 @@
RNA_def_property_clear_flag(prop, PROP_EDITABLE);
FunctionRNA *func = RNA_def_function(srna, "deselect_keys", "rna_Action_deselect_keys");
RNA_def_function_ui_description(func, "Deselects all keys of the Action");

Maybe it's unnecessary, but I think it would be nice to have a 2nd sentence here that explains what this function does not do: "The selection status of F-Curves themselves is unchanged" -- or something along those lines.

Maybe it's unnecessary, but I think it would be nice to have a 2nd sentence here that explains what this function does _not_ do: "The selection status of F-Curves themselves is unchanged" -- or something along those lines.
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 1 commit 2024-06-11 15:05:12 +02:00
Christoph Lendenfeld added 1 commit 2024-06-11 15:52:03 +02:00
Christoph Lendenfeld added 1 commit 2024-06-11 16:03:21 +02:00
Christoph Lendenfeld added 2 commits 2024-06-18 15:58:13 +02:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-18 15:58:22 +02:00
Sybren A. Stüvel reviewed 2024-06-19 10:54:53 +02:00
@ -294,0 +294,4 @@
/**
* Deselect all keys of the actions referenced by these objects.
*/
void deselect_action_keys(Span<Object *> objects);

I think some differentiation in the names would be nice, to avoid confusion with deselect_action_keys(Span<bAction *>). How about deselect_keys_actions(actions) vs. deselect_keys_assigned_actions(objects)?

I think some differentiation in the names would be nice, to avoid confusion with `deselect_action_keys(Span<bAction *>)`. How about `deselect_keys_actions(actions)` vs. `deselect_keys_assigned_actions(objects)`?
Author
Member

Since those functions are now together in a file they are named

  • deselect_keys_actions
  • deselect_keys_assigned_actions
  • action_deselect_keys

I hope it's clear enough from the name what they do

Since those functions are now together in a file they are named * deselect_keys_actions * deselect_keys_assigned_actions * action_deselect_keys I hope it's clear enough from the name what they do
ChrisLend marked this conversation as resolved
@ -1185,0 +1198,4 @@
{
BLI_assert(action.is_action_layered());
for (Layer *layer : action.layers()) {
if (!layer) {

Layers shouldn't be nullptr, so BLI_assert(layer) would be better, but you can also just let Blender crash in that case due to a nullptr dereference.

Layers shouldn't be `nullptr`, so `BLI_assert(layer)` would be better, but you can also just let Blender crash in that case due to a nullptr dereference.
ChrisLend marked this conversation as resolved
@ -1185,0 +1202,4 @@
continue;
}
for (Strip *strip : layer->strips()) {
if (!strip || !strip->is<KeyframeStrip>()) {

Same here, no need to check for null-ness.

Same here, no need to check for null-ness.
ChrisLend marked this conversation as resolved
@ -1185,0 +1207,4 @@
}
KeyframeStrip &key_strip = strip->as<KeyframeStrip>();
for (ChannelBag *ch_bag : key_strip.channelbags()) {
if (!ch_bag) {

And here too, null shouldn't exist here.

And here too, null shouldn't exist here.
ChrisLend marked this conversation as resolved
@ -1185,0 +1218,4 @@
}
}
void Action::deselect_keys()

I was already pondering whether things like Action::deselect_keys() should actually be a method on Action, or whether it should be something "outside" the class. It's something I often think about because it's not always a simple question to answer, whether something should be part of the class or not.

Looking at the code now, it might be clearer to move the entire "deselect keys" logic out of the Action class. Currently the code flows in & out of that class:

  1. outside: deselect_action_keys()
  2. inside: Action::deselect_keys()
  3. outside: clear_selection_layered_action / clear_selection_legacy_action

How would you feel about moving the entire key selection manipulation code into a file of its own, like action_selection.cc?

Eventually I want to add some nice iterators as well, so that the code here could just use an AllFCurvesIterator to get the F-Curves, rather than being dependent on the actual structure of the Action class itself.

I was already pondering whether things like `Action::deselect_keys()` should actually be a method on `Action`, or whether it should be something "outside" the class. It's something I often think about because it's not always a simple question to answer, whether something should be part of the class or not. Looking at the code now, it might be clearer to move the entire "deselect keys" logic out of the `Action` class. Currently the code flows in & out of that class: 1. outside: `deselect_action_keys()` 2. inside: `Action::deselect_keys()` 3. outside: `clear_selection_layered_action` / `clear_selection_legacy_action` How would you feel about moving the entire key selection manipulation code into a file of its own, like `action_selection.cc`? Eventually I want to add some nice iterators as well, so that the code here could just use an `AllFCurvesIterator` to get the F-Curves, rather than being dependent on the actual structure of the `Action` class itself.
Author
Member

good point, I'll make a action_selection.cc file

good point, I'll make a `action_selection.cc` file
ChrisLend marked this conversation as resolved
@ -1096,4 +1098,18 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
return combined_result;
}
void deselect_action_keys(Span<Object *> objects)

I think this should sit next to its sibling function, so that when you find one, you've found both. Otherwise it's too easy to think the other one doesn't exist.

I think this should sit next to its sibling function, so that when you find one, you've found both. Otherwise it's too easy to think the other one doesn't exist.
Author
Member

this is now in the new action_selection.cc file

this is now in the new `action_selection.cc` file
ChrisLend marked this conversation as resolved
@ -462,0 +466,4 @@
{
using namespace blender;
Vector<bAction *> actions;
LISTBASE_FOREACH (bAnimListElem *, ale, anim_data) {

There is a fair chance that there could be a lot of bAnimListElems and a low number of Actions. I think it makes sense here to either use a Set<bAction *> or use Vector::append_non_duplicates().

There is a fair chance that there could be a lot of `bAnimListElem`s and a low number of `Action`s. I think it makes sense here to either use a `Set<bAction *>` or use `Vector::append_non_duplicates()`.
Author
Member

used a Set and iterated over it directly in that function

used a `Set` and iterated over it directly in that function
ChrisLend marked this conversation as resolved
Sybren A. Stüvel requested changes 2024-06-19 17:44:54 +02:00
Dismissed
Sybren A. Stüvel left a comment
Member

.

.
Christoph Lendenfeld added 3 commits 2024-06-20 11:01:13 +02:00
Christoph Lendenfeld added 1 commit 2024-06-20 11:15:17 +02:00
Christoph Lendenfeld added 1 commit 2024-06-20 11:19:14 +02:00
use set in anim list elem code
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
ec36b401d4
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-20 11:26:05 +02:00
Author
Member

@blender-bot build

@blender-bot build
Iliya Katushenock reviewed 2024-06-20 11:34:07 +02:00
@ -0,0 +58,4 @@
{
Set<bAction *> visited_actions;
for (bAction *action : actions) {
if (visited_actions.contains(action)) {

You can write this as if (visited_actions.add(action)) to know if this addition was successful (there was not such key in the set before).

You can write this as `if (visited_actions.add(action))` to know if this addition was successful (there was not such key in the set before).
mod_moder marked this conversation as resolved
Sybren A. Stüvel approved these changes 2024-06-20 15:04:59 +02:00
Dismissed
Sybren A. Stüvel left a comment
Member

Nice work!

Just one note, related to a recent commit of mine. Choice is yours :)

Nice work! Just one note, related to a recent commit of mine. Choice is yours :)
@ -1182,4 +1182,5 @@ FCurve *action_fcurve_ensure(Main *bmain,
return fcu;
}

Before landing, remove this change so that the file isn't touched.

Before landing, remove this change so that the file isn't touched.
@ -0,0 +35,4 @@
KeyframeStrip &key_strip = strip->as<KeyframeStrip>();
for (ChannelBag *ch_bag : key_strip.channelbags()) {
BLI_assert(ch_bag != nullptr);
for (FCurve *fcu : ch_bag->fcurves()) {

Since aa0e5368e7 this could use the brand new fcurves_all(action), although that would need to get a non-const variant for this purpose. I'll leave it up to you to decide whether to make that non-const variant, or keep the code as-is for now.

Since aa0e5368e7b this could use the brand new `fcurves_all(action)`, although that would need to get a non-const variant for this purpose. I'll leave it up to you to decide whether to make that non-const variant, or keep the code as-is for now.
Author
Member

decided to use fcurves_all because that removes the need for the two static functions to handle legacy/layered actions

decided to use `fcurves_all` because that removes the need for the two static functions to handle legacy/layered actions
@ -1041,6 +1041,14 @@ bool BKE_fcurve_has_selected_control_points(const FCurve *fcu)
return false;
}
void BKE_fcurve_deselect_all_keys(FCurve *fcu)

Change the parameter to FCurve &fcurve), to indicate this is not null-safe.

Change the parameter to `FCurve &fcurve)`, to indicate this is not null-safe.

Here also, the FCurve ref shouldn't be const, as you're changing the data. I'm surprised the compiler even lets you do this 🙈

Here also, the FCurve ref shouldn't be `const`, as you're changing the data. I'm surprised the compiler even lets you do this 🙈
dr.sybren marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-06-20 15:52:38 +02:00
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel reviewed 2024-06-20 16:04:37 +02:00
@ -680,0 +690,4 @@
/**
* Deselect all keys within the action.
*/
void action_deselect_keys(const Action &action);

This shouldn't use a const Action &, as it's changing the data in the Action.

This shouldn't use a `const Action &`, as it's changing the data in the Action.
dr.sybren marked this conversation as resolved
Author
Member

@dr.sybren about const
If something is const means that I also cannot mutate data that the thing contains via pointers, then I cannot use fcurves_all because that returns const FCurve *
How should I go about that

edit: ignore me, just now saw that there is a non-const version of that function
edit of the edit: new day fresh brain, looked at the wrong function. fcurves_all has no non-const version

@dr.sybren about `const` If something is `const` means that I also cannot mutate data that the thing contains via pointers, then I cannot use `fcurves_all` because that returns `const FCurve *` How should I go about that edit: ignore me, just now saw that there is a non-const version of that function edit of the edit: new day fresh brain, looked at the wrong function. `fcurves_all` has no non-const version
Christoph Lendenfeld force-pushed deselect_keys_on_creation from d0019be3cf to 45ac4e2457 2024-06-21 09:48:56 +02:00 Compare
Author
Member

I reverted the last commit that introduced the const and used fcurves_all.
instead I used fcurves_for_animation for which a non-const version exists.

I reverted the last commit that introduced the `const` and used `fcurves_all`. instead I used `fcurves_for_animation` for which a non-const version exists.
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-25 12:19:39 +02:00
Sybren A. Stüvel requested changes 2024-06-25 14:32:14 +02:00
Dismissed
Sybren A. Stüvel left a comment
Member

@dr.sybren about const [...]
edit of the edit: new day fresh brain, looked at the wrong function. fcurves_all has no non-const version

Yeah, hence the "although that would need to get a non-const variant for this purpose" earlier ;-)
So now there is e067b11c3c :)

The current approach of using fcurves_for_animation() loops over all layers and all strips, for every binding. Now it doesn't matter because there's max one layer and one strip anyway, but the non-const fcurves_all() will be more future-proof.

> @dr.sybren about `const` [...] > edit of the edit: new day fresh brain, looked at the wrong function. `fcurves_all` has no non-const version Yeah, hence the "although that would need to get a non-const variant for this purpose" earlier ;-) So now there is e067b11c3ce0244fca417ebb997c659c8e62ab0b :) The current approach of using `fcurves_for_animation()` loops over all layers and all strips, for every binding. Now it doesn't matter because there's max one layer and one strip anyway, but the non-const `fcurves_all()` will be more future-proof.
Christoph Lendenfeld added 2 commits 2024-06-25 17:12:21 +02:00
use brand new function to get non const FCurve
All checks were successful
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
aaac4c771a
Author
Member

@blender-bot build

@blender-bot build
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-06-25 17:18:13 +02:00
Sybren A. Stüvel requested changes 2024-06-26 13:04:55 +02:00
Sybren A. Stüvel left a comment
Member

Code-wise things look pretty good. I managed to find a case that doesn't work well yet, though: when the Dope Sheet has "Only Show Selected" disabled. In that case, it's not enough to depend on the get_selection() function, as the Dope Sheet will show more than the data in just the Actions of the selected objects.

You might be able to use the animation filtering code, probably with the flags (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_CHANNELS | ANIMFILTER_CURVE_VISIBLE | ANIMFILTER_FCURVESONLY | ANIMFILTER_NODUPLIS).

Code-wise things look pretty good. I managed to find a case that doesn't work well yet, though: when the Dope Sheet has "Only Show Selected" disabled. In that case, it's not enough to depend on the `get_selection()` function, as the Dope Sheet will show more than the data in just the Actions of the selected objects. You might be able to use the animation filtering code, probably with the flags `(ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_CHANNELS | ANIMFILTER_CURVE_VISIBLE | ANIMFILTER_FCURVESONLY | ANIMFILTER_NODUPLIS)`.
Christoph Lendenfeld added 1 commit 2024-07-02 13:26:06 +02:00
Christoph Lendenfeld added 1 commit 2024-07-02 13:44:04 +02:00
Author
Member

@dr.sybren so when inserting a key in an animation editor, all keys get correctly deselected, even with "Only Show Selected" disabled.
It's only when keying happens outside those editors that it is an issue (and I noticed when keying from the n-panel it doesn't work at all yet)

In order to apply animation filtering code I need to have an bAnimContext. For that to work the context needs to be in an animation editor. I just wanted to confirm with you that iterating through all the areas and setting a temp context to that is a valid option. I can use a similar logic as in a91a8f3fed but I wanted to confirm with you because that feels very hacky

@dr.sybren so when inserting a key in an animation editor, all keys get correctly deselected, even with "Only Show Selected" disabled. It's only when keying happens outside those editors that it is an issue (and I noticed when keying from the n-panel it doesn't work at all yet) In order to apply animation filtering code I need to have an `bAnimContext`. For that to work the context needs to be in an animation editor. I just wanted to confirm with you that iterating through all the areas and setting a temp context to that is a valid option. I can use a similar logic as in a91a8f3fed but I wanted to confirm with you because that feels very hacky
Christoph Lendenfeld added 1 commit 2024-07-02 14:18:05 +02:00

It's hacky indeed, but I think it's the only way to make this really work well. I think you'd have to even iterate over all areas to find the Dope Sheet or Graph Editor that shows the most data, so that it still works properly when you have "Only Show Selected" on in one, but off in the other. So maybe the code would have to run multiple times, once for each editor, because they may be showing different anim data altogether. For example, the Action editor could be showing all bindings in the Action (showing bindings that the Graph editor does not), and the Graph editor could be showing all objects (which the Action editor does not). So it becomes more like a "deselect all visible keys", for which it becomes a bit more natural that you loop over all editors to do so.

It's hacky indeed, but I think it's the only way to make this really work well. I think you'd have to even iterate over all areas to find the Dope Sheet or Graph Editor that shows the most data, so that it still works properly when you have "Only Show Selected" on in one, but off in the other. So maybe the code would have to run multiple times, once for each editor, because they may be showing different anim data altogether. For example, the Action editor could be showing all bindings in the Action (showing bindings that the Graph editor does not), and the Graph editor could be showing all objects (which the Action editor does not). So it becomes more like a "deselect all visible keys", for which it becomes a bit more natural that you loop over all editors to do so.
Christoph Lendenfeld added 1 commit 2024-07-11 11:40:15 +02:00
Christoph Lendenfeld added 1 commit 2024-07-11 15:11:55 +02:00
Christoph Lendenfeld added 1 commit 2024-07-11 15:16:26 +02:00
Christoph Lendenfeld added 1 commit 2024-07-11 15:25:20 +02:00
Christoph Lendenfeld added 1 commit 2024-07-11 15:36:02 +02:00
Christoph Lendenfeld added 1 commit 2024-07-11 15:39:24 +02:00
Merge branch 'main' into deselect_keys_on_creation
All checks were successful
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
367a5b94f6
Author
Member

@blender-bot package

The changes to deselect visible keys in all editors are now in.
@dr.sybren as a result, the function to deselect keys based on a span of objects was no longer used, so I removed it.
I did a quick performance check in my heavy file and it doesn't feel slower than before, but I haven't done a measurement yet.

@blender-bot package The changes to deselect visible keys in all editors are now in. @dr.sybren as a result, the function to deselect keys based on a span of objects was no longer used, so I removed it. I did a quick performance check in my heavy file and it doesn't feel slower than before, but I haven't done a measurement yet.
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR121908) when ready.
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-07-11 15:46:25 +02:00
Sybren A. Stüvel approved these changes 2024-07-15 10:52:54 +02:00
Sybren A. Stüvel left a comment
Member

LGTM, thanks!

Just one note about a commented-out line of code that got left in there.

LGTM, thanks! Just one note about a commented-out line of code that got left in there.
@ -466,2 +470,4 @@
static int keyframe_insert_with_keyingset_exec(bContext *C, wmOperator *op)
{
ANIM_deselect_keys_in_animation_editors(C);
// deselect_keys_of_selection(C);

This line can be removed.

This line can be removed.
@ -1939,1 +1945,4 @@
FunctionRNA *func = RNA_def_function(srna, "deselect_keys", "rna_Action_deselect_keys");
RNA_def_function_ui_description(
func, "Deselects all keys of the Action. The selection status of F-Curves is unchanged");

💜 for the note about the selection state of F-Curves.

💜 for the note about the selection state of F-Curves.
Christoph Lendenfeld added 2 commits 2024-07-18 14:32:26 +02:00
Christoph Lendenfeld merged commit 6ef77a0d22 into main 2024-07-18 14:48:20 +02:00
Christoph Lendenfeld deleted branch deselect_keys_on_creation 2024-07-18 14:48:24 +02:00
Sign in to join this conversation.
No reviewers
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
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
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
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
6 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#121908
No description provided.