Drivers: implement fallback values for RNA path based variables. #110135
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110135
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "angavrilov/blender:pr-driver-var-default"
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?
As discussed in #105407, it can be useful to support returning
a fallback value specified by the user instead of failing the driver
if a driver variable cannot resolve its RNA path. This especially
applies to context variables referencing custom properties, since
when the object with the driver is linked into another scene, the
custom property can easily not exist there.
This patch adds an optional fallback value setting to properties
based on RNA path (including ordinary Single Property variables
due to shared code and similarity). When enabled, RNA path lookup
failures (including invalid array index) cause the fallback value
to be used instead of marking the driver invalid.
A flag is added to track when this happens for UI use. It is
also exposed to python for lint type scripts.
When the fallback value is used, the input field containing
the property RNA path that failed to resolve is highlighted in red
(identically to the case without a fallback), and the driver
can be included in the With Errors filter of the Drivers editor.
However, the channel name is not underlined in red, because
the driver as a whole evaluates successfully.
This PR will be pushed as a sequence of 3 commits separating major logical changes:
This comment has an attached file that demonstrates a hypothetical use case for this feature.
#105407 (comment)
Specifically, I was mainly thinking of an NPR lighting setup that replaces most of the standard lighting calculations (and/or uses the ad-hoc light group trick that uses R/G/B channels to separate lights into three groups instead of representing actual color), and thus needs direct access to custom light positions, colors etc from materials and actual lamp objects (they would still be needed for detecting shadows). Lighting data is obviously scene (or layer) global and should be in the context.
Without this feature drivers are less flexible than the View Layer Attribute nodes in materials, because Attributes already allow implementing defaults via the Alpha output (if the property has <4 channels, alpha behaves basically like a 'found' flag), and automatically search for the property in View Layer, Scene and World (to e.g. allow specifying the value per Scene or per View Layer at the scene layout maker's discretion rather than the rigger's). As shown in the screenshot below, defaults for drivers allow implementing those behaviors there as well.
The same applies to any scene-global settings, e.g. a hypothetical example by @Mets:
|
|
|
e085f61ab4
to1a7dc42620
@blender-bot package
Package build started. Download here when ready.
I am not sure about the following two things:
Why do we need the checkbox next to the default value? Can it be just a
Default Value
slider, with a default value of 0 (which matches the current implicit behavior, I believe). And then if rigger wants it to be different they just modify a single value.What is the flag which indicates the default value was actually used is used for.
Can please you elaborate on this?
It does not match the implicit behavior, because the implicit behavior marks the driver as invalid, which I think actually blocks it from subsequent evaluation. I.e. it is a hard error.
Internally it is used to still highlight the path in red, even though the invalid flag isn't applied. I also expose it to python api for scripts to use if they want (the invalid flag on the drivers is likewise exposed).
By the way, I wonder if a default fallback should include the driver in the drivers with errors filter or not? One one hand the lookup still fails, on the other hand the whole point is that it is 'expected' behavior (in the fallback example image it is truly expected for some lookups to always fail).
Another possible design for this would be for invalid variables to simply evaluate to
None
, which can then be checked for in aScripted Expression
driver:Not quite as user friendly, and certainly less discoverable. But depending on how often we expect use cases that need this to come up, it could make sense.
It can't really, it has to be a number. Simple Expressions only support floating point values.
You can use NaN though, you just need to allow entering it through the UI as the default value, and support isfinite() in simple expressions.
Ah, right.
It should be somehow clear in the interface that the default value is used, but it should not be displayed in the same way as error.
The way I see it, the fact that the path fails to resolve is the same so it's fine to highlight it as an error. Basically the default value feature is like a
try {} catch(NotResolved) { return default; }
in a way.I've put this on the itinerary to discuss at the next Animation & Rigging module meeting (Thursday 2023-07-27 18:00 CEST/Amsterdam time), because I think it would be good to get some feedback from riggers there on how they would want it to work.
It doesn't have to be a Simple Expression. My suspicion (though I could be wrong) is that only a minority of drivers would be set up this way, in which case the benefits of avoiding Python evaluation wouldn't be especially important.
Having said that, we'd still need a checkbox to distinguish driver variables that are "allowed" to be
None
from those that aren't. So I agree that it's probably not the way to go, since we'll be adding UI either way, and this approach is less user friendly.But it's always worth exploring alternative designs to help ensure that we land on the one with the most appropriate set of trade offs.
1a7dc42620
todb68e7bec9
Sergey asked me about this in DMs some time ago, here's what I said (also repeated in the Anim Module meeting):
Example use case (entirely hypothetical):
That said:
only_show_errors
. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good.Off-topic / Suggestions for separate patch:
Well basically that's the idea.
Also, without this feature drivers are less flexible than the View Layer Attribute nodes in materials, because Attributes already allow implementing defaults via the Alpha output (if the property has <4 channels, alpha behaves basically like a 'found' flag), and automatically search for the property in View Layer, Scene and World (to e.g. allow specifying the value per Scene or per View Layer at the scene layout maker's discretion rather than the rigger's). As shown in the screenshot above, defaults for drivers allow implementing those behaviors.
I don't have a 'production' use case (as in a real scene), but I made an artificial example:
#105407 (comment)
I was mainly thinking of an NPR lighting setup that replaces most of the standard lighting calculations (and/or uses the ad-hoc light group trick that uses R/G/B channels to separate lights into three groups instead of representing actual color), and thus needs direct access to custom light positions, colors etc from materials and actual lamp objects (they would still be needed for detecting shadows). Lighting data is obviously scene (or layer) global and should be in the context. This is basically a more concrete instance of your usage case idea.
In the driver UI itself the rna path field is highlighted in red when it can't be resolved. I made sure to preserve that even when default is triggered, so basically a red path with default enabled = default used.
I think it's a known bug that nobody fixed. It was there when I was adding Copy As New Driver. IIRC there is a UI context problem with popups, as in some kind of ambiguity between the context of the button that triggered the popup (needed to populate the popup obviously) and the context of the button you clicked in the popup itself. This needs attention of a UI expert who knows that stuff.
db68e7bec9
to0a1bdb8f0c
I implemented this: now Only Show Errors includes drivers with failed defaults. However this made it difficult to distinguish drivers that completely failed and those that were saved by defaults. So I made another UI behavior change as a separate commit included in this PR.
Specifically, @Sergey in chat said that channel names of failed drivers are underlined with a red line in the dopesheet. Actually, that isn't true. However, I thought that it makes sense to do that, and implemented it, applying it to completely failed drivers.
Thus:
I du not really remember mentioning this. Dopesheet does not display drivers.
In the Drivers editor, however, invalid drivers are high-lighted with a red underline. I see this in almost any production file. So there is definitely a mechanism for this. However, it is possible that a wrong RNA path on a variable does not trigger the driver to be considered failed and underlined with red.
To me it is a bis strange on a semantic level of the UI and intent of the feature.
If a "fallback" to use a default value considered an error then it feels that the default value is just there to help hiding the error, which is not really good.
If use of a default value is a valid way of defining your rigs, then it should not appear in the errors list.
I agree. I think it would nevertheless be good to have a way to quickly find drivers that are using default fall backs. So maybe it should be a separate filter?
I think having it a separate filter will indeed be a better and clearer fit to the UI.
I'm sure you meant the drivers editor, but you did say dopesheet: 😜
Also, from the code and actual behavior in a small test, invalid drivers (as in the driver variables or expression failed to evaluate) are definitely not highlighted like that. Only drivers on non-existant properties (actually F-Curves in general) get that highlighting currently.
I think a separate "filter" would be too much - I interpret that as being of equal status to the other three like Only Show Errors, including a button directly in the toolbar etc. However it could be an "option" of the Only Show Errors filter, available in the Filters dropdown.
Gah! Also, I forgot to report back after the animation & rigging module meeting. The two main things discussed were:
(Oops, forgot some stuff. Edited to add.)
0a1bdb8f0c
toe75bbac195
Drivers: implement default fallback values for RNA path based variables.to Drivers: implement fallback values for RNA path based variables.I decided to completely rebrand the feature from 'default' to 'fallback', because that's what it actually is - as signified by multiple UI and comment strings using 'default fallback'.
I don't think any kind of 'advanced section' is appropriate or useful, because it's already one line of UI. One thing I can do is hide the input field for the actual fallback value unless it's enabled (see attached).
First, that specific comment is completely off topic and irrelevant, because it misunderstands what 'fallback' and 'default' were about in the discussion. Also, I have already provided my example case both here and in that task multiple times.
Edit: I also added a filter option on whether to include drivers with fallbacks in Only Show Errors.
@blender-bot package
Package build started. Download here when ready.
I'm definitely liking this direction!
e75bbac195
to89923a2a18
Some feedback from testing:
Single Property
andContext Property
variable types. I think that's probably fine for now, but maybe that could be added in a follow up PR, for consistency.Just a partial review so far (haven't made it through all the code yet). So far it's looking good code-wise. Just one minor nit.
@ -294,1 +317,4 @@
else {
if (dtar_try_use_fallback(dtar)) {
ptr = PointerRNA_NULL;
*r_prop = NULL;
Use
nullptr
instead ofNULL
.Well, the fallback is provided for the case when resolving the RNA path fails. This is only meaningful for properties with an RNA path, and probably only really useful for Context Property, because for the other type you presumably are in control of what datablock it tries to access. I supported it for both mainly because they share most of the implementation code.
89923a2a18
toe6b81f0346
Is it not triggered if the ID pointer is null? Although even more niche, I could see that also being useful, if someone has a workflow where they append in a rig and then delete the parts they don't need. (Likely not for characters, but for articulated props, etc.) It also just feels like a pretty natural "why not?" kind of thing, where it might as well be available for all variable types for consistency.
Continuing my review. Still have more to go. So far, just nits. Looking good!
@ -1304,3 +1308,3 @@
if ((ads) && (ads->filterflag & ADS_FILTER_ONLY_ERRORS)) {
/* skip if no errors... */
if (fcurve_has_errors(fcu) == false) {
if (!fcurve_has_errors(fcu, ads->filterflag2 & ADS_FILTER_DRIVER_FALLBACK_AS_ERROR)) {
I think it would read a little better if
ads
was just passed tofcurve_has_errors()
itself, and then the flag check can be done in there.(Usually I would agree with keeping function parameters minimal in scope. But in this case I think there's a semantic change to
fcurve_has_errors()
, where it now effectively depends onads
anyway.)@ -743,0 +747,4 @@
if (dtar->options & DTAR_OPTION_USE_FALLBACK) {
uiLayout *row = uiLayoutRow(layout, true);
uiItemR(row, dtar_ptr, "use_fallback_value", UI_ITEM_NONE, "", ICON_NONE);
uiItemR(row, dtar_ptr, "fallback_value", UI_ITEM_NONE, nullptr, ICON_NONE);
Is there a semantic difference between passing
""
vsnullptr
touiItemR()
here? If not, let's be consistent. It looks like most calls touiItemR()
are passingnullptr
when there's no value.nullptr means default caption, while "" means no caption at all, so they are different.
@ -332,1 +335,4 @@
/** Driver Target options. */
typedef enum eDriverTarget_Options {
/** use the fallback value */
The comment here is fairly redundant with the name. Maybe add a tiny bit more detail, like:
Just a few more nits, but otherwise the code looks good to me.
Since I'm not super familiar with driver code, I'd like a review from at least one more person. @Sergey would you mind giving this a quick review?
One final change I'd like to see: rather than hiding the fallback checkbox etc. if the ID target isn't filled in, I think graying it out would be better. That makes it a little more discoverable and a little less jarring.
@ -658,0 +663,4 @@
prop,
"Variable Fallback As Error",
"Include drivers that relied on fallback values to evaluate some of their variables "
"into the Only Show Errors filter, even if the driver evaluation succeeded overall");
I'm struggling a bit to come up with something better, but maybe this reads a little better:
You changed it to "The Only Show Errors filter should include drivers that...", which reads like a recommendation, not a description. I think it's better to either stick with what you had, or switch to what I wrote (either is fine). This one is admittedly tricky to phase clearly and succinctly.
Tried yet another mix of wordings.
Seems about as good as we can do, I think. Only one minor nit: instead of "into", just "in" sounds more natural here.
@ -1974,0 +1977,4 @@
RNA_def_property_ui_text(prop,
"Use Fallback",
"Use the fallback value if the data path can't be resolved, instead of "
"triggering driver evaluation failure");
Maybe slightly better wording:
@ -1974,0 +1990,4 @@
RNA_def_property_boolean_negative_sdna(prop, nullptr, "flag", DTAR_FLAG_FALLBACK_USED);
RNA_def_property_clear_flag(prop, PROP_EDITABLE);
RNA_def_property_ui_text(
prop, "Is Fallback Used", "The last variable evaluation used the fallback value");
If I understand correctly, this isn't a setting but rather an indicator of what happened. Is that correct? If so, I think this might be a bit clearer as a description:
@ -62,2 +68,4 @@
}
}
else if (dtar && (dtar->flag & DTAR_FLAG_FALLBACK_USED)) {
driver_arg = PyFloat_FromDouble(dtar->defval);
Shouldn't the second part of the condition here be the same as on line 44:
dtar->options & DTAR_OPTION_USE_FALLBACK
? And then also setdtar->flag |= DTAR_FLAG_FALLBACK_USED
?I'm also wondering if there's a reasonable way to put the fallback case as a single guard condition, rather than spreading it out through the other cases here. Although I admit no really satisfying way pops out at me. But if we can, I think that would make the logic a littler easier to follow.
It was relying on side effects of
driver_get_variable_property
. I refactored it to return an enum value, and also do the index bounds check internally so that all fallback checking code is in fcurve_driver.cc. Now bpy_rna_driver.cc only checks the returned enum value.Yeah, this seems better to me. Although it did make me realize even more how sprawling and scattered throughout the source code the logic for drivers is in general.
A nit that I don't feel super strongly about, but I think might improve readability: since you're returning an enum now, it might be better to use a switch rather than an if-else chain. And as a side-benefit, you can then explicitly handle all the enum variants succinctly, and then we can get compiler warnings if an enum variant is ever added but not handled.
Converted to a switch.
The path field isn't shown without a configured datablock either. Thus the new UI follows the current behavior in my view.
Ah, that's fair. Let's leave it as you have it, then. If/when we add it to the other variable types, we can move it out then.
37c089ca8b
tof7f38d4b22
s/into/to/ and rebased
Looks good to me! But as I said earlier, I'd like at least one more person to review this (preferably with more familiarity with this area of Blender's code than me) before actually landing.
I appreciate the example file, especially since it shows the bigger picture. It's IMO too complex to serve as a simple example that we could use in release notes. It seems to show off features that are not necessarily related to this particular PR. Its complexity also make it hard to use for inspecting the dependency graph, as that'll also be quite complex as a result.
I've attached a more trivial example. It seems to work well, and AFAICS the code is in pretty good shape. I've left some inline notes.
Since this is a UI change, please include a screenshot that shows what this would look like.
@ -144,3 +153,1 @@
struct PointerRNA *r_ptr,
struct PropertyRNA **r_prop,
int *r_index);
eDriverVariablePropertyResult driver_get_variable_property(
It's a good idea to replace the
bool
return type with something more descriptive. This should be done in a separate commit, though, where first the existing functionality is moved to theenum
, and then in a 2nd commit the enum can be extended with theDRIVER_VAR_PROPERTY_FALLBACK
value for this particular feature.@ -4557,6 +4557,23 @@ static bool achannel_is_being_renamed(const bAnimContext *ac,
return false;
}
/** Check if the animation channel name should be underlined in red due to fatal errors. */
The word "fatal" can be removed -- if it were fatal, the Blender process would have terminated already.
@ -319,3 +319,4 @@
* (NOTE: these get reset every time the types change).
*/
short flag;
/** Single-bit user-visible toggles (not reset on type change). */
Document that this is of
eDriverTarget_Options
type.@ -329,2 +331,3 @@
int _pad1;
/* Fallback value to use with DTAR_OPTION_USE_FALLBACK. */
float defval;
Avoid shortened names,
float fallback_value
is much better, also because it's referred to as "fallback value" in other places as such as well.@ -658,0 +661,4 @@
RNA_def_property_ui_text(
prop,
"Variable Fallback As Error",
"Include drivers that relied on variable fallback values for their evaluation "
"variable fallback values" is a bit ambiguous; I think "any fallback values" would be better.
@ -658,0 +662,4 @@
prop,
"Variable Fallback As Error",
"Include drivers that relied on variable fallback values for their evaluation "
"in the Only Show Errors filter, even if the driver evaluation otherwise succeeded");
"if the driver evaluation otherwise succeeded" can be replaced by "when the driver evaluation succeeded".
The word "otherwise" might be a bit confusing, and AFAIK in this form the word "when" would be appropriate.
@ -1974,0 +1977,4 @@
RNA_def_property_ui_text(prop,
"Use Fallback",
"Use the fallback value if the data path can't be resolved, instead of "
"failing to evaluate the whole driver");
Since "failing a partial driver" is never possible, I think the word "whole" can be removed.
@ -41,0 +38,4 @@
anim_eval_context, driver, dvar, dtar, true, &ptr, &prop, &index))
{
case DRIVER_VAR_PROPERTY_SUCCESS:
if (prop) {
This code was already nested too deeply, and this change adds yet another indentation on top of that. I feel it should first be cleaned up. By replacing assignments to
driver_arg
by areturn
statement, and some reordering of the conditions, I think a lot of indentation can be avoided.For example, instead of having the entire function body indented like this:
it could just be:
You can't do this with a switch on enum. However I restructured the contents of the case branches.
b897fdce9a
to208186205e
208186205e
to06f9abb6d5
is_fallback_used
(found while making the new tests).LGTM! Just a small request, can be handled when landing the PR.
@ -146,6 +146,17 @@ bool driver_get_target_property(const DriverTargetContext *driver_target_context
return true;
}
static bool dtar_try_use_fallback(DriverTarget *dtar)
Since this function doesn't seem to actually try and use the fallback, I think the naming is slightly misleading. How about
dtar_should_use_fallback
instead?Only after reading the rest of the code, I realise that this function both acts as a query ('should use fallback?') and as an operation ('mark as fallback used').
This function is tightly bound to the places where it is called, as it expects that the caller actually returns the fallback value. Please document this assumption in the function docstring. Then the function name can stay as it is.
@ -4560,2 +4560,4 @@
}
/** Check if the animation channel name should be underlined in red due to errors. */
static bool achannel_is_broken(const bAnimListElem *ale)
Refactoring this into its own function makes it so much better to understand, thanks!
@ -57,3 +36,1 @@
}
}
else {
switch (driver_get_variable_property(
This is also much nicer than the nested
if
s :)06f9abb6d5
to4ccd1bec83