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

Split off from #116943: Fix: Issue with Cycle aware keying when inserting second key

The keyframing functions up to this point created many
messages through the ReportList about what went wrong
when inserting keys.

This has 2 issues:

  • Everyone that wants to insert keyframes needs to have a ReportList which comes from bContext
  • The keyframing functions are very verbose. Providing the user with errors when there is actually nothing wrong.

To combat that, return a status enum from functions that deal with a single FCurve.
Functions that deal with more than one FCurve collect those statuses into a new class CombinedKeyingResult
Based on that we can create a single well worded error message,
instead of potentially flooding the user with individual messages.

The downside is that some info is lost, e.g. we do not know which exact FCurve has failed.

This will show a formatted error when inserting keys with a keying set or when pressing I over the property to keyframe.
But not when pressing I in the viewport. The latter can be fixed in a later patch.

Split off from [#116943: Fix: Issue with Cycle aware keying when inserting second key](https://projects.blender.org/blender/blender/pulls/116943) The keyframing functions up to this point created many messages through the `ReportList` about what went wrong when inserting keys. This has 2 issues: * Everyone that wants to insert keyframes needs to have a `ReportList` which comes from `bContext` * The keyframing functions are very verbose. Providing the user with errors when there is actually nothing wrong. To combat that, return a status enum from functions that deal with a single FCurve. Functions that deal with more than one FCurve collect those statuses into a new class `CombinedKeyingResult` Based on that we can create a single well worded error message, instead of potentially flooding the user with individual messages. The downside is that some info is lost, e.g. we do not know which exact FCurve has failed. This will show a formatted error when inserting keys with a keying set or when pressing `I` over the property to keyframe. But not when pressing `I` in the viewport. The latter can be fixed in a later patch.
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2024-01-23 15:17:24 +01:00
Christoph Lendenfeld added 1 commit 2024-01-23 15:17:29 +01:00
Christoph Lendenfeld changed title from WIP: Refactor: keyframing functions return more detailled status to WIP: Refactor: keyframing functions return more detailed status 2024-01-23 17:56:13 +01:00
Christoph Lendenfeld added 1 commit 2024-01-23 18:27:41 +01:00
Christoph Lendenfeld changed title from WIP: Refactor: keyframing functions return more detailed status to Refactor: keyframing functions return more detailed status 2024-01-23 18:45:58 +01:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-01-23 18:49:51 +01:00
Christoph Lendenfeld added 1 commit 2024-01-23 18:59:00 +01:00
Sybren A. Stüvel reviewed 2024-01-23 20:38:12 +01:00
@ -46,6 +46,15 @@
namespace blender::animrig {
enum KeyframingResult {

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`.
Author
Member

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
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-01-25 11:01:55 +01:00
Nathan Vegdahl requested changes 2024-01-25 12:28:25 +01:00
Nathan Vegdahl left a comment
Member

Looks really good! Just a simple one-line change that I think would improve things, but otherwise great.

Looks really good! Just a simple one-line change that I think would improve things, but otherwise great.
@ -550,6 +571,8 @@ int insert_keyframe(Main *bmain,
&force_all,
&successful_remaps);
keying_result::eResult keying_result = keying_result::SUCCESS;
Member

Maybe I'm misunderstanding the code, but shouldn't this be initialized to zero, since it's a bitfield that's collecting results? If there are no successes, that's probably useful information to pass up the call chain.

Maybe I'm misunderstanding the code, but shouldn't this be initialized to zero, since it's a bitfield that's collecting results? If there are no successes, that's probably useful information to pass up the call chain.
Author
Member

I have SUCCESS defined as 0
I could do keying_result::eResult keying_result = 0 but 0 on its own doesn't carry any meaning.

My reasoning was that if no error flag is set, the insertion was successful, meaning the return value is 0. Maybe that's too convoluted though and it needs a simpler solution

I have `SUCCESS` defined as 0 I could do `keying_result::eResult keying_result = 0` but 0 on its own doesn't carry any meaning. My reasoning was that if no error flag is set, the insertion was successful, meaning the return value is 0. Maybe that's too convoluted though and it needs a simpler solution
Member

Ah, got it. If this were only used to pass up the failure conditions of a single key frame insertion, then that would work well since success excludes the errors and vice-versa.

However, if I'm reading the code correctly, this is used to collect the results of multiple different key frame insertions. And I think for that, including every case (including success) as its own bit makes more sense. Because then it acts as a kind of summary of what happened. E.g. "There were some successes, but also these failures," might make sense to handle differently than, "Literally all of the insertions failed." Even if we aren't handling those cases differently right now, it's not much effort to make success a bit as well to support it in the future, if needed for e.g. better user-facing error messages.

Ah, got it. If this were only used to pass up the failure conditions of a single key frame insertion, then that would work well since success excludes the errors and vice-versa. However, if I'm reading the code correctly, this is used to collect the results of multiple different key frame insertions. And I think for that, including every case (including success) as its own bit makes more sense. Because then it acts as a kind of summary of what happened. E.g. "There were some successes, but also these failures," might make sense to handle differently than, "Literally all of the insertions failed." Even if we aren't handling those cases differently right now, it's not much effort to make success a bit as well to support it in the future, if needed for e.g. better user-facing error messages.
Author
Member

@nathanvegdahl good point. I've changed the code quite a bit to accommodate that.
I've added a KeyingResult class that tracks how many times which result has occurred.

One thing I am uncertain about is the memory allocation of the array I've used within that class.
Do I need to deallocate that in a destructor?

@nathanvegdahl good point. I've changed the code quite a bit to accommodate that. I've added a `KeyingResult` class that tracks how many times which result has occurred. One thing I am uncertain about is the memory allocation of the array I've used within that class. Do I need to deallocate that in a destructor?
Member

I've added a KeyingResult class that tracks how many times which result has occurred.

Oh, I like that approach, yeah. However, I think it's become a bit over-complicated due to trying to keep the enum involved. If we're doing to keep a count of the events, I would just drop the enum altogether and use a plain-old-data struct with fields for each case:

struct Keying Result {
  int success;
  int cannot_create_fcurve;
  int fcurve_not_keyframeable;
  int no_key_needed;

  // Convenience methods for various queries...
}

And then there's no need for a destructor at all, it's trivially copyable, and everything stays pretty lightweight.

> I've added a KeyingResult class that tracks how many times which result has occurred. Oh, I like that approach, yeah. However, I think it's become a bit over-complicated due to trying to keep the enum involved. If we're doing to keep a count of the events, I would just drop the enum altogether and use a plain-old-data struct with fields for each case: ``` struct Keying Result { int success; int cannot_create_fcurve; int fcurve_not_keyframeable; int no_key_needed; // Convenience methods for various queries... } ``` And then there's no need for a destructor at all, it's trivially copyable, and everything stays pretty lightweight.
Author
Member

My reason for having the list it that it simplifies adding and querying an enum value.
I would need to add a switch to almost every function if I am not dealing with a list.

wait a struct in C++ can have functions?

My reason for having the list it that it simplifies adding and querying an enum value. I would need to add a `switch` to almost every function if I am not dealing with a list. wait a `struct` in C++ can have functions?
Member

My reason for having the list it that it simplifies adding and querying an enum value.
I would need to add a switch to almost every function if I am not dealing with a list.

Not sure about switches (clarification: I'm suggesting nuking the enum entirely), but it does mean you would need to manually write out multiple fields for the methods that involve more than one. So for example, has_errors() would become:

bool has_errors() const
{
  return cannot_create_fcurve || fcurve_not_keyframeable || no_key_needed;
}

And would need to be kept up-to-date when additional fields are added.

On the other hand, add_result() and has_error() would disappear entirely, because you would just use the fields directly.

wait a struct in C++ can have functions?

Yup. The only difference between a struct and a class on a language level is the default visibility: class members are private by default, struct members are public. In practice, people don't usually put methods on structs, though. So maybe it actually should be a class, for clarity.

But personally, I think of structs as being for things that are (by design, rather than coincidence) plain-old-data with all public members, and classes being anything else. Rather than being about having methods or not.

> My reason for having the list it that it simplifies adding and querying an enum value. > I would need to add a switch to almost every function if I am not dealing with a list. Not sure about switches (clarification: I'm suggesting nuking the enum entirely), but it does mean you would need to manually write out multiple fields for the methods that involve more than one. So for example, `has_errors()` would become: ```c++ bool has_errors() const { return cannot_create_fcurve || fcurve_not_keyframeable || no_key_needed; } ``` And would need to be kept up-to-date when additional fields are added. On the other hand, `add_result()` and `has_error()` would disappear entirely, because you would just use the fields directly. > wait a struct in C++ can have functions? Yup. The only difference between a struct and a class on a language level is the default visibility: class members are private by default, struct members are public. In practice, people don't usually put methods on structs, though. So maybe it actually should be a class, for clarity. But personally, I think of structs as being for things that are (by design, rather than coincidence) plain-old-data with all public members, and classes being anything else. Rather than being about having methods or not.
Member

Another thought:

Although I still personally prefer the approach I outlined, if you prefer to stick with the enum + array approach (which is also fine), I'd say instead of blender::Array use std::array, which is essentially just a nicer C-style array (i.e. stack-allocated, rather than heap allocated, so it's data is stored directly in the struct). That way it's still plain-old-data and the question of destruction disappears.

Another thought: Although I still personally prefer the approach I outlined, if you prefer to stick with the enum + array approach (which is also fine), I'd say instead of `blender::Array` use `std::array`, which is essentially just a nicer C-style array (i.e. stack-allocated, rather than heap allocated, so it's data is stored directly in the struct). That way it's still plain-old-data and the question of destruction disappears.
nathanvegdahl marked this conversation as resolved
@ -605,0 +625,4 @@
flag);
keying_result |= result;
if (result == keying_result::SUCCESS) {
key_count++;
Member

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. :-)
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2024-02-01 12:49:59 +01:00
Christoph Lendenfeld added 1 commit 2024-02-01 14:50:36 +01:00
Sybren A. Stüvel requested changes 2024-02-01 15:59:23 +01:00
Sybren A. Stüvel left a comment
Member

Some feedback :)

Some feedback :)
@ -333,3 +367,2 @@
return KeyingResultOptions::NO_KEY_NEEDED;
}
if (insert_vert_fcurve(fcu, {cfra, curval}, settings, flag) < 0) {

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.
@ -46,6 +46,41 @@
namespace blender::animrig {
enum class KeyingResultOptions {

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:

  • KeyingResultOptionsSingleKeyingResult
  • KeyingResultAggregatedKeyingResult

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.
Author
Member

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
@ -49,0 +51,4 @@
CANNOT_CREATE_FCURVE,
FCURVE_NOT_KEYFRAMEABLE,
NO_KEY_NEEDED,
/* When adding an enum, make sure to update the constructor of `KeyingResult`. */

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.
Author
Member

really good idea.
Also removes the weird +1

really good idea. Also removes the weird `+1`
ChrisLend marked this conversation as resolved
@ -49,0 +56,4 @@
class KeyingResult {
private:
/* The index into the array is defined by `KeyingResultOptions`. */

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.
ChrisLend marked this conversation as resolved
@ -49,0 +57,4 @@
class KeyingResult {
private:
/* The index into the array is defined by `KeyingResultOptions`. */
std::array<int, (int(KeyingResultOptions::NO_KEY_NEEDED) + 1)> result_counter{0};

Rename to result_counter_ (trailing _ because private).

Rename to `result_counter_` (trailing `_` because `private`).
ChrisLend marked this conversation as resolved
@ -49,0 +60,4 @@
std::array<int, (int(KeyingResultOptions::NO_KEY_NEEDED) + 1)> result_counter{0};
public:
void add_result(const KeyingResultOptions result)

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.
ChrisLend marked this conversation as resolved
@ -49,0 +65,4 @@
result_counter[int(result)]++;
}
bool has_error(const KeyingResultOptions result) const

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.
Author
Member

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

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 :/
Author
Member

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

Yeah that seems to be the way to do it.

Yeah that seems to be the way to do it.
ChrisLend marked this conversation as resolved
@ -49,0 +72,4 @@
bool has_errors() const
{
for (int i = 1; i < result_counter.size(); i++) {

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; } } ```
Author
Member

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

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.
ChrisLend marked this conversation as resolved
Christoph Lendenfeld added 1 commit 2024-02-01 17:14:36 +01:00
Christoph Lendenfeld added 1 commit 2024-02-01 17:15:38 +01:00
Christoph Lendenfeld added 1 commit 2024-02-01 17:25:04 +01:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-02-01 17:29:07 +01:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-02-01 17:29:09 +01:00
Christoph Lendenfeld added 1 commit 2024-02-02 09:20:05 +01:00
Christoph Lendenfeld added 1 commit 2024-02-02 14:50:42 +01:00
Christoph Lendenfeld added 1 commit 2024-02-02 15:20:51 +01:00
Sybren A. Stüvel requested changes 2024-02-05 11:07:17 +01:00
Sybren A. Stüvel left a comment
Member

The code is getting nice! I just spend some time critically looking at the exact wording of the error messages.

The code is getting nice! I just spend some time critically looking at the exact wording of the error messages.
@ -484,0 +523,4 @@
std::string error = "Inserting keyframes failed due to the following reasons:";
if (result.get_count(SingleKeyingResult::CANNOT_CREATE_FCURVE) > 0) {
error.append(
"\n- Cannot create F-Curves. Can happen when only inserting to available F-Curves.");

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`.
@ -484,0 +537,4 @@
}
if (result.get_count(SingleKeyingResult::NO_KEY_NEEDED) > 0) {
error.append(
"\n- The setting 'Only Insert Needed' is enabled and no changes have been recorded.");

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.
Author
Member

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
Christoph Lendenfeld added 2 commits 2024-02-06 09:37:30 +01:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-02-06 09:46:12 +01:00
Sybren A. Stüvel reviewed 2024-02-06 17:13:03 +01:00
Sybren A. Stüvel left a comment
Member

The code looks good to me, but when I try to insert keys into locked FCurves, I still just get "Error: Failed to insert any keys". Is that expected?

The code looks good to me, but when I try to insert keys into locked FCurves, I still just get "Error: Failed to insert any keys". Is that expected?
Author
Member

The code looks good to me, but when I try to insert keys into locked FCurves, I still just get "Error: Failed to insert any keys". Is that expected?

yes because the codepath when pressing I is different. This PR will enable fixing that though

> The code looks good to me, but when I try to insert keys into locked FCurves, I still just get "Error: Failed to insert any keys". Is that expected? yes because the codepath when pressing `I` is different. This PR will enable fixing that though

In that case, please expand the PR description so that it's clear which key insertion operators will show the new messages, and which ones won't yet.

In that case, please expand the PR description so that it's clear which key insertion operators will show the new messages, and which ones won't yet.
Christoph Lendenfeld changed target branch from main to blender-v4.1-release 2024-02-08 10:57:01 +01:00
Author
Member

@dr.sybren done that. I've also changed the target branch to 4.1 assuming refactors can still land?

@dr.sybren done that. I've also changed the target branch to 4.1 assuming refactors can still land?

I tried this (example file attached):

  • loc/rot/scale keying set enabled
  • armature in pose mode
  • locked the selected bone's loc/rot/scale FCurves
  • unlocked a custom property's FCurve (as the I hotkey doesn't do anything when all FCurves are locked) this is where the example file was saved
  • press I in the graph editor. In the popup, choose 'All Channels'.

This shows the following message on the system console (stdout):

Error: Inserting keyframes failed due to the following reasons:
- Due to the setting 'Only Insert Needed', 1 keyframe has not been inserted.

This is strange though, as there are 9 loc/rot/scale FCurves that are all locked, so I'd expect the failure count to be higher. Also if there is 1 keyframe that wasn't inserted, it's unclear what happened to the other 8 channels. They didn't receive keyframes either (which is good, because they are locked) but the message is inconsistent with this.

This message is only shown in the system console, and not in the UI, but that's the same for what happens in main right now, so not an issue for this PR.

As for hovering over loc/rot/scale properties and pressing I, that almost works as I expected:

  • Given the example file:
  • Unlock the FCurve for loc_x
  • Keep the FCurve for loc_y
  • In the 3D Viewport side-panel, modify the bone's loc_x and loc_y properties.
  • Hover over loc_x and press I to key all of location x/y/z.

Since loc_x could be keyed succesfully, there is no message. However, I did expect some indicator that one of the keys could not be set. I'm guessing that the "success count > 0 ⇒ everything went fine" logic might need some tweakage. I'm careful here though; don't blindly do this .One could argue that the operation was indeed succesful, and that the still-orange colour of the loc_y property is enough of an indicator that not everything was keyed. That might be too weak though, especially since the FCurve can be locked even when the property itself is not (and it's the property lock that can be seen in the 3D Viewport side-panel, and not the FCurve lock).

I've also changed the target branch to 4.1 assuming refactors can still land?

Unfortunately, that's only for bugfixes :/

I tried this (example file attached): - loc/rot/scale keying set enabled - armature in pose mode - locked the selected bone's loc/rot/scale FCurves - unlocked a custom property's FCurve (as the `I` hotkey doesn't do anything when all FCurves are locked) **←** this is where the example file was saved - press `I` in the graph editor. In the popup, choose 'All Channels'. This shows the following message on the system console (stdout): ``` Error: Inserting keyframes failed due to the following reasons: - Due to the setting 'Only Insert Needed', 1 keyframe has not been inserted. ``` This is strange though, as there are 9 loc/rot/scale FCurves that are all locked, so I'd expect the failure count to be higher. Also if there is 1 keyframe that wasn't inserted, it's unclear what happened to the other 8 channels. They didn't receive keyframes either (which is good, because they are locked) but the message is inconsistent with this. This message is only shown in the system console, and not in the UI, but that's the same for what happens in `main` right now, so not an issue for this PR. As for hovering over loc/rot/scale properties and pressing `I`, that almost works as I expected: - Given the example file: - Unlock the FCurve for loc_x - Keep the FCurve for `loc_y` - In the 3D Viewport side-panel, modify the bone's `loc_x` and `loc_y` properties. - Hover over `loc_x` and press `I` to key all of location x/y/z. Since `loc_x` could be keyed succesfully, there is no message. However, I did expect some indicator that one of the keys could not be set. I'm guessing that the "success count > 0 ⇒ everything went fine" logic might need some tweakage. I'm careful here though; don't blindly do this .One could argue that the operation was indeed succesful, and that the still-orange colour of the `loc_y` property is enough of an indicator that not everything was keyed. That might be too weak though, especially since the FCurve can be locked even when the property itself is not (and it's the property lock that can be seen in the 3D Viewport side-panel, and not the FCurve lock). > I've also changed the target branch to 4.1 assuming refactors can still land? Unfortunately, that's [only for bugfixes](https://developer.blender.org/docs/handbook/release_process/release_branch/) :/
Author
Member

Unfortunately, that's only for bugfixes :/

In that case I'll put this refactor on ice and will try to get the bugfix in from which this was spawned off.
Will be a bit messy but that will need to be cleaned up after 4.1 has launched

This is strange though, as there are 9 loc/rot/scale FCurves that are all locked,

that's because the Graph Editor uses a keying set for that, which inserts each keyframe index individually. I was hoping to be able to use this refactor as a first step to extract the error message even further until it finally ends up at an operator function where we can meaningfully print a message to the user.
The behavior of multiple error messages is also present in 4.0 with this simple case:

  • on the default cube, insert location keys
  • lock x and y
  • on a different frame insert location keys again
  • in the "Info" panel you will see 2 error messages

The only difference is that those errors are covered by this other message, so the user won't see them.

About not notifying the user if some insertions failed.
I did that on purpose but there is no clear right way to do it.
From what I understand the animators don't want to be interrupted except something truly goes wrong.

> Unfortunately, that's only for bugfixes :/ In that case I'll put this refactor on ice and will try to get the bugfix in from which this was spawned off. Will be a bit messy but that will need to be cleaned up after 4.1 has launched > This is strange though, as there are 9 loc/rot/scale FCurves that are all locked, that's because the Graph Editor uses a keying set for that, which inserts each keyframe index individually. I was hoping to be able to use this refactor as a first step to extract the error message even further until it finally ends up at an operator function where we can meaningfully print a message to the user. The behavior of multiple error messages is also present in 4.0 with this simple case: * on the default cube, insert location keys * lock x and y * on a different frame insert location keys again * in the "Info" panel you will see 2 error messages The only difference is that those errors are covered by this other message, so the user won't see them. About not notifying the user if some insertions failed. I did that on purpose but there is no clear right way to do it. From what I understand the animators don't want to be interrupted except something truly goes wrong.
Christoph Lendenfeld changed target branch from blender-v4.1-release to main 2024-02-08 12:59:06 +01:00

In that case I'll put this refactor on ice and will try to get the bugfix in from which this was spawned off.
Will be a bit messy but that will need to be cleaned up after 4.1 has launched

👍

that's because the Graph Editor uses a keying set for that, which inserts each keyframe index individually. I was hoping to be able to use this refactor as a first step to extract the error message even further until it finally ends up at an operator function where we can meaningfully print a message to the user.

👍 in that case, I like this refactor even more :)

About not notifying the user if some insertions failed.
I did that on purpose but there is no clear right way to do it.
From what I understand the animators don't want to be interrupted except something truly goes wrong.

👍 let's keep the behaviour as it is in this PR, then.

As usual, excellent work Christoph! Toying around with this, and seeing the detailed error messages, really made it much clearer why Blender isn't doing what I tell it to do.

> In that case I'll put this refactor on ice and will try to get the bugfix in from which this was spawned off. > Will be a bit messy but that will need to be cleaned up after 4.1 has launched :+1: > that's because the Graph Editor uses a keying set for that, which inserts each keyframe index individually. I was hoping to be able to use this refactor as a first step to extract the error message even further until it finally ends up at an operator function where we can meaningfully print a message to the user. :+1: in that case, I like this refactor even more :) > About not notifying the user if some insertions failed. > I did that on purpose but there is no clear right way to do it. > From what I understand the animators don't want to be interrupted except something truly goes wrong. :+1: let's keep the behaviour as it is in this PR, then. As usual, excellent work Christoph! Toying around with this, and seeing the detailed error messages, really made it much clearer why Blender isn't doing what I tell it to do.
Sybren A. Stüvel approved these changes 2024-02-08 14:02:15 +01:00
Sybren A. Stüvel left a comment
Member

Accepting because I think the code is good. I'll leave it up to @ChrisLend to decide when to land this, given the aforementioned bugfix in blender-v4.1-release and the necessity to merge that branch back to main.

Accepting because I think the code is good. I'll leave it up to @ChrisLend to decide when to land this, given the aforementioned bugfix in `blender-v4.1-release` and the necessity to merge that branch back to `main`.
Nathan Vegdahl approved these changes 2024-02-09 10:05:09 +01:00
Nathan Vegdahl left a comment
Member

Thumbs up from me as well!

Thumbs up from me as well!
Christoph Lendenfeld added 1 commit 2024-03-07 17:00:25 +01:00
Christoph Lendenfeld added 1 commit 2024-03-07 17:11:50 +01:00
formatting
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
90e6d45787
Author
Member

resolved merge conflicts, one last build before landing it

@blender-bot build

resolved merge conflicts, one last build before landing it @blender-bot build
Christoph Lendenfeld added 1 commit 2024-03-07 17:53:33 +01:00
Christoph Lendenfeld merged commit 57419c9653 into main 2024-03-08 09:56:52 +01:00
Christoph Lendenfeld deleted branch refactor_keying_reports 2024-03-08 09:56:54 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117449
No description provided.