Refactor: keyframing functions return more detailed status #117449

Christoph Lendenfeld · 2024-01-23T15:17:24+01:00

2024-01-23 15:17:24 +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

5cc77ecc25 wip

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

8c6071a6aa error messages

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

1e5d7a3eac move enum to cc file

Sybren A. Stüvel reviewed 2024-01-23 20:38:12 +01:00

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -46,6 +46,15 @@

				namespace blender::animrig {

				enum KeyframingResult {

Sybren A. Stüvel commented

2024-01-23 20:38:12 +01:00

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

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

b6d125df67 Merge branch 'main' into refactor_keying_reports

9d60d583e8 put enum into namespace

Nathan Vegdahl requested changes 2024-01-25 12:28:25 +01:00

Nathan Vegdahl left a comment

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -550,6 +571,8 @@ int insert_keyframe(Main *bmain,

				                                             &force_all,

				                                             &successful_remaps);

				  keying_result::eResult keying_result = keying_result::SUCCESS;

Nathan Vegdahl commented

2024-01-25 12:18:48 +01:00

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.

Christoph Lendenfeld commented

2024-01-25 12:44:05 +01:00

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

Nathan Vegdahl commented

2024-01-31 11:06:21 +01:00

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.

👍 1

Christoph Lendenfeld commented

2024-02-01 12:52:44 +01:00

@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?

Nathan Vegdahl commented

2024-02-01 13:03:34 +01:00

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.

Christoph Lendenfeld commented

2024-02-01 13:21:17 +01:00

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?

Nathan Vegdahl commented

2024-02-01 14:23:38 +01:00

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.

Nathan Vegdahl commented

2024-02-01 14:32:53 +01:00

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -605,0 +625,4 @@

				                                                                               flag);

				            keying_result |= result;

				            if (result == keying_result::SUCCESS) {

				              key_count++;

Nathan Vegdahl commented

2024-01-25 12:25:53 +01:00

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

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 2 commits 2024-02-01 12:49:59 +01:00

4aac11cd6d Merge branch 'main' into refactor_keying_reports

6dc1e56d7d make keying result a class

Christoph Lendenfeld added 1 commit 2024-02-01 14:50:36 +01:00

d3c7d91707 blender::array to std::array

Sybren A. Stüvel requested changes 2024-02-01 15:59:23 +01:00

Sybren A. Stüvel left a comment

Some feedback :)

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -333,3 +367,2 @@

				      return KeyingResultOptions::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

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -46,6 +46,41 @@

				namespace blender::animrig {

				enum class KeyingResultOptions {

Sybren A. Stüvel commented

2024-02-01 15:39:53 +01:00

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

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -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`. */

Sybren A. Stüvel commented

2024-02-01 15:41:03 +01:00

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

really good idea.
Also removes the weird +1

really good idea. Also removes the weird `+1`

ChrisLend marked this conversation as resolved

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -49,0 +56,4 @@

				class KeyingResult {

				 private:

				  /* The index into the array is defined by `KeyingResultOptions`. */

Sybren A. Stüvel commented

2024-02-01 15:42:46 +01:00

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -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};

Sybren A. Stüvel commented

2024-02-01 15:46:29 +01:00

Rename to result_counter_ (trailing _ because private).

Rename to `result_counter_` (trailing `_` because `private`).

ChrisLend marked this conversation as resolved

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -49,0 +60,4 @@

				  std::array<int, (int(KeyingResultOptions::NO_KEY_NEEDED) + 1)> result_counter{0};

				 public:

				  void add_result(const KeyingResultOptions result)

Sybren A. Stüvel commented

2024-02-01 15:44:16 +01:00

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

source/blender/animrig/intern/keyframing.cc

						
				@ -49,0 +65,4 @@

				    result_counter[int(result)]++;

				  }

				  bool has_error(const KeyingResultOptions result) const

Sybren A. Stüvel commented

2024-02-01 15:51:49 +01:00

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

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

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

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

Yeah that seems to be the way to do it.

ChrisLend marked this conversation as resolved

source/blender/animrig/intern/keyframing.cc

						
				@ -49,0 +72,4 @@

				  bool has_errors() const

				  {

				    for (int i = 1; i < result_counter.size(); i++) {

Sybren A. Stüvel commented

2024-02-01 15:45:45 +01:00

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

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

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

ChrisLend marked this conversation as resolved

Christoph Lendenfeld added 1 commit 2024-02-01 17:14:36 +01:00

e52a3ac1d8 sybrens feedback

Christoph Lendenfeld added 1 commit 2024-02-01 17:15:38 +01:00

5b5ec168d9 fix get_count

Christoph Lendenfeld added 1 commit 2024-02-01 17:25:04 +01:00

5db2d1f555 undo changes to INSERTKEY_NEEDED

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

302602f8ff fix warning about unused var

Christoph Lendenfeld added 1 commit 2024-02-02 14:50:42 +01:00

d8f8d8db75 Merge branch 'main' into refactor_keying_reports

Christoph Lendenfeld added 1 commit 2024-02-02 15:20:51 +01:00

583478e9f9 format result

Sybren A. Stüvel requested changes 2024-02-05 11:07:17 +01:00

Sybren A. Stüvel left a comment

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -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.");

Sybren A. Stüvel commented

2024-02-05 11:05:14 +01:00

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -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.");

Sybren A. Stüvel commented

2024-02-05 11:06:06 +01:00

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

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

03ea3b7b18 Merge branch 'main' into refactor_keying_reports

0994bf5ceb better error messages

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

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?

Christoph Lendenfeld commented

2024-02-06 18:58:09 +01:00

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

Sybren A. Stüvel commented

2024-02-08 10:28:33 +01:00

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.

ChrisLend changed target branch from main to blender-v4.1-release

2024-02-08 10:57:01 +01:00

Christoph Lendenfeld commented

2024-02-08 10:57:18 +01:00

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

Sybren A. Stüvel commented

2024-02-08 12:42:43 +01:00

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/) :/

117449-simple-example.blend

107 KiB

Christoph Lendenfeld commented

2024-02-08 12:58:14 +01:00

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.

ChrisLend changed target branch from blender-v4.1-release to main

2024-02-08 12:59:06 +01:00

Christoph Lendenfeld referenced this pull request

2024-02-08 13:08:00 +01:00

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

Sybren A. Stüvel commented

2024-02-08 14:01:02 +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

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

👍 1

Christoph Lendenfeld referenced this pull request

2024-02-08 15:04:02 +01:00

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

Nathan Vegdahl approved these changes 2024-02-09 10:05:09 +01:00

Nathan Vegdahl left a comment

Thumbs up from me as well!

👍 1

Christoph Lendenfeld added 1 commit 2024-03-07 17:00:25 +01:00

9cbbd51eb0 Merge branch 'main' into refactor_keying_reports

Christoph Lendenfeld added 1 commit 2024-03-07 17:11:50 +01:00