Drivers: implement fallback values for RNA path based variables. #110135

Merged
Alexander Gavrilov merged 3 commits from angavrilov/blender:pr-driver-var-default into main 2024-01-08 15:25:08 +01:00
13 changed files with 446 additions and 57 deletions

View File

@ -93,6 +93,7 @@ class GRAPH_PT_filters(DopesheetFilterPopoverBase, Panel):
def draw(self, context):
layout = self.layout
st = context.space_data
DopesheetFilterPopoverBase.draw_generic_filters(context, layout)
layout.separator()
@ -100,6 +101,12 @@ class GRAPH_PT_filters(DopesheetFilterPopoverBase, Panel):
layout.separator()
DopesheetFilterPopoverBase.draw_standard_filters(context, layout)
if st.mode == 'DRIVERS':
layout.separator()
col = layout.column(align=True)
col.label(text="Drivers:")
col.prop(st.dopesheet, "show_driver_fallback_as_error")
class GRAPH_PT_snapping(Panel):
bl_space_type = 'GRAPH_EDITOR'

View File

@ -134,16 +134,31 @@ struct DriverVar *driver_add_new_variable(struct ChannelDriver *driver);
float driver_get_variable_value(const struct AnimationEvalContext *anim_eval_context,
struct ChannelDriver *driver,
struct DriverVar *dvar);
typedef enum eDriverVariablePropertyResult {
/** The property reference has been succesfully resolved and can be accessed. */
DRIVER_VAR_PROPERTY_SUCCESS,
/** Evaluation should use the fallback value. */
DRIVER_VAR_PROPERTY_FALLBACK,
/** The target property could not be resolved. */
DRIVER_VAR_PROPERTY_INVALID,
/** The property was resolved (output parameters are set),
* but the array index is out of bounds. */
DRIVER_VAR_PROPERTY_INVALID_INDEX
} eDriverVariablePropertyResult;
/**
* Same as 'dtar_get_prop_val'. but get the RNA property.
*/
bool driver_get_variable_property(const struct AnimationEvalContext *anim_eval_context,
struct ChannelDriver *driver,
struct DriverVar *dvar,
struct DriverTarget *dtar,
struct PointerRNA *r_ptr,
struct PropertyRNA **r_prop,
int *r_index);
eDriverVariablePropertyResult driver_get_variable_property(
angavrilov marked this conversation as resolved Outdated

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 the enum, and then in a 2nd commit the enum can be extended with the DRIVER_VAR_PROPERTY_FALLBACK value for this particular feature.

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 the `enum`, and then in a 2nd commit the enum can be extended with the `DRIVER_VAR_PROPERTY_FALLBACK` value for this particular feature.
const struct AnimationEvalContext *anim_eval_context,
struct ChannelDriver *driver,
struct DriverVar *dvar,
struct DriverTarget *dtar,
bool allow_no_index,
struct PointerRNA *r_ptr,
struct PropertyRNA **r_prop,
int *r_index);
/**
* Check if the expression in the driver conforms to the simple subset.

View File

@ -146,6 +146,21 @@ bool driver_get_target_property(const DriverTargetContext *driver_target_context
return true;
}
/**

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.

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.
* Checks if the fallback value can be used, and if so, sets dtar flags to signal its usage.
* The caller is expected to immediately return the fallback value if this returns true.
*/
static bool dtar_try_use_fallback(DriverTarget *dtar)
{
if ((dtar->options & DTAR_OPTION_USE_FALLBACK) == 0) {
return false;
}
dtar->flag &= ~DTAR_FLAG_INVALID;
dtar->flag |= DTAR_FLAG_FALLBACK_USED;
return true;
}
/**
* Helper function to obtain a value using RNA from the specified source
* (for evaluating drivers).
@ -160,6 +175,8 @@ static float dtar_get_prop_val(const AnimationEvalContext *anim_eval_context,
return 0.0f;
}
dtar->flag &= ~DTAR_FLAG_FALLBACK_USED;
/* Get property to resolve the target from.
* Naming is a bit confusing, but this is what is exposed as "Prop" or "Context Property" in
* interface. */
@ -184,6 +201,10 @@ static float dtar_get_prop_val(const AnimationEvalContext *anim_eval_context,
if (!RNA_path_resolve_property_full(
&property_ptr, dtar->rna_path, &value_ptr, &value_prop, &index))
{
if (dtar_try_use_fallback(dtar)) {
return dtar->fallback_value;
}
/* Path couldn't be resolved. */
if (G.debug & G_DEBUG) {
CLOG_ERROR(&LOG,
@ -200,6 +221,10 @@ static float dtar_get_prop_val(const AnimationEvalContext *anim_eval_context,
if (RNA_property_array_check(value_prop)) {
/* Array. */
if (index < 0 || index >= RNA_property_array_length(&value_ptr, value_prop)) {
if (dtar_try_use_fallback(dtar)) {
return dtar->fallback_value;
}
/* Out of bounds. */
if (G.debug & G_DEBUG) {
CLOG_ERROR(&LOG,
@ -253,13 +278,15 @@ static float dtar_get_prop_val(const AnimationEvalContext *anim_eval_context,
return value;
}
bool driver_get_variable_property(const AnimationEvalContext *anim_eval_context,
ChannelDriver *driver,
DriverVar *dvar,
DriverTarget *dtar,
PointerRNA *r_ptr,
PropertyRNA **r_prop,
int *r_index)
eDriverVariablePropertyResult driver_get_variable_property(
const AnimationEvalContext *anim_eval_context,
ChannelDriver *driver,
DriverVar *dvar,
DriverTarget *dtar,
const bool allow_no_index,
PointerRNA *r_ptr,
PropertyRNA **r_prop,
int *r_index)
{
PointerRNA ptr;
PropertyRNA *prop;
@ -267,9 +294,11 @@ bool driver_get_variable_property(const AnimationEvalContext *anim_eval_context,
/* Sanity check. */
if (ELEM(nullptr, driver, dtar)) {
return false;
return DRIVER_VAR_PROPERTY_INVALID;
}
dtar->flag &= ~DTAR_FLAG_FALLBACK_USED;
/* Get RNA-pointer for the data-block given in target. */
const DriverTargetContext driver_target_context = driver_target_context_from_animation_context(
anim_eval_context);
@ -281,7 +310,7 @@ bool driver_get_variable_property(const AnimationEvalContext *anim_eval_context,
driver->flag |= DRIVER_FLAG_INVALID;
dtar->flag |= DTAR_FLAG_INVALID;
return false;
return DRIVER_VAR_PROPERTY_INVALID;
}
/* Get property to read from, and get value as appropriate. */
@ -293,6 +322,13 @@ bool driver_get_variable_property(const AnimationEvalContext *anim_eval_context,
/* OK. */
}
else {
if (dtar_try_use_fallback(dtar)) {
ptr = PointerRNA_NULL;
*r_prop = nullptr;
*r_index = -1;
return DRIVER_VAR_PROPERTY_FALLBACK;
}
/* Path couldn't be resolved. */
if (G.debug & G_DEBUG) {
CLOG_ERROR(&LOG,
@ -307,16 +343,38 @@ bool driver_get_variable_property(const AnimationEvalContext *anim_eval_context,
driver->flag |= DRIVER_FLAG_INVALID;
dtar->flag |= DTAR_FLAG_INVALID;
return false;
return DRIVER_VAR_PROPERTY_INVALID;
}
*r_ptr = ptr;
*r_prop = prop;
*r_index = index;
/* Verify the array index and apply fallback if appropriate. */
if (prop && RNA_property_array_check(prop)) {
if ((index < 0 && !allow_no_index) || index >= RNA_property_array_length(&ptr, prop)) {
if (dtar_try_use_fallback(dtar)) {
return DRIVER_VAR_PROPERTY_FALLBACK;
}
/* Out of bounds. */
if (G.debug & G_DEBUG) {
CLOG_ERROR(&LOG,
"Driver Evaluation Error: array index is out of bounds for %s -> %s (%d)",
ptr.owner_id->name,
dtar->rna_path,
index);
}
driver->flag |= DRIVER_FLAG_INVALID;
dtar->flag |= DTAR_FLAG_INVALID;
return DRIVER_VAR_PROPERTY_INVALID_INDEX;
}
}
/* If we're still here, we should be ok. */
dtar->flag &= ~DTAR_FLAG_INVALID;
return true;
return DRIVER_VAR_PROPERTY_SUCCESS;
}
static short driver_check_valid_targets(ChannelDriver *driver, DriverVar *dvar)

View File

@ -4559,6 +4559,23 @@ static bool achannel_is_being_renamed(const bAnimContext *ac,
return false;
}
angavrilov marked this conversation as resolved Outdated

The word "fatal" can be removed -- if it were fatal, the Blender process would have terminated already.

The word "fatal" can be removed -- if it were fatal, the Blender process would have terminated already.
/** 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!

Refactoring this into its own function makes it so much better to understand, thanks!
{
switch (ale->type) {
case ANIMTYPE_FCURVE:
case ANIMTYPE_NLACURVE: {
const FCurve *fcu = static_cast<const FCurve *>(ale->data);
/* The channel is disabled (has a bad rna path), or it's a driver that failed to evaluate. */
return (ale->flag & FCURVE_DISABLED) ||
(fcu->driver != nullptr && (fcu->driver->flag & DRIVER_FLAG_INVALID));
}
default:
return false;
}
}
float ANIM_UI_get_keyframe_scale_factor()
{
bTheme *btheme = UI_GetTheme();
@ -4742,7 +4759,7 @@ void ANIM_channel_draw(
UI_fontstyle_draw_simple(fstyle, offset, ytext, name, col);
/* draw red underline if channel is disabled */
if (ELEM(ale->type, ANIMTYPE_FCURVE, ANIMTYPE_NLACURVE) && (ale->flag & FCURVE_DISABLED)) {
if (achannel_is_broken(ale)) {
uint pos = GPU_vertformat_attr_add(
immVertexFormat(), "pos", GPU_COMP_F32, 2, GPU_FETCH_FLOAT);

View File

@ -1212,7 +1212,7 @@ static bool skip_fcurve_with_name(
*
* \return true if F-Curve has errors/is disabled
*/
static bool fcurve_has_errors(const FCurve *fcu)
static bool fcurve_has_errors(const FCurve *fcu, bDopeSheet *ads)
{
/* F-Curve disabled (path evaluation error). */
if (fcu->flag & FCURVE_DISABLED) {
@ -1238,6 +1238,12 @@ static bool fcurve_has_errors(const FCurve *fcu)
if (dtar->flag & DTAR_FLAG_INVALID) {
return true;
}
if ((dtar->flag & DTAR_FLAG_FALLBACK_USED) &&
(ads->filterflag2 & ADS_FILTER_DRIVER_FALLBACK_AS_ERROR))
{
return true;
}
}
DRIVER_TARGETS_LOOPER_END;
}
@ -1305,7 +1311,7 @@ static FCurve *animfilter_fcurve_next(bDopeSheet *ads,
/* error-based filtering... */
if ((ads) && (ads->filterflag & ADS_FILTER_ONLY_ERRORS)) {
/* skip if no errors... */
if (fcurve_has_errors(fcu) == false) {
if (!fcurve_has_errors(fcu, ads)) {
continue;
}
}

View File

@ -740,6 +740,20 @@ static bool graph_panel_drivers_poll(const bContext *C, PanelType * /*pt*/)
return graph_panel_context(C, nullptr, nullptr);
}
static void graph_panel_driverVar_fallback(uiLayout *layout,
const DriverTarget *dtar,
PointerRNA *dtar_ptr)
{
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);
nathanvegdahl marked this conversation as resolved Outdated

Is there a semantic difference between passing "" vs nullptr to uiItemR() here? If not, let's be consistent. It looks like most calls to uiItemR() are passing nullptr when there's no value.

Is there a semantic difference between passing `""` vs `nullptr` to `uiItemR()` here? If not, let's be consistent. It looks like most calls to `uiItemR()` are passing `nullptr` when there's no value.

nullptr means default caption, while "" means no caption at all, so they are different.

nullptr means default caption, while "" means no caption at all, so they are different.
}
else {
uiItemR(layout, dtar_ptr, "use_fallback_value", UI_ITEM_NONE, nullptr, ICON_NONE);
}
}
/* settings for 'single property' driver variable type */
static void graph_panel_driverVar__singleProp(uiLayout *layout, ID *id, DriverVar *dvar)
{
@ -761,12 +775,15 @@ static void graph_panel_driverVar__singleProp(uiLayout *layout, ID *id, DriverVa
/* rna path */
col = uiLayoutColumn(layout, true);
uiLayoutSetRedAlert(col, (dtar->flag & DTAR_FLAG_INVALID));
uiLayoutSetRedAlert(col, (dtar->flag & (DTAR_FLAG_INVALID | DTAR_FLAG_FALLBACK_USED)));
uiTemplatePathBuilder(col,
&dtar_ptr,
"data_path",
&root_ptr,
CTX_IFACE_(BLT_I18NCONTEXT_EDITOR_FILEBROWSER, "Path"));
/* Default value. */
graph_panel_driverVar_fallback(layout, dtar, &dtar_ptr);
}
}
@ -904,13 +921,16 @@ static void graph_panel_driverVar__contextProp(uiLayout *layout, ID *id, DriverV
/* Target Path */
{
uiLayout *col = uiLayoutColumn(layout, true);
uiLayoutSetRedAlert(col, (dtar->flag & DTAR_FLAG_INVALID));
uiLayoutSetRedAlert(col, (dtar->flag & (DTAR_FLAG_INVALID | DTAR_FLAG_FALLBACK_USED)));
uiTemplatePathBuilder(col,
&dtar_ptr,
"data_path",
nullptr,
CTX_IFACE_(BLT_I18NCONTEXT_EDITOR_FILEBROWSER, "Path"));
}
/* Default value. */
graph_panel_driverVar_fallback(layout, dtar, &dtar_ptr);
}
/* ----------------------------------------------------------------- */

View File

@ -827,6 +827,9 @@ typedef enum eDopeSheet_FilterFlag2 {
ADS_FILTER_NOHAIR = (1 << 3),
ADS_FILTER_NOPOINTCLOUD = (1 << 4),
ADS_FILTER_NOVOLUME = (1 << 5),
/** Include working drivers with variables using their fallback values into Only Show Errors. */
ADS_FILTER_DRIVER_FALLBACK_AS_ERROR = (1 << 6),
} eDopeSheet_FilterFlag2;
/* DopeSheet general flags */

View File

@ -312,13 +312,15 @@ typedef struct DriverTarget {
/** Rotation channel calculation type. */
char rotation_mode;
char _pad[7];
char _pad[5];
/**
* Flags for the validity of the target
* (NOTE: these get reset every time the types change).
*/
short flag;
/** Single-bit user-visible toggles (not reset on type change) from eDriverTarget_Options. */
angavrilov marked this conversation as resolved Outdated

Document that this is of eDriverTarget_Options type.

Document that this is of `eDriverTarget_Options` type.
short options;
/** Type of ID-block that this target can use. */
int idtype;
@ -327,9 +329,16 @@ typedef struct DriverTarget {
* This is a value of enumerator #eDriverTarget_ContextProperty. */
int context_property;
int _pad1;
/* Fallback value to use with DTAR_OPTION_USE_FALLBACK. */
float fallback_value;
angavrilov marked this conversation as resolved Outdated

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.

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.
} DriverTarget;
/** Driver Target options. */
typedef enum eDriverTarget_Options {
/** Use the fallback value when the target is invalid (rna_path cannot be resolved). */
angavrilov marked this conversation as resolved Outdated

The comment here is fairly redundant with the name. Maybe add a tiny bit more detail, like:

/* Use the fallback value when the target is invalid. */
The comment here is fairly redundant with the name. Maybe add a tiny bit more detail, like: ``` /* Use the fallback value when the target is invalid. */ ```
DTAR_OPTION_USE_FALLBACK = (1 << 0),
} eDriverTarget_Options;
/** Driver Target flags. */
typedef enum eDriverTarget_Flag {
/** used for targets that use the pchan_name instead of RNA path
@ -346,6 +355,9 @@ typedef enum eDriverTarget_Flag {
/** error flags */
DTAR_FLAG_INVALID = (1 << 4),
/** the fallback value was actually used */
DTAR_FLAG_FALLBACK_USED = (1 << 5),
} eDriverTarget_Flag;
/* Transform Channels for Driver Targets */

View File

@ -655,6 +655,16 @@ static void rna_def_dopesheet(BlenderRNA *brna)
prop, "Display Movie Clips", "Include visualization of movie clip related animation data");
RNA_def_property_ui_icon(prop, ICON_TRACKER, 0);
RNA_def_property_update(prop, NC_ANIMATION | ND_ANIMCHAN | NA_EDITED, nullptr);
prop = RNA_def_property(srna, "show_driver_fallback_as_error", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "filterflag2", ADS_FILTER_DRIVER_FALLBACK_AS_ERROR);
RNA_def_property_ui_text(
prop,
"Variable Fallback As Error",
"Include drivers that relied on any fallback values for their evaluation "
angavrilov marked this conversation as resolved Outdated

"variable fallback values" is a bit ambiguous; I think "any fallback values" would be better.

"variable fallback values" is a bit ambiguous; I think "any fallback values" would be better.
"in the Only Show Errors filter, even if the driver evaluation succeeded");
angavrilov marked this conversation as resolved Outdated

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

"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.
RNA_def_property_ui_icon(prop, ICON_RNA, 0);
nathanvegdahl marked this conversation as resolved Outdated

I'm struggling a bit to come up with something better, but maybe this reads a little better:

Include drivers in the Only Show Errors filter that relied on variable fallback values for their evaluation, even if their evaluation otherwise succeeded

I'm struggling a bit to come up with something better, but maybe this reads a little better: > Include drivers in the Only Show Errors filter that relied on variable fallback values for their evaluation, even if their evaluation otherwise succeeded

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.

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.

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.

Seems about as good as we can do, I think. Only one minor nit: instead of "into", just "in" sounds more natural here.
RNA_def_property_update(prop, NC_ANIMATION | ND_ANIMCHAN | NA_EDITED, nullptr);
}
static void rna_def_action_group(BlenderRNA *brna)

View File

@ -1974,6 +1974,28 @@ static void rna_def_drivertarget(BlenderRNA *brna)
RNA_def_property_ui_text(
prop, "Context Property", "Type of a context-dependent data-block to access property from");
RNA_def_property_update(prop, 0, "rna_DriverTarget_update_data");
prop = RNA_def_property(srna, "use_fallback_value", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "options", DTAR_OPTION_USE_FALLBACK);
RNA_def_property_ui_text(prop,
nathanvegdahl marked this conversation as resolved Outdated

Maybe slightly better wording:

"Use a fallback value if the data path can't be resolved, instead of failing to evaluate"

Maybe slightly better wording: > "Use a fallback value if the data path can't be resolved, instead of failing to evaluate"

Since "failing a partial driver" is never possible, I think the word "whole" can be removed.

Since "failing a partial driver" is never possible, I think the word "whole" can be removed.
"Use Fallback",
"Use the fallback value if the data path can't be resolved, instead of "
"failing to evaluate the driver");
RNA_def_property_update(prop, 0, "rna_DriverTarget_update_data");
prop = RNA_def_property(srna, "fallback_value", PROP_FLOAT, PROP_NONE);
RNA_def_property_float_sdna(prop, nullptr, "fallback_value");
RNA_def_property_ui_text(
prop, "Fallback", "The value to use if the data path can't be resolved");
RNA_def_property_update(prop, 0, "rna_DriverTarget_update_data");
prop = RNA_def_property(srna, "is_fallback_used", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "flag", DTAR_FLAG_FALLBACK_USED);
nathanvegdahl marked this conversation as resolved Outdated

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:

Indicates that the most recent 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: > Indicates that the most recent variable evaluation used the fallback value
RNA_def_property_clear_flag(prop, PROP_EDITABLE);
RNA_def_property_ui_text(
prop,
"Is Fallback Used",
"Indicates that the most recent variable evaluation used the fallback value");
}
static void rna_def_drivervar(BlenderRNA *brna)

View File

@ -12,6 +12,8 @@
#include "MEM_guardedalloc.h"
#include "DNA_anim_types.h"
#include "BLI_utildefines.h"
#include "BKE_fcurve_driver.h"
@ -27,45 +29,44 @@ PyObject *pyrna_driver_get_variable_value(const AnimationEvalContext *anim_eval_
DriverVar *dvar,
DriverTarget *dtar)
{
PyObject *driver_arg = nullptr;
PointerRNA ptr;
PropertyRNA *prop = nullptr;
int index;
if (driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) {
if (prop) {
if (index != -1) {
if (index < RNA_property_array_length(&ptr, prop) && index >= 0) {
/* object, property & index */
driver_arg = pyrna_array_index(&ptr, prop, index);
}
else {
/* out of range, pass */
}
}
else {
/* object & property */
const PropertyType type = RNA_property_type(prop);
if (type == PROP_ENUM) {
/* Note that enum's are converted to strings by default,
* we want to avoid that, see: #52213 */
driver_arg = PyLong_FromLong(RNA_property_enum_get(&ptr, prop));
}
else {
driver_arg = pyrna_prop_to_py(&ptr, prop);
}
}
}
else {
switch (driver_get_variable_property(

This is also much nicer than the nested ifs :)

This is also much nicer than the nested `if`s :)
anim_eval_context, driver, dvar, dtar, true, &ptr, &prop, &index))
{
case DRIVER_VAR_PROPERTY_SUCCESS:
/* object only */
driver_arg = pyrna_struct_CreatePyObject(&ptr);
}
}
else {
/* can't resolve path, pass */
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 a return 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:

if (driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) {
  // ... all the code sits here
}

it could just be:

if (!driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) {
  return nullptr;
}
// ... all the code sits here
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 a `return` 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: ```cpp if (driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) { // ... all the code sits here } ``` it could just be: ```cpp if (!driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) { return nullptr; } // ... all the code sits here ```

You can't do this with a switch on enum. However I restructured the contents of the case branches.

You can't do this with a switch on enum. However I restructured the contents of the case branches.
return pyrna_struct_CreatePyObject(&ptr);
}
/* object, property & index */
if (index >= 0) {
return pyrna_array_index(&ptr, prop, index);
}
/* object & property (enum) */
if (RNA_property_type(prop) == PROP_ENUM) {
/* Note that enum's are converted to strings by default,
* we want to avoid that, see: #52213 */
return PyLong_FromLong(RNA_property_enum_get(&ptr, prop));
}
/* object & property */
return pyrna_prop_to_py(&ptr, prop);
case DRIVER_VAR_PROPERTY_FALLBACK:
return PyFloat_FromDouble(dtar->fallback_value);
case DRIVER_VAR_PROPERTY_INVALID:
case DRIVER_VAR_PROPERTY_INVALID_INDEX:
/* can't resolve path, pass */
return nullptr;
}
return driver_arg;
return nullptr;
}
nathanvegdahl marked this conversation as resolved Outdated

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 set dtar->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.

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

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.

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.

Converted to a switch.
PyObject *pyrna_driver_self_from_anim_rna(PathResolvedRNA *anim_rna)

View File

@ -364,6 +364,13 @@ add_blender_test(
--python ${CMAKE_CURRENT_LIST_DIR}/bl_animation_armature.py
)
add_blender_test(
bl_animation_drivers
--python ${CMAKE_CURRENT_LIST_DIR}/bl_animation_drivers.py
--
--testdir "${TEST_SRC_DIR}/animation"
)
add_blender_test(
bl_animation_fcurves
--python ${CMAKE_CURRENT_LIST_DIR}/bl_animation_fcurves.py

View File

@ -0,0 +1,211 @@
# SPDX-FileCopyrightText: 2020-2023 Blender Authors
#
# SPDX-License-Identifier: GPL-2.0-or-later
import unittest
import bpy
import pathlib
import sys
from rna_prop_ui import rna_idprop_quote_path
"""
blender -b -noaudio --factory-startup --python tests/python/bl_animation_drivers.py -- --testdir /path/to/lib/tests/animation
"""
class AbstractEmptyDriverTest:
def setUp(self):
super().setUp()
bpy.ops.wm.read_homefile(use_factory_startup=True)
self.obj = bpy.data.objects['Cube']
def assertPropValue(self, prop_name, value):
self.assertEqual(self.obj[prop_name], value)
def _make_context_driver(obj, prop_name, ctx_type, ctx_path, index=None, fallback=None, force_python=False):
obj[prop_name] = 0
fcu = obj.driver_add(rna_idprop_quote_path(prop_name), -1)
drv = fcu.driver
if force_python:
# Expression that requires full python interpreter
drv.type = 'SCRIPTED'
drv.expression = '[var][0]'
else:
drv.type = 'SUM'
var = drv.variables.new()
var.name = "var"
var.type = 'CONTEXT_PROP'
tgt = var.targets[0]
tgt.context_property = ctx_type
tgt.data_path = rna_idprop_quote_path(ctx_path) + (f"[{index}]" if index is not None else "")
if fallback is not None:
tgt.use_fallback_value = True
tgt.fallback_value = fallback
return fcu
def _is_fallback_used(fcu):
return fcu.driver.variables[0].targets[0].is_fallback_used
class ContextSceneDriverTest(AbstractEmptyDriverTest, unittest.TestCase):
""" Ensure keying things by name or with a keying set adds the right keys. """
def setUp(self):
super().setUp()
bpy.context.scene["test_property"] = 123
def test_context_valid(self):
fcu = _make_context_driver(
self.obj, 'test_valid', 'ACTIVE_SCENE', 'test_property')
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertPropValue('test_valid', 123)
def test_context_invalid(self):
fcu = _make_context_driver(
self.obj, 'test_invalid', 'ACTIVE_SCENE', 'test_property_bad')
bpy.context.view_layer.update()
self.assertFalse(fcu.driver.is_valid)
def test_context_fallback(self):
fcu = _make_context_driver(
self.obj, 'test_fallback', 'ACTIVE_SCENE', 'test_property_bad', fallback=321)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertTrue(_is_fallback_used(fcu))
self.assertPropValue('test_fallback', 321)
def test_context_fallback_valid(self):
fcu = _make_context_driver(
self.obj, 'test_fallback_valid', 'ACTIVE_SCENE', 'test_property', fallback=321)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertFalse(_is_fallback_used(fcu))
self.assertPropValue('test_fallback_valid', 123)
def test_context_fallback_python(self):
fcu = _make_context_driver(
self.obj, 'test_fallback_py', 'ACTIVE_SCENE', 'test_property_bad', fallback=321, force_python=True)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertTrue(_is_fallback_used(fcu))
self.assertPropValue('test_fallback_py', 321)
class ContextSceneArrayDriverTest(AbstractEmptyDriverTest, unittest.TestCase):
""" Ensure keying things by name or with a keying set adds the right keys. """
def setUp(self):
super().setUp()
bpy.context.scene["test_property"] = [123, 456]
def test_context_valid(self):
fcu = _make_context_driver(
self.obj, 'test_valid', 'ACTIVE_SCENE', 'test_property', index=0)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertPropValue('test_valid', 123)
def test_context_invalid(self):
fcu = _make_context_driver(
self.obj, 'test_invalid', 'ACTIVE_SCENE', 'test_property', index=2)
bpy.context.view_layer.update()
self.assertFalse(fcu.driver.is_valid)
def test_context_fallback(self):
fcu = _make_context_driver(
self.obj, 'test_fallback', 'ACTIVE_SCENE', 'test_property', index=2, fallback=321)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertTrue(_is_fallback_used(fcu))
self.assertPropValue('test_fallback', 321)
def test_context_fallback_valid(self):
fcu = _make_context_driver(
self.obj, 'test_fallback_valid', 'ACTIVE_SCENE', 'test_property', index=0, fallback=321)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertFalse(_is_fallback_used(fcu))
self.assertPropValue('test_fallback_valid', 123)
def test_context_fallback_python(self):
fcu = _make_context_driver(
self.obj, 'test_fallback_py', 'ACTIVE_SCENE', 'test_property', index=2, fallback=321, force_python=True)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertTrue(_is_fallback_used(fcu))
self.assertPropValue('test_fallback_py', 321)
def _select_view_layer(index):
bpy.context.window.view_layer = bpy.context.scene.view_layers[index]
class ContextViewLayerDriverTest(AbstractEmptyDriverTest, unittest.TestCase):
""" Ensure keying things by name or with a keying set adds the right keys. """
def setUp(self):
super().setUp()
bpy.ops.scene.view_layer_add(type='COPY')
scene = bpy.context.scene
scene.view_layers[0]['test_property'] = 123
scene.view_layers[1]['test_property'] = 456
_select_view_layer(0)
def test_context_valid(self):
fcu = _make_context_driver(
self.obj, 'test_valid', 'ACTIVE_VIEW_LAYER', 'test_property')
_select_view_layer(0)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertPropValue('test_valid', 123)
_select_view_layer(1)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertPropValue('test_valid', 456)
def test_context_fallback(self):
del bpy.context.scene.view_layers[1]['test_property']
fcu = _make_context_driver(
self.obj, 'test_fallback', 'ACTIVE_VIEW_LAYER', 'test_property', fallback=321)
_select_view_layer(0)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertFalse(_is_fallback_used(fcu))
self.assertPropValue('test_fallback', 123)
_select_view_layer(1)
bpy.context.view_layer.update()
self.assertTrue(fcu.driver.is_valid)
self.assertTrue(_is_fallback_used(fcu))
self.assertPropValue('test_fallback', 321)
def main():
global args
import argparse
if '--' in sys.argv:
argv = [sys.argv[0]] + sys.argv[sys.argv.index('--') + 1:]
else:
argv = sys.argv
parser = argparse.ArgumentParser()
parser.add_argument('--testdir', required=True, type=pathlib.Path)
args, remaining = parser.parse_known_args(argv)
unittest.main(argv=remaining)
if __name__ == "__main__":
main()