Anim: Detailed report if no keyframes have been inserted #119201
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
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119201
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ChrisLend/blender:return_keying_result"
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?
With this PR, when pressing
I
in the viewport and the codeis unable to insert ANY keyframes, the user will be presented
with a single message detailing exactly why it has failed.
This PR promotes the functionality introduced in
#117449: Refactor: keyframing functions return more detailed status
into the header file so it can be used elsewhere.
The
CombinedKeyingResult
class is returnedfrom
insert_key_action
andinsert_key_rna
, andused to produce a single report from the operator if it
failed to insert any keyframes.
In order to easily create a report from a
CombinedKeyingResult
the function
generate_keyframe_reports_from_result
has been moved into the class as
generate_reports
.In addition to that the
UNABLE_TO_INSERT_TO_NLA_STACK
resulthas been added. This notifies the user if keyframe insertion is not
possible due to NLA stack settings.
WIP: Anim: Detailled report if no keyframes have been insertedto WIP: Anim: Detailed report if no keyframes have been insertedWIP: Anim: Detailed report if no keyframes have been insertedto Anim: Detailed report if no keyframes have been inserted@blender-bot build
@ -196,3 +232,3 @@
* \param keying_mask is expected to have the same size as `rna_path`. A false bit means that index
* will be skipped.
* \returns The number of keys inserted.
* \returns How often keyframe insertion was successful how often it failed for which reason.
How often keyframe insertion was successful
and
how often it failed/
for which reason.Same for the copies below, of course.
@ -85,0 +75,4 @@
{
/* For loop starts at 1 to skip the SUCCESS flag. Assumes that SUCCESS is 0 and the rest of the
* enum are sequential values. */
for (int i = 1; i < result_counter.size(); i++) {
For a future improvement: adding a
static_assert(SingleKeyingResult::SUCCESS == 0)
could be a nice touch here.decided to do that right away, imo this makes it more obvious what the code is expecting
@ -89,0 +83,4 @@
return false;
}
void CombinedKeyingResult::generate_reports(ReportList *reports)
Below this code is called when
get_count(animrig::SingleKeyingResult::SUCCESS) == 0
. However, this function does not take that case into account. The success count could theoretically be zero because nothing happened, in which case the generated report will end with "due to the following reasons:", which will be rather strange.It's probably best to have a
bool has_reported_error
that's set totrue
in each specificif (error count > 0)
block. That way you can check at the bottom of the function and, if it'sfalse
, add a report that explains the situation. That will ensure that something is shown, even when a configuration occurs that isn't (yet) supported by this function.@ -369,0 +377,4 @@
anim_eval_context));
}
if (combined_result.get_count(animrig::SingleKeyingResult::SUCCESS) == 0) {
This is a bit of a weird thing to check for, given that there is a function
has_errors()
. I can imagine it's necessary, as there are cases in the code above where a call toinsert_key_rna()
is skipped, and thus it wouldn't register as error on thecombined_result
.I think the weirdness comes from the use of the mechanism to track key insertion errors to track key insertion skips. I think it's better to just use
combined_result.has_errors()
here, as the skips already have their own calls toBKE_report
.using
has_errors()
is not the same as checking if there was at least one success.The reason I am checking for 0 successes is that we don't want to spam the user with messages. E.g. there might be an FCurve that is locked and the user is aware of that, still when we use
has_errors
he would get an error popup every single time he wants to insert a key.Edit:
what I did now is to explicitly check for an empty selection list and report a warning + exit early. That makes it clearer what happened.
I think the new code is indeed better. Still, for future devs it might be a good idea to add a comment that explains why
.has_errors()
is not being used there.@ -89,0 +135,4 @@
}
if (has_reported_error) {
BKE_reportf(reports, RPT_ERROR, "%s", error.c_str());
BKE_report(reports, RPT_ERROR, error.c_str());
@ -339,0 +341,4 @@
}
if (BLI_listbase_is_empty(&selection)) {
BKE_reportf(op->reports, RPT_WARNING, "Nothing selected to keyframe");
In the rest of this PR "keyframe" is only used as a noun. Maybe "Nothing selected to key" is better?
I think the text of the report could use a little bit more polish, if only to remove some repetition of the words "due to".
Also there's a bit of the good old singular-vs-plural going on, where a list of only one thing is a bit clunky.
How about this:
bool has_reported_error
, put the errors in aVector<string>
, and without the-
prefix.I think this'll simplify the common case ,where there is a single reason for the insertion failure.
Changed it so if there is only one error type, it prints solely that.
In the case of multiple error types it shows them in a list like before, with the difference that the intro text has been modified to
"Inserting keyframes failed:"
Thanks for the feedback. Much better now
👍