Refactor: keyframing functions return more detailed status #117449
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117449
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ChrisLend/blender:refactor_keying_reports"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 wrongwhen inserting keys.
This has 2 issues:
ReportList
which comes frombContext
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.WIP: Refactor: keyframing functions return more detailled statusto WIP: Refactor: keyframing functions return more detailed statusWIP: Refactor: keyframing functions return more detailed statusto Refactor: keyframing functions return more detailed status@ -46,6 +46,15 @@
namespace blender::animrig {
enum KeyframingResult {
This will pollute the namespace, as it'll create
blender::animrig::SUCESS
. Better to use anenum class
, as that limits the scope toblender::animrig::KeyframingResult::SUCESS
.I've put the enum into a namespace to combat that.
enum class
doesn't work with bitwise logicMaybe this needs a completely different solution. Probably worth waiting for the result of https://devtalk.blender.org/t/developer-discussion-handling-errors/33054/5
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;
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.
I have
SUCCESS
defined as 0I 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
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.
@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?
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:
And then there's no need for a destructor at all, it's trivially copyable, and everything stays pretty lightweight.
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?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:And would need to be kept up-to-date when additional fields are added.
On the other hand,
add_result()
andhas_error()
would disappear entirely, because you would just use the fields directly.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.
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
usestd::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.@ -605,0 +625,4 @@
flag);
keying_result |= result;
if (result == keying_result::SUCCESS) {
key_count++;
Love that this turned into an increment. The previous code that just added the
bool
returned frominsert_keyframe_fcurve_value()
tripped me up for a bit, because I thought that must mean thatinsert_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. :-)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.@ -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:
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:
When read out of context, it's unclear that
keying_result
andresult
are of different semantic levels. Something likeaggregated_result
vsresult
might work well, although it is a bit long.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 ofint(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.really good idea.
Also removes the weird
+1
@ -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.@ -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_
becauseprivate
).@ -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 nameAggregatedKeyingResult
and the parameter.@ -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:has_error(SUCCESS)
is weird.Without this, I don't think it's worth going through the efforts of using the array and keeping the separate counts.
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 :/
done that, had to include
../../../extern/fmtlib/include
in theCMakeLists.txt
for thatLet me know if that is ok
Yeah that seems to be the way to do it.
@ -49,0 +72,4 @@
bool has_errors() const
{
for (int i = 1; i < result_counter.size(); i++) {
This can be simplified to:
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 thoughHmmm 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.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
toThis can happen
orThis 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.true, I check that outside the function but it makes sense not to assume that check here
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 thoughIn 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.
@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):
I
hotkey doesn't do anything when all FCurves are locked) ← this is where the example file was savedI
in the graph editor. In the popup, choose 'All Channels'.This shows the following message on the system console (stdout):
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:loc_y
loc_x
andloc_y
properties.loc_x
and pressI
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 theloc_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).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
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:
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.
👍
👍 in that case, I like this refactor even more :)
👍 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.
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 tomain
.Thumbs up from me as well!
resolved merge conflicts, one last build before landing it
@blender-bot build