Page MenuHome
Differential D14738 Diff 52727 source/blender/editors/space_nla/nla_draw.c

Changeset View

Standalone View

View Options

source/blender/editors/space_nla/nla_draw.c

Show First 20 Lines • Show All 615 Lines • ▼ Show 20 Linesstatic void nla_draw_strip(SpaceNla *snla,
immUnbindProgram(); immUnbindProgram();
} }
/* add the relevant text to the cache of text-strings to draw in pixelspace */ /* add the relevant text to the cache of text-strings to draw in pixelspace */
static void nla_draw_strip_text(AnimData *adt, static void nla_draw_strip_text(AnimData *adt,
NlaTrack *nlt, NlaTrack *nlt,
NlaStrip *strip, NlaStrip *strip,
int index,
View2D *v2d, View2D *v2d,
float xminc, float xminc,
float xmaxc, float xmaxc,
float yminc, float yminc,
float ymaxc) float ymaxc)
{ {
const bool non_solo = ((adt && (adt->flag & ADT_NLA_SOLO_TRACK)) && const bool non_solo = ((adt && (adt->flag & ADT_NLA_SOLO_TRACK)) &&
(nlt->flag & NLATRACK_SOLO) == 0); (nlt->flag & NLATRACK_SOLO) == 0);
char str[256]; char str[256];
size_t str_len; size_t str_len;
uchar col[4]; uchar col[4];
/* just print the name and the range */ /* just print the name and the range */
if (strip->flag & NLASTRIP_FLAG_TEMP_META) { if (strip->flag & NLASTRIP_FLAG_TEMP_META) {
str_len = BLI_snprintf_rlen(str, sizeof(str), "%d) Temp-Meta", index); str_len = BLI_snprintf_rlen(str, sizeof(str), "Temp-Meta");
} }
else { else {
str_len = BLI_strncpy_rlen(str, strip->name, sizeof(str)); str_len = BLI_strncpy_rlen(str, strip->name, sizeof(str));
} }
/* set text color - if colors (see above) are light, draw black text, otherwise draw white */ /* set text color - if colors (see above) are light, draw black text, otherwise draw white */
if (strip->flag & (NLASTRIP_FLAG_ACTIVE | NLASTRIP_FLAG_TWEAKUSER)) { if (strip->flag & (NLASTRIP_FLAG_ACTIVE | NLASTRIP_FLAG_TWEAKUSER)) {
col[0] = col[1] = col[2] = 0; col[0] = col[1] = col[2] = 0;
▲ Show 20 Lines • Show All 49 Lines • ▼ Show 20 Linesstatic void nla_draw_strip_frames_text(
/* end frame */ /* end frame */
numstr_len = BLI_snprintf_rlen(numstr, sizeof(numstr), "%.1f", strip->end); numstr_len = BLI_snprintf_rlen(numstr, sizeof(numstr), "%.1f", strip->end);
UI_view2d_text_cache_add(v2d, strip->end, ymaxc + ytol, numstr, numstr_len, col); UI_view2d_text_cache_add(v2d, strip->end, ymaxc + ytol, numstr, numstr_len, col);
} }
/* ---------------------- */ /* ---------------------- */
/**
* Gets the first and last visible NLA strips on a track.
* Note that this also includes tracks that might only be
* visible because of their extendmode.
sybrenUnsubmitted
Done
Inline Actions

Comments should be English sentences, so including capitalisation and punctuation. Same for the other comments.

sybren: Comments should be English sentences, so including capitalisation and punctuation. Same for the…
*/
sybrenUnsubmitted
Done
Inline Actions

static void

sybren: `static void`
sybrenUnsubmitted
Done
Inline Actions

See https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming on naming of function arguments.

Having said that, I think it's better to have this function return a ListBase /* NlaStrip */ instead.

sybren: See https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming on naming of function arguments.
static ListBase get_visible_nla_strips(NlaTrack *nlt, View2D *v2d)
{
if (BLI_listbase_is_empty(&nlt->strips)) {
ListBase empty = {NULL, NULL};
return empty;
}
NlaStrip *first = NULL;
NlaStrip *last = NULL;
sybrenUnsubmitted
Done
Inline Actions

Many comments refer to tracks, when they should refer to strips.

sybren: Many comments refer to tracks, when they should refer to strips.
sybrenUnsubmitted
Not Done
Inline Actions

I think the code will clean up a bit if you defer the ListBase construction to a spot later in the function. That way you can do something like this:

if (BLI_listbase_is_empty(&nlt->strips)) {
  ListBase empty = {NULL, NULL};
  return empty;
}

NlaStrip *first = NULL;
NlaStrip *last = NULL;

// ... Do stuff, set first/last

ListBase visible_strips = {first, last};
return visible_strips;

This also makes breaking up this function into smaller bits easier as you can just do something like this, without having to bother passing first and last around:

if (has_strips_within_bounds) {
  return the_first_function(...);
}
return the_other_function(...);
sybren: I think the code will clean up a bit if you defer the `ListBase` construction to a spot later…
/* Find the first strip that is within the bounds of the view. */
LISTBASE_FOREACH (NlaStrip *, strip, &nlt->strips) {
if (BKE_nlastrip_within_bounds(strip, v2d->cur.xmin, v2d->cur.xmax)) {
first = last = strip;
break;
}
}
const bool has_strips_within_bounds = first != NULL;
sybrenUnsubmitted
Not Done
Inline Actions

This means that the if and else blocks should just be their own function.

sybren: This means that the `if` and `else` blocks should just be their own function.
cmbasnettAuthorUnsubmitted
Done
Inline Actions

I'm not sure what you mean here. Perhaps the naming was confusing? I've changed it in the updated diff.

cmbasnett: I'm not sure what you mean here. Perhaps the naming was confusing? I've changed it in the…
sybrenUnsubmitted
Not Done
Inline Actions

What I mean is that the need to comments blocks of code, to explain what they do, can be used as an indicator that that particular block should actually be in its own function. Looking at the maximum indentation level of the code below, I think that's actually a good idea.

sybren: What I mean is that the need to comments blocks of code, to explain what they do, can be used…
if (has_strips_within_bounds) {
/* Find the last visible strip. */
for (NlaStrip *strip = first->next; strip; strip = strip->next) {
sybrenUnsubmitted
Done
Inline Actions

if the strips are stored in order, you should be able to break here.

sybren: if the strips are stored in order, you should be able to `break` here.
if (!BKE_nlastrip_within_bounds(strip, v2d->cur.xmin, v2d->cur.xmax)) {
break;
}
last = strip;
}
/* Check if the first strip is adjacent to a strip outside the view to the left
* that has an extendmode region that should be drawn.
* If so, adjust the first strip to include drawing that strip as well.
*/
NlaStrip *prev = first->prev;
if (prev && prev->extendmode != NLASTRIP_EXTEND_NOTHING) {
first = prev;
sybrenUnsubmitted
Done
Inline Actions

This is not what happens in the code. The comment suggests comparison between distances, which doesn't happen.

sybren: This is not what happens in the code. The comment suggests comparison between distances, which…
}
}
else {
/* No immediately visible strips.
* Figure out where our view is relative to the strips, then determine
* if the view is adjacent to a strip that should have its extendmode
* rendered.
*/
NlaStrip *first_strip = nlt->strips.first;
NlaStrip *last_strip = nlt->strips.last;
if (first_strip && v2d->cur.xmax < first_strip->start &&
first_strip->extendmode == NLASTRIP_EXTEND_HOLD) {
/* The view is to the left of all strips and the first strip has an
* extendmode that should be drawn.
*/
first = last = first_strip;
}
else if (last_strip && v2d->cur.xmin > last_strip->end &&
sybrenUnsubmitted
Done
Inline Actions

use LISTBASE_FOREACH

sybren: use `LISTBASE_FOREACH`
last_strip->extendmode != NLASTRIP_EXTEND_NOTHING) {
/* The view is to the right of all strips and the last strip has an
* extendmode that should be drawn.
sybrenUnsubmitted
Done
Inline Actions

This considers each and every strip that's after the current frame, and if it's preceeded by an extending strip, returns that preceeding strip. This will produce false positives, namely when the actual strip just left of the viewport is NOT extending, but some later strip actually is.

sybren: This considers each and every strip that's after the current frame, and if it's preceeded by an…
cmbasnettAuthorUnsubmitted
Done
Inline Actions

Very good catch. This is a very subtle bug that doesn't actually visually manifest.

cmbasnett: Very good catch. This is a very subtle bug that doesn't actually visually manifest.
*/
first = last = last_strip;
}
else {
/* The view is in the middle of two strips. */
LISTBASE_FOREACH (NlaStrip *, strip, &nlt->strips) {
/* Find the strip to the left by finding the strip to the right and getting its prev. */
if (v2d->cur.xmax < strip->start) {
/* If the strip to the left has an extendmode, set that as the only visible strip. */
if (strip->prev && strip->prev->extendmode != NLASTRIP_EXTEND_NOTHING) {
first = last = strip->prev;
}
break;
}
}
}
}
ListBase visible_strips = {first, last};
return visible_strips;
}
void draw_nla_main_data(bAnimContext *ac, SpaceNla *snla, ARegion *region) void draw_nla_main_data(bAnimContext *ac, SpaceNla *snla, ARegion *region)
{ {
View2D *v2d = &region->v2d; View2D *v2d = &region->v2d;
const float pixelx = BLI_rctf_size_x(&v2d->cur) / BLI_rcti_size_x(&v2d->mask); const float pixelx = BLI_rctf_size_x(&v2d->cur) / BLI_rcti_size_x(&v2d->mask);
const float text_margin_x = (8 * UI_DPI_FAC) * pixelx; const float text_margin_x = (8 * UI_DPI_FAC) * pixelx;
/* build list of channels to draw */ /* build list of channels to draw */
ListBase anim_data = {NULL, NULL}; ListBase anim_data = {NULL, NULL};
Show All 19 Lines for (bAnimListElem *ale = anim_data.first; ale; ale = ale->next, ymax -= NLACHANNEL_STEP(snla)) {
/* check if visible */ /* check if visible */
if (IN_RANGE(ymin, v2d->cur.ymin, v2d->cur.ymax) || if (IN_RANGE(ymin, v2d->cur.ymin, v2d->cur.ymax) ||
IN_RANGE(ymax, v2d->cur.ymin, v2d->cur.ymax)) { IN_RANGE(ymax, v2d->cur.ymin, v2d->cur.ymax)) {
/* data to draw depends on the type of channel */ /* data to draw depends on the type of channel */
switch (ale->type) { switch (ale->type) {
case ANIMTYPE_NLATRACK: { case ANIMTYPE_NLATRACK: {
AnimData *adt = ale->adt; AnimData *adt = ale->adt;
NlaTrack *nlt = (NlaTrack *)ale->data; NlaTrack *nlt = (NlaTrack *)ale->data;
NlaStrip *strip; ListBase visible_nla_strips = get_visible_nla_strips(nlt, v2d);
int index;
/* draw each strip in the track (if visible) */ /* Draw each visible strip in the track. */
sybrenUnsubmitted
Done
Inline Actions

Touching means updating to latest code style ;-)
So start with capital letter and end in a period. I can do this while landing the patch.

sybren: Touching means updating to latest code style ;-) So start with capital letter and end in a…
for (strip = nlt->strips.first, index = 1; strip; strip = strip->next, index++) { LISTBASE_FOREACH (NlaStrip *, strip, &visible_nla_strips) {
if (BKE_nlastrip_within_bounds(strip, v2d->cur.xmin, v2d->cur.xmax)) {
const float xminc = strip->start + text_margin_x; const float xminc = strip->start + text_margin_x;
const float xmaxc = strip->end - text_margin_x; const float xmaxc = strip->end - text_margin_x;
/* draw the visualization of the strip */ /* draw the visualization of the strip */
nla_draw_strip(snla, adt, nlt, strip, v2d, ymin, ymax); nla_draw_strip(snla, adt, nlt, strip, v2d, ymin, ymax);
/* add the text for this strip to the cache */ /* add the text for this strip to the cache */
if (xminc < xmaxc) { if (xminc < xmaxc) {
nla_draw_strip_text(adt, nlt, strip, index, v2d, xminc, xmaxc, ymin, ymax); nla_draw_strip_text(adt, nlt, strip, v2d, xminc, xmaxc, ymin, ymax);
} }
/* if transforming strips (only real reason for temp-metas currently), /* if transforming strips (only real reason for temp-metas currently),
* add to the cache the frame numbers of the strip's extents * add to the cache the frame numbers of the strip's extents
*/ */
if (strip->flag & NLASTRIP_FLAG_TEMP_META) { if (strip->flag & NLASTRIP_FLAG_TEMP_META) {
nla_draw_strip_frames_text(nlt, strip, v2d, ymin, ymax); nla_draw_strip_frames_text(nlt, strip, v2d, ymin, ymax);
} }
} }
}
break; break;
} }
case ANIMTYPE_NLAACTION: { case ANIMTYPE_NLAACTION: {
AnimData *adt = ale->adt; AnimData *adt = ale->adt;
/* Draw the manually set intended playback frame range highlight. */ /* Draw the manually set intended playback frame range highlight. */
if (ale->data) { if (ale->data) {
ANIM_draw_action_framerange(adt, ale->data, v2d, ymin, ymax); ANIM_draw_action_framerange(adt, ale->data, v2d, ymin, ymax);
▲ Show 20 Lines • Show All 120 Lines • Show Last 20 Lines