Refactor: use insert_key_rna() for button keyframing #121491

Nathan Vegdahl · 2024-05-06T17:55:36+02:00

Nathan Vegdahl commented

2024-05-06 17:55:36 +02:00

This commit does two things:

Switches manual (not auto) keyframing of buttons to use
insert_key_rna() instead of insert_keyframe().
Modifies insert_key_rna() to accept and understand rna paths that
specify an array element (for array properties), in order to make
the above possible.

This is part of a progressive refactor that is unifying the keyframing
code to use the same keyframing functions (in particular converging
on insert_key_rna()), rather than many different functions with
subtly different behavior.

Notes for reviewers:

@ChrisLend and I already discussed that we want insert_key_rna() to be able to take paths with fully qualified array indices. However, I'm not 100% sold on appending "[##]" to the path string to accomplish that. An alternative is for insert_key_rna() to take a span of string + int pairs rather than just a span strings like it does now.
This refactor is also partially in preparation for enabling button keyframing for Baklava, because insert_key_rna() will already handle Baklava keying after #120428 lands.

This commit does two things: - Switches manual (not auto) keyframing of buttons to use `insert_key_rna()` instead of `insert_keyframe()`. - Modifies `insert_key_rna()` to accept and understand rna paths that specify an array element (for array properties), in order to make the above possible. This is part of a progressive refactor that is unifying the keyframing code to use the same keyframing functions (in particular converging on `insert_key_rna()`), rather than many different functions with subtly different behavior. **Notes for reviewers:** - @ChrisLend and I already discussed that we want `insert_key_rna()` to be able to take paths with fully qualified array indices. However, I'm not 100% sold on appending "[##]" to the path string to accomplish that. An alternative is for `insert_key_rna()` to take a span of string + int pairs rather than just a span strings like it does now. - This refactor is also partially in preparation for enabling button keyframing for Baklava, because `insert_key_rna()` will already handle Baklava keying after #120428 lands.

Nathan Vegdahl added the

Module

Animation & Rigging

label 2024-05-06 17:55:36 +02:00

Nathan Vegdahl added 1 commit 2024-05-06 17:55:46 +02:00

Refactor: use insert_key_rna() for button keyframing 362380fe46

This commit does two things:
- Modifies `insert_key_rna()` to accept and understand rna paths that
  specify a specific array element of an array property.
- Switches manual (not auto) keyframing of property buttons to use
  `insert_key_rna()` instead of `insert_keyframe()` like it was before.

This is part of a progressive refactor that is trying to unify the
keyframing code to use the same keyframing functions, rather than many
different functions with subtley different behavior.

Nathan Vegdahl requested review from Christoph Lendenfeld 2024-05-06 17:56:10 +02:00

Nathan Vegdahl requested review from Sybren A. Stüvel 2024-05-06 17:56:44 +02:00

Christoph Lendenfeld approved these changes 2024-05-07 10:00:19 +02:00

Christoph Lendenfeld left a comment

Curious what sybren thinks of this, but I like this solution.
It keeps the function interface of insert_key_rna really simple.

The only thing that is slightly annoying is having to hardcode the [] into the string.

Curious what sybren thinks of this, but I like this solution. It keeps the function interface of `insert_key_rna` really simple. The only thing that is slightly annoying is having to hardcode the `[]` into the string.

Christoph Lendenfeld referenced this pull request

2024-05-07 10:01:10 +02:00

Refactor keyframing.cc in animrig #119776

Nathan Vegdahl commented

2024-05-07 10:24:19 +02:00

Yeah, I think this is a perfectly reasonable solution. And I agree that it keeps the interface for insert_key_rna() simpler when looking at just insert_key_rna().

However, I suspect that in practice it will make insert_key_rna() more complicated to call from most places: you usually have the index as an integer, not already embedded in the path string, so this way you have to take the time and code space to append that index to the string before you can call insert_key_rna(). Whereas if it's passed as an integer alongside the path, you can just do that directly without any real prep work.

Having said that, I think it would be a pretty straightforward refactor going from one to the other, so I don't think it's critical that we get it "right" straight away.

Yeah, I think this is a perfectly reasonable solution. And I agree that it keeps the interface for `insert_key_rna()` simpler when looking at just `insert_key_rna()`. However, I suspect that in practice it will make `insert_key_rna()` more complicated to call from most places: you usually have the index as an integer, not already embedded in the path string, so this way you have to take the time and code space to append that index to the string before you can call `insert_key_rna()`. Whereas if it's passed as an integer alongside the path, you can just do that directly without any real prep work. Having said that, I think it would be a pretty straightforward refactor going from one to the other, so I don't think it's critical that we get it "right" straight away.

Nathan Vegdahl added 1 commit 2024-05-10 15:13:17 +02:00

Remove unnecessary temporary Vector 1c2501d7d0

It turns out that you can just create a temporary Span with initializer
lists, and it works fine.  (Confirmed with Jacques Lucke, who wrote
Span.)

Nathan Vegdahl added 1 commit 2024-05-10 15:13:31 +02:00

Merge branch 'main' into button_keying_uniformity f47c49ccd3

Nathan Vegdahl commented

2024-05-10 15:53:02 +02:00

After some discussion about a related refactor with @ChrisLend, I think I'm going to split off the Modifies insert_key_rna() to accept and understand rna paths that specify an array element part of this into a separate PR.

Edit: here's that PR: #121879

After some discussion about a related refactor with @ChrisLend, I think I'm going to split off the `Modifies insert_key_rna() to accept and understand rna paths that specify an array element` part of this into a separate PR. Edit: here's that PR: #121879

~~Nathan Vegdahl referenced this pull request 2024-05-16 17:07:47 +02:00~~

Anim: change `insert_key_rna()` to take indices with its rna paths #121879

Sybren A. Stüvel requested changes 2024-06-26 17:52:03 +02:00

source/blender/animrig/intern/keyframing.cc

						
				@ -1083,3 +1083,2 @@

				    PropertyRNA *prop = nullptr;

				    const bool path_resolved = RNA_path_resolve_property(

				        rna_pointer, rna_path.c_str(), &ptr, &prop);

				    int index;

Sybren A. Stüvel commented

2024-06-26 17:49:33 +02:00

I think this is the array index, and if so, please name it array_index. That just makes it more concrete.

I *think* this is the array index, and if so, please name it `array_index`. That just makes it more concrete.

source/blender/editors/animation/keyframing.cc

						
				@ -1037,0 +1016,4 @@

				         * then add the specific index of the element we're keying to end of the

				         * path. */

				        std::string real_path = std::move(path.value());

				        if (!all) {

Sybren A. Stüvel commented

2024-06-26 17:51:51 +02:00

Given that #121879 has landed in main, why is this still necessary?

Given that #121879 has landed in `main`, why is this still necessary?

Nathan Vegdahl commented

2024-06-27 10:41:09 +02:00

Gah! Yeah, this whole PR is redundant with other stuff that's already landed. Will close.

Nathan Vegdahl commented

2024-06-27 10:43:21 +02:00

Everything this PR was trying to do has now been taken care of in #121879 and #122053. So it's no longer needed.

Nathan Vegdahl closed this pull request

2024-06-27 10:43:24 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Refactor: use insert_key_rna() for button keyframing #121491

Pull request closed

Refactor: use `insert_key_rna()` for button keyframing #121491