Fix T76597: Adds support for Copy To Selected for Keyframes.
Example File:
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- keyframes_copy_to_selected (branched from master)
- Build Status
Buildable 8110 Build 8110: arc lint + arc unit
Event Timeline
Comment Actions
Thanks for the patch.
| source/blender/editors/interface/interface_ops.c | ||
|---|---|---|
| 812–830 | This code should be in a separate function. | |
| 818 | This variable could have a more descriptive name. There is no need to call this "tmp", as the variable's scope is just as temporary as all the other ones. I'm also not sure what the "r" stands for, or why it's separate from the other variable declarations. | |
| 822 | Use if ((bezt->f2 & SELECT) == 0) { continue; }, and un-indent the remaining code. | |
| 824 | This is the only place in this function where memory is explicitly allocated. Why is this necessary here, but not in the other cases? | |
Comment Actions
Addressed the code changes. In the new commit, I've moved the core of the code into screen_context.c since that seemed to follow existing conventions.
I'm not sure the proper way to mark things as "done" in the comments. Let me know where I messed up.
| source/blender/editors/interface/interface_ops.c | ||
|---|---|---|
| 812–830 | Alright, it looks like the function already existed: CTX_data_list_add() in context.c | |
| 818 | Alright. In the new commit, the variable is unnecessary anyways. | |
| 824 | The others allocate too. They're just nested and eventually lead to the only function in screen_context.c. The Copy To Selected op frees the memory after use. | |
Comment Actions
Nice work, just a few tiny nags left.
| source/blender/editors/screen/screen_context.c | ||
|---|---|---|
| 767 ↗ | (On Diff #25651) | Comparing ac.spacetype to two values is a lot faster than getting animdata from the context. Swap the two conditions, so that the fastest is evaluated first (so that it can entirely skip the call to ANIM_animdata_get_context()). |
| 782 ↗ | (On Diff #25651) | You can flip the condition here and use a continue statement. This entire function is already way too complex, so let's keep new additions as little nested as possible. |
Comment Actions
-rebase
-Applied 2nd note.
Did not apply first note since we need to obtain the animation context first. I can go into Anim_animdata_get_context() and extract the related code to get spacetype if that's preferable.
Comment Actions
Not all notes have been resolved. If you're not planning to do those, please add a comment to them to explain why not.
| source/blender/editors/screen/screen_context.c | ||
|---|---|---|
| 767 ↗ | (On Diff #25651) | Not applied since we need to obtain the animation context first before we can test ac.space_type. I can go into Anim_animdata_get_context() and extract the related code to get space_type if that's preferable. |
Comment Actions
The patch looks good to me. As it is described as depending on D7737: Fix T76595: Indicate the Active Keyframe in Graph Editor, it probably should be commited after that patch.
| source/blender/editors/screen/screen_context.c | ||
|---|---|---|
| 767 ↗ | (On Diff #25651) |
Doh! |
Comment Actions
I've tried to apply the patch to current master, now that D7737: Fix T76595: Indicate the Active Keyframe in Graph Editor is in. I can select multiple keyframes, and one of them is shown as active, but I still can't select "Copy to Selected".
I'd love to get this into 2.91, but that means that it has to be fixed in the coming hour or so.