blender
/
blender
134
422
Fork 605
Code Issues 6.4k Pull Requests 767 Projects 19 Wiki Activity

Refactor: keyframing functions return more detailed status #117449

Merged
Christoph Lendenfeld merged 19 commits from ChrisLend/blender:refactor_keying_reports into main 2024-03-08 09:56:52 +01:00
Conversation 7 Commits 19 Files Changed 2 +182 -98
2 changed files with 182 additions and 98 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
source/blender/animrig/CMakeLists.txt
View File

@ -11,6 +11,7 @@ set(INC
../editors/include
../makesrna
../windowmanager
../../../extern/fmtlib/include
# RNA_prototypes.h
${CMAKE_BINARY_DIR}/source/blender/makesrna
)

279
source/blender/animrig/intern/keyframing.cc
View File

@ -10,6 +10,8 @@
#include <cmath>
#include <string>
#include <fmt/format.h>
#include "ANIM_action.hh"
#include "ANIM_animdata.hh"
#include "ANIM_fcurve.hh"
@ -46,6 +48,45 @@
namespace blender::animrig {
ChrisLend marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-01-23 20:38:12 +01:00
Outdated
Review
Copy Link

This will pollute the namespace, as it'll create blender::animrig::SUCESS. Better to use an enum class, as that limits the scope to blender::animrig::KeyframingResult::SUCESS.

This will pollute the namespace, as it'll create `blender::animrig::SUCESS`. Better to use an `enum class`, as that limits the scope to `blender::animrig::KeyframingResult::SUCESS`.
Christoph Lendenfeld commented 2024-01-25 11:26:25 +01:00
Outdated
Review
Copy Link

I've put the enum into a namespace to combat that. enum class doesn't work with bitwise logic
Maybe this needs a completely different solution. Probably worth waiting for the result of https://devtalk.blender.org/t/developer-discussion-handling-errors/33054/5

I've put the enum into a namespace to combat that. `enum class` doesn't work with bitwise logic Maybe this needs a completely different solution. Probably worth waiting for the result of https://devtalk.blender.org/t/developer-discussion-handling-errors/33054/5
Sybren A. Stüvel commented 2024-02-01 15:39:53 +01:00
Outdated
Review
Copy Link

I think those names need some work, as it's not clear how these two classes interact with each other and how they'd be used.

My suggestion:

  • KeyingResultOptions → SingleKeyingResult
  • KeyingResult → AggregatedKeyingResult

It'll also help in the rest of the code to be more explicit about whether a variable represents a single result or an aggregated one. For example in a line like this:

keying_result.add_result(result);

When read out of context, it's unclear that keying_result and result are of different semantic levels. Something like aggregated_result vs result might work well, although it is a bit long.

I think those names need some work, as it's not clear how these two classes interact with each other and how they'd be used. My suggestion: - `KeyingResultOptions` → `SingleKeyingResult` - `KeyingResult` → `AggregatedKeyingResult` It'll also help in the rest of the code to be more explicit about whether a variable represents a single result or an aggregated one. For example in a line like this: ```cpp keying_result.add_result(result); ``` When read out of context, it's unclear that `keying_result` and `result` are of different semantic levels. Something like `aggregated_result` vs `result` might work well, although it is a bit long.
Christoph Lendenfeld commented 2024-02-01 17:11:24 +01:00
Outdated
Review
Copy Link

I used CombinedKeyingResult
That felt like a simpler word to me that conveys the same meaning

I used `CombinedKeyingResult` That felt like a simpler word to me that conveys the same meaning
👍 1
enum class SingleKeyingResult {
SUCCESS = 0,
CANNOT_CREATE_FCURVE,
FCURVE_NOT_KEYFRAMEABLE,
ChrisLend marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-02-01 15:41:03 +01:00
Outdated
Review
Copy Link

There is no constructor of KeyingResult, so this comment is a bit misleading. It might be better to add an extra _KEYING_RESULT_MAX, which you can then use instead of int(KeyingResultOptions::NO_KEY_NEEDED) + 1) to remove the interdependency of the code. Do add a comment that that should be the last item in the enum.

There is no constructor of `KeyingResult`, so this comment is a bit misleading. It might be better to add an extra `_KEYING_RESULT_MAX`, which you can then use instead of `int(KeyingResultOptions::NO_KEY_NEEDED) + 1)` to remove the interdependency of the code. Do add a comment that that should be the last item in the enum.
Christoph Lendenfeld commented 2024-02-01 17:10:41 +01:00
Outdated
Review
Copy Link

really good idea.
Also removes the weird +1

really good idea. Also removes the weird `+1`
NO_KEY_NEEDED,
/* Make sure to always keep this at the end of the enum. */
_KEYING_RESULT_MAX,
};
ChrisLend marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-02-01 15:42:46 +01:00
Outdated
Review
Copy Link

I think this can be better described as "Mapping from SingleKeyingResult to the number of times that that result occurred". Or something along those lines.

I think this can be better described as "Mapping from `SingleKeyingResult` to the number of times that that result occurred". Or something along those lines.
class CombinedKeyingResult {
ChrisLend marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-02-01 15:46:29 +01:00
Outdated
Review
Copy Link

Rename to result_counter_ (trailing _ because private).

Rename to `result_counter_` (trailing `_` because `private`).
private:
/* The index to the array maps a `SingleKeyingResult` to the number of times this result has
* occurred. */
ChrisLend marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-02-01 15:44:16 +01:00
Outdated
Review
Copy Link

I think add() would be enough, given that it's clear what it adds from its name AggregatedKeyingResult and the parameter.

I think `add()` would be enough, given that it's clear what it adds from its name `AggregatedKeyingResult` and the parameter.
std::array<int, int(SingleKeyingResult::_KEYING_RESULT_MAX)> result_counter{0};
public:
void add(const SingleKeyingResult result)
{
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2024-02-01 15:51:49 +01:00
Review
Copy Link

For reporting I think it's better if this is changed to int get_count(const SingleKeyingResult result). This has a few advantages:

  • Not all results are errors, so has_error(SUCCESS) is weird.
  • Instead of "One or more FCurves ..." you can write "{count} FCurves".

Without this, I don't think it's worth going through the efforts of using the array and keeping the separate counts.

For reporting I think it's better if this is changed to `int get_count(const SingleKeyingResult result)`. This has a few advantages: - Not all results are errors, so `has_error(SUCCESS)` is weird. - Instead of "One or more FCurves ..." you can write "{count} FCurves". Without this, I don't think it's worth going through the efforts of using the array and keeping the separate counts.
👍 1
Christoph Lendenfeld commented 2024-02-01 17:19:24 +01:00
Review
Copy Link

how does {count} FCurves work?
as far as I can tell that is a C++ 20 feature

how does `{count} FCurves` work? as far as I can tell that is a C++ 20 feature
Sybren A. Stüvel commented 2024-02-02 11:06:42 +01:00
Review
Copy Link

Oh sorry, that was just me typing shorthand. But, fmt::format("{} F-Curves…", the_count) will bring you quite close.
Of course there's also the always-annoying handling of the singular form :/

Oh sorry, that was just me typing shorthand. But, `fmt::format("{} F-Curves…", the_count)` will bring you quite close. Of course there's also the always-annoying handling of the singular form :/
Christoph Lendenfeld commented 2024-02-02 15:21:29 +01:00
Review
Copy Link

done that, had to include ../../../extern/fmtlib/include in the CMakeLists.txt for that
Let me know if that is ok

done that, had to include `../../../extern/fmtlib/include` in the `CMakeLists.txt` for that Let me know if that is ok
Sybren A. Stüvel commented 2024-02-05 10:59:58 +01:00
Review
Copy Link

Yeah that seems to be the way to do it.

Yeah that seems to be the way to do it.
result_counter[int(result)]++;
}
int get_count(const SingleKeyingResult result) const
{
return result_counter[int(result)];
}
ChrisLend marked this conversation as resolved
Sybren A. Stüvel commented 2024-02-01 15:45:45 +01:00
Review
Copy Link

This can be simplified to:

    for (const int result_count : result_counter) {
      if (result_count) {
        return true;
      }
    }
This can be simplified to: ```cpp for (const int result_count : result_counter) { if (result_count) { return true; } } ```
Christoph Lendenfeld commented 2024-02-01 16:53:45 +01:00
Review
Copy Link

since SUCCESS is at index 0 I don't think I can do that.
That's why I have int i=1
There might be a [1:] equivalent in C++ that I haven't found though

since `SUCCESS` is at index 0 I don't think I can do that. That's why I have `int i=1` There might be a `[1:]` equivalent in C++ that I haven't found though
Sybren A. Stüvel commented 2024-02-02 11:08:22 +01:00
Review
Copy Link

Hmmm good point. You could keep track of errors & successes in a different way, that way the array would only contain errors. I don't think it's worth the trouble though, juts keep the code as-is. Maybe add a comment for people who overlook the i=1 bit ;-)

And add a comment that this code assumes that 0 is SUCCESS and the rest of the enum has sequential values. That's not always a given.

Hmmm good point. You could keep track of errors & successes in a different way, that way the array would only contain errors. I don't think it's worth the trouble though, juts keep the code as-is. Maybe add a comment for people who overlook the `i=1` bit ;-) And add a comment that this code assumes that 0 is `SUCCESS` and the rest of the enum has sequential values. That's not always a given.
👍 1
bool has_errors() const
{
/* For loop starts at 1 to skip the SUCCESS flag. Assumes that SUCCESS is 0 and the rest of the
* enum are sequential values. */
for (int i = 1; i < result_counter.size(); i++) {
if (result_counter[i] > 0) {
return true;
}
}
return false;
}
};
void update_autoflags_fcurve_direct(FCurve *fcu, PropertyRNA *prop)
{
/* Set additional flags for the F-Curve (i.e. only integer values). */
@ -309,11 +350,11 @@ static float nla_time_remap(const AnimationEvalContext *anim_eval_context,
}
/* Insert the specified keyframe value into a single F-Curve. */
static bool insert_keyframe_value(
static SingleKeyingResult insert_keyframe_value(
FCurve *fcu, float cfra, float curval, eBezTriple_KeyframeType keytype, eInsertKeyFlags flag)
{
if (!BKE_fcurve_is_keyframable(fcu)) {
return false;
return SingleKeyingResult::FCURVE_NOT_KEYFRAMEABLE;
}
/* Adjust coordinates for cycle aware insertion. */
@ -329,17 +370,19 @@ static bool insert_keyframe_value(
if (flag & INSERTKEY_NEEDED) {
if (!new_key_needed(fcu, cfra, curval)) {
return false;
return SingleKeyingResult::NO_KEY_NEEDED;
}
if (insert_vert_fcurve(fcu, {cfra, curval}, settings, flag) < 0) {
Sybren A. Stüvel commented 2024-02-01 15:37:14 +01:00
Outdated
Review
Copy Link

This code got popped out of the if (flag & INSERTKEY_NEEDED). Even though the new code might do the same thing, I would advice against making such changes in this PR. It's much easier to see what's going on when only the result type changes, and not the structure of the code itself.

This code got popped out of the `if (flag & INSERTKEY_NEEDED)`. Even though the new code might do the same thing, I would advice against making such changes in this PR. It's much easier to see what's going on when *only* the result type changes, and not the structure of the code itself.
👍 1
return false;
return SingleKeyingResult::FCURVE_NOT_KEYFRAMEABLE;
}
return true;
return SingleKeyingResult::SUCCESS;
}
return insert_vert_fcurve(fcu, {cfra, curval}, settings, flag) >= 0;
if (insert_vert_fcurve(fcu, {cfra, curval}, settings, flag) < 0) {
return SingleKeyingResult::FCURVE_NOT_KEYFRAMEABLE;
}
return SingleKeyingResult::SUCCESS;
}
bool insert_keyframe_direct(ReportList *reports,
@ -401,9 +444,9 @@ bool insert_keyframe_direct(ReportList *reports,
}
const float cfra = anim_eval_context->eval_time;
const bool success = insert_keyframe_value(fcu, cfra, current_value, keytype, flag);
const SingleKeyingResult result = insert_keyframe_value(fcu, cfra, current_value, keytype, flag);
if (!success) {
if (result != SingleKeyingResult::SUCCESS) {
BKE_reportf(reports,
RPT_ERROR,
"Failed to insert keys on F-Curve with path '%s[%d]', ensure that it is not "
@ -411,22 +454,21 @@ bool insert_keyframe_direct(ReportList *reports,
fcu->rna_path,
fcu->array_index);
}
return success;
return result == SingleKeyingResult::SUCCESS;
}
/** Find or create the FCurve based on the given path, and insert the specified value into it. */
static bool insert_keyframe_fcurve_value(Main *bmain,
ReportList *reports,
PointerRNA *ptr,
PropertyRNA *prop,
bAction *act,
const char group[],
const char rna_path[],
int array_index,
const float fcurve_frame,
float curval,
eBezTriple_KeyframeType keytype,
eInsertKeyFlags flag)
static SingleKeyingResult insert_keyframe_fcurve_value(Main *bmain,
PointerRNA *ptr,
PropertyRNA *prop,
bAction *act,
const char group[],
const char rna_path[],
int array_index,
const float fcurve_frame,
float curval,
eBezTriple_KeyframeType keytype,
eInsertKeyFlags flag)
{
/* Make sure the F-Curve exists.
* - if we're replacing keyframes only, DO NOT create new F-Curves if they do not exist yet
@ -439,7 +481,7 @@ static bool insert_keyframe_fcurve_value(Main *bmain,
/* We may not have a F-Curve when we're replacing only. */
if (!fcu) {
return false;
return SingleKeyingResult::CANNOT_CREATE_FCURVE;
}
const bool is_new_curve = (fcu->totvert == 0);
@ -454,23 +496,51 @@ static bool insert_keyframe_fcurve_value(Main *bmain,
/* Update F-Curve flags to ensure proper behavior for property type. */
update_autoflags_fcurve_direct(fcu, prop);
const bool success = insert_keyframe_value(fcu, fcurve_frame, curval, keytype, flag);
if (!success && reports != nullptr) {
BKE_reportf(reports,
RPT_ERROR,
"Failed to insert keys on F-Curve with path '%s[%d]', ensure that it is not "
"locked or sampled, and try removing F-Modifiers",
fcu->rna_path,
fcu->array_index);
}
const SingleKeyingResult result = insert_keyframe_value(
fcu, fcurve_frame, curval, keytype, flag);
/* If the curve is new, make it cyclic if appropriate. */
if (is_cyclic_action && is_new_curve) {
make_new_fcurve_cyclic(fcu, {act->frame_start, act->frame_end});
}
return success;
return result;
}
static void generate_keyframe_reports_from_result(ReportList *reports,
const CombinedKeyingResult &result)
{
std::string error = "Inserting keyframes failed due to the following reasons:";
if (result.get_count(SingleKeyingResult::CANNOT_CREATE_FCURVE) > 0) {
const int error_count = result.get_count(SingleKeyingResult::CANNOT_CREATE_FCURVE);
error.append(
fmt::format("\n- Could not create {} F-Curve{}. This can happen when only inserting to "
"available F-Curves.",
error_count,
error_count > 1 ? "s" : ""));
}
if (result.get_count(SingleKeyingResult::FCURVE_NOT_KEYFRAMEABLE) > 0) {
const int error_count = result.get_count(SingleKeyingResult::FCURVE_NOT_KEYFRAMEABLE);
if (error_count == 1) {
Sybren A. Stüvel commented 2024-02-05 11:05:14 +01:00
Outdated
Review
Copy Link

I think this text is slightly misleading. "Cannot create F-Curves" is a broad statement, which could be interpreted as "It was not possible to create any F-Curves". However, CANNOT_CREATE_FCURVE indicates that there was that many F-Curves that could not be created. There might have been more that could be created, but at this point that is not known.

Maybe something like Could not create {count} F-Curves… ?

For consistency, change Can happen to This can happen or This might happen.

I think this text is slightly misleading. "Cannot create F-Curves" is a broad statement, which could be interpreted as "It was not possible to create any F-Curves". However, `CANNOT_CREATE_FCURVE` indicates that there was that many F-Curves that could not be created. There might have been more that *could* be created, but at this point that is not known. Maybe something like `Could not create {count} F-Curves…` ? For consistency, change `Can happen` to `This can happen` or `This might happen`.
👍 1
error.append("\n- One F-Curve is not keyframeable. It might be locked or sampled.");
}
else {
error.append(fmt::format(
"\n- {} F-Curves are not keyframeable. They might be locked or sampled.", error_count));
}
}
if (result.get_count(SingleKeyingResult::NO_KEY_NEEDED) > 0) {
const int error_count = result.get_count(SingleKeyingResult::NO_KEY_NEEDED);
error.append(fmt::format(
"\n- Due to the setting 'Only Insert Needed', {} keyframe{} not been inserted.",
error_count,
error_count > 1 ? "s have" : " has"));
Sybren A. Stüvel commented 2024-02-05 11:06:06 +01:00
Outdated
Review
Copy Link

Same as above: for the text to be true, the code assumes that count(SUCCESS) == 0. That's not checked for, though.

Same as above: for the text to be true, the code assumes that `count(SUCCESS) == 0`. That's not checked for, though.
Christoph Lendenfeld commented 2024-02-06 09:44:57 +01:00
Outdated
Review
Copy Link

true, I check that outside the function but it makes sense not to assume that check here

true, I check that outside the function but it makes sense not to assume that check here
}
BKE_reportf(reports, RPT_ERROR, "%s", error.c_str());
}
int insert_keyframe(Main *bmain,
@ -540,6 +610,8 @@ int insert_keyframe(Main *bmain,
&force_all,
successful_remaps);
CombinedKeyingResult combined_result;
/* Key the entire array. */
int key_count = 0;
if (array_index == -1 || force_all) {
@ -551,20 +623,19 @@ int insert_keyframe(Main *bmain,
if (!successful_remaps[array_index]) {
continue;
}
if (insert_keyframe_fcurve_value(bmain,
reports,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag))
{
const SingleKeyingResult result = insert_keyframe_fcurve_value(bmain,
&ptr,
prop,
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-01-25 12:25:53 +01:00
Outdated
Review
Copy Link

Love that this turned into an increment. The previous code that just added the bool returned from insert_keyframe_fcurve_value() tripped me up for a bit, because I thought that must mean that insert_keyframe_fcurve_value() returned an int, which then made me think it might insert multiple keys, etc. This is way clearer to me, in addition to giving a better error handling flow. :-)

Love that this turned into an increment. The previous code that just added the `bool` returned from `insert_keyframe_fcurve_value()` tripped me up for a bit, because I thought that must mean that `insert_keyframe_fcurve_value()` returned an int, which then made me think it might insert multiple keys, etc. This is way clearer to me, in addition to giving a better error handling flow. :-)
❤️ 1
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
combined_result.add(result);
if (result == SingleKeyingResult::SUCCESS) {
key_count++;
exclude = array_index;
break;
@ -580,18 +651,21 @@ int insert_keyframe(Main *bmain,
}
if (array_index != exclude) {
key_count += insert_keyframe_fcurve_value(bmain,
reports,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
const SingleKeyingResult result = insert_keyframe_fcurve_value(bmain,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
combined_result.add(result);
if (result == SingleKeyingResult::SUCCESS) {
key_count++;
}
}
}
}
@ -603,36 +677,42 @@ int insert_keyframe(Main *bmain,
continue;
}
key_count += insert_keyframe_fcurve_value(bmain,
reports,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
const SingleKeyingResult result = insert_keyframe_fcurve_value(bmain,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
combined_result.add(result);
if (result == SingleKeyingResult::SUCCESS) {
key_count++;
}
}
}
}
/* Key a single index. */
else {
if (array_index >= 0 && array_index < values.size() && successful_remaps[array_index]) {
key_count += insert_keyframe_fcurve_value(bmain,
reports,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
const SingleKeyingResult result = insert_keyframe_fcurve_value(bmain,
&ptr,
prop,
act,
group,
rna_path,
array_index,
nla_mapped_frame,
values[array_index],
keytype,
flag);
combined_result.add(result);
if (result == SingleKeyingResult::SUCCESS) {
key_count++;
}
}
}
@ -647,6 +727,10 @@ int insert_keyframe(Main *bmain,
}
}
if (key_count == 0) {
generate_keyframe_reports_from_result(reports, combined_result);
}
return key_count;
}
@ -872,19 +956,18 @@ int insert_key_action(Main *bmain,
property_array_index++;
continue;
}
const bool inserted_key = insert_keyframe_fcurve_value(bmain,
nullptr,
ptr,
prop,
action,
group.c_str(),
rna_path.c_str(),
property_array_index,
frame,
value,
key_type,
insert_key_flag);
if (inserted_key) {
const SingleKeyingResult inserted_key = insert_keyframe_fcurve_value(bmain,
ptr,
prop,
action,
group.c_str(),
rna_path.c_str(),
property_array_index,
frame,
value,
key_type,
insert_key_flag);
if (inserted_key == SingleKeyingResult::SUCCESS) {
inserted_keys++;
}
property_array_index++;