blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 767 Projects 19 Wiki Activity

Animation: Add "Frame Channel" operators #104523

Merged
Christoph Lendenfeld merged 27 commits from ChrisLend/blender:frame-channel-operator into main 2023-02-17 18:11:10 +01:00
Conversation 3 Commits 27 Files Changed 7 +22 -21
4 changed files with 22 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 406e2c0ac0 - Show all commits

2
release/datafiles/locale

@ -1 +1 @@
Subproject commit 4331c8e76c2f42b9fd903716c333d6cdeaa5cebd
Subproject commit 8d04896a130c12eb440b7b9ac33bf9dc152506b5

2
release/scripts/addons_contrib

@ -1 +1 @@
Subproject commit 14ab9273409ea0231d08ba6e86fdc73d4e459e99
Subproject commit 8f4599c4fb6ab2386f2498814c203fd676b2ccf2

37
source/blender/editors/animation/anim_channels_edit.c
View File

@ -3643,10 +3643,10 @@ static void ANIM_OT_channel_select_keys(wmOperatorType *ot)
/** \name View Channel Operator
* \{ */
dr.sybren marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:38:34 +01:00
Review
Copy Link

I think these three pointers can all be const, as this only gets the bounds and shouldn't set anything.

I think these three pointers can all be `const`, as this only gets the bounds and shouldn't set anything.
Christoph Lendenfeld commented 2023-02-16 13:21:53 +01:00
Review
Copy Link

only ale can be const in this case
because ANIM_get_normalization_flags and ANIM_unit_mapping_get_factor would throw warnings otherwise since they aren't set to const

only `ale` can be const in this case because `ANIM_get_normalization_flags` and `ANIM_unit_mapping_get_factor` would throw warnings otherwise since they aren't set to const
static void get_normalized_fcurve_bounds(FCurve *fcu,
bAnimContext *ac,
bAnimListElem *ale,
bool include_handles,
static void get_normalized_fcurve_bounds(const FCurve *fcu,
const bAnimContext *ac,
const bAnimListElem *ale,
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:38:06 +01:00
Review
Copy Link

const

`const`
const bool include_handles,
const float range[2],
rctf *r_bounds)
{
@ -3659,14 +3659,14 @@ static void get_normalized_fcurve_bounds(FCurve *fcu,
fcu_selection_only,
include_handles,
range);
const short mapping_flag = ANIM_get_normalization_flags(ac);
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:39:03 +01:00
Review
Copy Link

const

`const`
float unitFac, offset;
short mapping_flag = ANIM_get_normalization_flags(ac);
unitFac = ANIM_unit_mapping_get_factor(ac->scene, ale->id, fcu, mapping_flag, &offset);
r_bounds->ymin += offset;
r_bounds->ymax += offset;
r_bounds->ymin *= unitFac;
r_bounds->ymax *= unitFac;
float offset;
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:38:59 +01:00
Review
Copy Link

Declaring unitFac here makes it possible to const float it.

Declaring `unitFac` here makes it possible to `const float` it.
const float unit_fac = ANIM_unit_mapping_get_factor(
ac->scene, ale->id, fcu, mapping_flag, &offset);
Christoph Lendenfeld commented 2023-02-17 17:19:00 +01:00
Review
Copy Link

@dr.sybren
fixed the issue with the flat curves with that

@dr.sybren fixed the issue with the flat curves with that
r_bounds->ymin = (r_bounds->ymin + offset) * unit_fac;
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:37:46 +01:00
Review
Copy Link

I think it's nicer to see the entire formula in one go:

r_bounds->ymin = unitFac * (r_bounds->ymin + offset);
I think it's nicer to see the entire formula in one go: ```c r_bounds->ymin = unitFac * (r_bounds->ymin + offset); ```
r_bounds->ymax = (r_bounds->ymax + offset) * unit_fac;
}
static void get_gpencil_bounds(bGPDlayer *gpl, const float range[2], rctf *r_bounds)
@ -3727,20 +3727,20 @@ static void get_view_range(Scene *scene, const bool use_preview_range, float r_r
}
}
dr.sybren marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:40:30 +01:00
Review
Copy Link

How can the view be blocked, by taking regions into account?
I think for the right meaning the comma has to be removed.

Having said that, the comment is a bit vague, "take into account" doesn't mean that much until you know where & how the function is used.

It's better to be more descriptive of what it does, rather than about where it is supposed to be used.

How can the view be blocked, by taking regions into account? I think for the right meaning the comma has to be removed. Having said that, the comment is a bit vague, "take into account" doesn't mean that much until you know where & how the function is used. It's better to be more descriptive of what it does, rather than about where it is supposed to be used.
Christoph Lendenfeld commented 2023-02-16 13:23:07 +01:00
Review
Copy Link

let me know if the comment makes more sense now

let me know if the comment makes more sense now
/* Take regions into account, that could block the view.
* Marker region is supposed to be larger than the scroll-bar, so prioritize it. */
/* Pad the given rctf with regions that could block the view.
* For example Markers and Time Scrubbing. */
static void add_region_padding(bContext *C, bAnimContext *ac, rctf *bounds)
{
BLI_rctf_scale(bounds, 1.1f);
float pad_top = UI_TIME_SCRUB_MARGIN_Y;
float pad_bottom = BLI_listbase_is_empty(ED_context_get_markers(C)) ? V2D_SCROLL_HANDLE_HEIGHT :
UI_MARKER_MARGIN_Y;
const float pad_top = UI_TIME_SCRUB_MARGIN_Y;
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:42:26 +01:00
Review
Copy Link

const

`const`
const float pad_bottom = BLI_listbase_is_empty(ED_context_get_markers(C)) ?
V2D_SCROLL_HANDLE_HEIGHT :
UI_MARKER_MARGIN_Y;
BLI_rctf_pad_y(bounds, ac->region->winy, pad_bottom, pad_top);
}
dr.sybren marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:43:10 +01:00
Review
Copy Link

Within the context of this patch "the operator" can be understood, but once this is in Blender's sources, it's a bit vague. The doc also explains what this code does in terms of what other code didn't do, and that creates a tight coupling that's not wanted.

Within the context of this patch "the operator" can be understood, but once this is in Blender's sources, it's a bit vague. The doc also explains what this code does in terms of what other code didn't do, and that creates a tight coupling that's not wanted.
Christoph Lendenfeld commented 2023-02-16 13:28:52 +01:00
Review
Copy Link

I hope the comment is better now

I hope the comment is better now
/* Iterates through regions because the operator might not have been called from the correct
* region. */
/* Find the window region in the bAnimContext area and move it to bounds. */
static void move_graph_view(bContext *C, bAnimContext *ac, rctf *bounds, const int smooth_viewtx)
{
LISTBASE_FOREACH (ARegion *, region, &ac->area->regionbase) {
@ -3765,6 +3765,7 @@ static int graphkeys_view_selected_channels_exec(bContext *C, wmOperator *op)
size_t anim_data_length = ANIM_animdata_filter(&ac, &anim_data, filter, ac.data, ac.datatype);
if (anim_data_length == 0) {
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-14 14:45:25 +01:00
Review
Copy Link

Use WM_report to show a warning that explains why the operator couldn't run.

Use `WM_report` to show a warning that explains why the operator couldn't run.
WM_report(RPT_WARNING, "No channels to operate on");
return OPERATOR_CANCELLED;
}

2
source/tools

@ -1 +1 @@
Subproject commit e133fc08cd3254bb3d3bd1345028c8486700bca4
Subproject commit 27826b4aed138b7e3aed882d0eab96740c3a55c7