blender
/
blender
136
418
Fork 600
Code Issues 6.3k Pull Requests 781 Projects 19 Wiki Activity

Refactor: remove ReportList argument from insert_key #120784

Merged
Christoph Lendenfeld merged 4 commits from ChrisLend/blender:refactor_remove_reports_from_key_fn into main 2024-04-23 10:10:34 +02:00
Conversation 2 Commits 4 Files Changed 8 +24 -22
2 changed files with 24 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ad5e94ed7c - Show all commits

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

@ -130,8 +130,7 @@ void CombinedKeyingResult::generate_reports(ReportList *reports)
if (this->get_count(SingleKeyingResult::ID_NOT_EDITABLE) > 0) {
const int error_count = this->get_count(SingleKeyingResult::ID_NOT_EDITABLE);
if (error_count == 1) {
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl commented 2024-04-19 10:46:48 +02:00
Review
Copy Link

This pattern of printing one message if count == 1 and a different message otherwise is repeated over and over again here. So my first instinct was to try to factor that repetition out.

However, thinking about it more, I think it's likely that factoring it out wouldn't meaningfully simplify the code nor make it easier to understand or work with, and would instead just end up forcing people to jump around the file to follow the flow of the code. It's of course possible there's a good way to factor it out that I'm not thinking of, but I just wanted to leave an explicit note that as far as I can tell this particular case of repetition is probably fine as-is.

This pattern of printing one message if `count == 1` and a different message otherwise is repeated over and over again here. So my first instinct was to try to factor that repetition out. However, thinking about it more, I think it's likely that factoring it out wouldn't meaningfully simplify the code nor make it easier to understand or work with, and would instead just end up forcing people to jump around the file to follow the flow of the code. It's of course possible there's a good way to factor it out that I'm not thinking of, but I just wanted to leave an explicit note that as far as I can tell this particular case of repetition is probably fine as-is.
👍 1
errors.append(
fmt::format("{} ID has been skipped because it is not editable.", error_count));
errors.append("Inserting keys for one ID has been skipped because it is not editable.");
}
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl commented 2024-04-19 10:55:27 +02:00
Review
Copy Link

Maybe adding "Key insertion for" at the front of these strings would be good, so if it's the only error it's still explicit in the message itself that the error was about inserting key frames.

Also, for the count == 1 case I think it reads more clearly if you just skip using fmt::format() and replace {} with an explicit "1". Seeing the {} kept making me think, "Any number can go here... wait, that would read weird with anything other than 1... oh, right".

Maybe adding "Key insertion for" at the front of these strings would be good, so if it's the only error it's still explicit in the message itself that the error was about inserting key frames. Also, for the `count == 1` case I think it reads more clearly if you just skip using `fmt::format()` and replace `{}` with an explicit "1". Seeing the `{}` kept making me think, "Any number can go here... wait, that would read weird with anything other than 1... oh, right".
👍 1
Christoph Lendenfeld commented 2024-04-19 13:34:01 +02:00
Review
Copy Link

good point about the fmt with error_count 1.
Changed it to also spell out one so it is consistent with the previous error messages.

Instead of "Key insertion for" I went with "Inserting keys for". Felt a bit easier to understand for me

good point about the `fmt` with error_count 1. Changed it to also spell out `one` so it is consistent with the previous error messages. Instead of "Key insertion for" I went with "Inserting keys for". Felt a bit easier to understand for me
👍 1
else {
errors.append(
@ -142,8 +141,7 @@ void CombinedKeyingResult::generate_reports(ReportList *reports)
if (this->get_count(SingleKeyingResult::ID_NOT_ANIMATABLE) > 0) {
const int error_count = this->get_count(SingleKeyingResult::ID_NOT_ANIMATABLE);
if (error_count == 1) {
errors.append(
fmt::format("{} ID has been skipped because it cannot be animated.", error_count));
errors.append("Inserting keys for one ID has been skipped because it cannot be animated.");
}
else {
errors.append(
@ -154,8 +152,8 @@ void CombinedKeyingResult::generate_reports(ReportList *reports)
if (this->get_count(SingleKeyingResult::CANNOT_RESOLVE_PATH) > 0) {
const int error_count = this->get_count(SingleKeyingResult::CANNOT_RESOLVE_PATH);
if (error_count == 1) {
errors.append(fmt::format("{} ID has been skipped because the RNA path wasn't valid for it",
error_count));
errors.append(
"Inserting keys for one ID has been skipped because the RNA path wasn't valid for it");
}
else {
errors.append(fmt::format(

36
source/blender/animrig/intern/keyframing_auto.cc
View File

@ -99,11 +99,11 @@ bool autokeyframe_cfra_can_key(const Scene *scene, ID *id)
void autokeyframe_object(bContext *C, Scene *scene, Object *ob, Span<std::string> rna_paths)
{
ID *id = &ob->id;
if (id == nullptr) {
return;
}
BLI_assert(ob != nullptr);
BLI_assert(scene != nullptr);
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-04-19 11:36:46 +02:00
Outdated
Review
Copy Link

I think(?) this code path will never actually be taken, unless via some weird undefined behavior: the id field is just a struct directly inside the Object type, so taking its address should never be null.

However, in theory ob itself could be null. So we can check for that. But it looks like that isn't supposed to ever happen, so I would just make an assert for it (and possibly document it as an invariant in the function's doc comment, if it isn't already). Same with the C and scene parameters, for that matter.

I think(?) this code path will never actually be taken, unless via some weird undefined behavior: the `id` field is just a struct directly inside the `Object` type, so taking its address should never be null. However, in theory `ob` itself could be null. So we can check for that. But it looks like that isn't supposed to ever happen, so I would just make an assert for it (and possibly document it as an invariant in the function's doc comment, if it isn't already). Same with the `C` and `scene` parameters, for that matter.
Christoph Lendenfeld commented 2024-04-19 13:45:13 +02:00
Outdated
Review
Copy Link

yeah you are right. In that case I'll remove the check.

yeah you are right. In that case I'll remove the check.
BLI_assert(C != nullptr);
ID *id = &ob->id;
if (!autokeyframe_cfra_can_key(scene, id)) {
return;
}
@ -214,15 +214,16 @@ void autokeyframe_pose_channel(bContext *C,
Span<std::string> rna_paths,
short targetless_ik)
{
BLI_assert(C != nullptr);
BLI_assert(scene != nullptr);
BLI_assert(ob != nullptr);
BLI_assert(pose_channel != nullptr);
Main *bmain = CTX_data_main(C);
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-04-19 12:03:04 +02:00
Outdated
Review
Copy Link

Same here: I don't think(?) this code path is possible. Should be replaced by an assert that ob is non-null.

Same here: I don't think(?) this code path is possible. Should be replaced by an assert that `ob` is non-null.
👍 1
ID *id = &ob->id;
AnimData *adt = ob->adt;
bAction *act = (adt) ? adt->action : nullptr;
if (id == nullptr) {
return;
}
if (!blender::animrig::autokeyframe_cfra_can_key(scene, id)) {
return;
}
@ -274,14 +275,17 @@ void autokeyframe_pose_channel(bContext *C,
* NOTE: this will do constraints too, but those are ok to do here too?
*/
if (STREQ(pchan_name, pose_channel->name)) {
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-04-19 12:06:42 +02:00
Outdated
Review
Copy Link

Do we not need to accumulate and handle the returned CombinedKeyingResult here?

Do we not need to accumulate and handle the returned `CombinedKeyingResult` here?
Christoph Lendenfeld commented 2024-04-19 13:49:54 +02:00
Outdated
Review
Copy Link

good point, the function didn't handle the return argument before, but of course now we would be missing the messages

good point, the function didn't handle the return argument before, but of course now we would be missing the messages
insert_keyframe(bmain,
*id,
((fcu->grp) ? (fcu->grp->name) : (nullptr)),
fcu->rna_path,
fcu->array_index,
&anim_eval_context,
eBezTriple_KeyframeType(ts->keyframe_type),
flag);
CombinedKeyingResult result = insert_keyframe(bmain,
*id,
((fcu->grp) ? (fcu->grp->name) : (nullptr)),
fcu->rna_path,
fcu->array_index,
&anim_eval_context,
eBezTriple_KeyframeType(ts->keyframe_type),
flag);
if (result.get_count(SingleKeyingResult::SUCCESS) == 0) {
result.generate_reports(reports);
}
}
}
return;