Drivers: Introduce the Context Properties #105132

Merged
Sergey Sharybin merged 3 commits from Sergey/blender:driver_variable into main 2023-03-06 16:02:00 +01:00

The goal: allow accessing context dependent data, such as active scene camera
without linking to a specific scene data-block. This is useful in cases when,
for example, geometry node setup needs to be aware of the camera position.

A possible work-around without changes like this is to have some scene
evaluation hook which will update driver variables for the currently evaluating
scene. But this raises an issue of linking: it is undesirable that the asset
scene is linked to the shot file.
Surely, it is possible to have post-evaluation handler to clear the variables,
but it all starts to be quite messy. Not to mention possible threading
conflicts.

Another possibility of introducing a way to achieve the goal is to make it so
the dependency graph somehow parses the python expression where artists can
(and already are trying to) type something like:

depsgraph.scene.camera.matrix_world.col[3][0]

But this is not only tricky to implement properly and reliably, it hits two
limitations:

  • Currently dependency graph can only easily resolve dependencies to a RNA
    property.

  • Some properties access which are valid in Python are not considered valid
    RNA properties by the existing property resolution functions:

    camera.matrix_world[3][0] is a valid RNA property, but
    camera.matrix_world.col[3][0] is not.

Using driver variables allows to have visual feedback when the path resolution
fails, and there is no way to visualize errors in the python expression itself.

This change introduces the new variable type: Context Property. Using this
variable type makes allows to choose between Active Scene and Active View
Layer. These scene and view layer are resolved during the driver evaluation
time, based on the current dependency graph.

This allows to create a driver variable in the following configuration:

  • Type: Context Property
  • Context Property: Active Scene
  • Path: camera.matrix_world[3][0]

The naming is a bit confusing. Tried my best to keep it clear keeping two
aspects in mind: using UI naming when possible, and follow the existing
naming.

A lot of the changes are related on making it so the required data is available
from the variable evaluation functions. It wasn't really clear what the data
would be, and the scope of the changes, so it is done together with the
functional changes.

It seems that there is some variable evaluation logic duplicated in the
bpy_rna_driver.c. This change does not change it. It is not really clear why
this separate code path with much more limited scope of supported target types
is even needed.

There is also a possible change in the behavior of the dependency graph: it
is now using ID of the resolved path when building driver variables. It used
to use the variable ID. In common cases they match, but when going into nested
data-blocks it is actually correct to use relation to the resolved ID. Not sure
if there was some code to ensure that, which now can be resolved. Also not sure
whether it is still needed to ensure the ID specified in the driver target is
build as well. Intuitively it is not needed.

The simple demo file is attached in the PR.

The goal: allow accessing context dependent data, such as active scene camera without linking to a specific scene data-block. This is useful in cases when, for example, geometry node setup needs to be aware of the camera position. A possible work-around without changes like this is to have some scene evaluation hook which will update driver variables for the currently evaluating scene. But this raises an issue of linking: it is undesirable that the asset scene is linked to the shot file. Surely, it is possible to have post-evaluation handler to clear the variables, but it all starts to be quite messy. Not to mention possible threading conflicts. Another possibility of introducing a way to achieve the goal is to make it so the dependency graph somehow parses the python expression where artists can (and already are trying to) type something like: depsgraph.scene.camera.matrix_world.col[3][0] But this is not only tricky to implement properly and reliably, it hits two limitations: - Currently dependency graph can only easily resolve dependencies to a RNA property. - Some properties access which are valid in Python are not considered valid RNA properties by the existing property resolution functions: `camera.matrix_world[3][0]` is a valid RNA property, but `camera.matrix_world.col[3][0]` is not. Using driver variables allows to have visual feedback when the path resolution fails, and there is no way to visualize errors in the python expression itself. This change introduces the new variable type: Context Property. Using this variable type makes allows to choose between Active Scene and Active View Layer. These scene and view layer are resolved during the driver evaluation time, based on the current dependency graph. This allows to create a driver variable in the following configuration: - Type: Context Property - Context Property: Active Scene - Path: camera.matrix_world[3][0] The naming is a bit confusing. Tried my best to keep it clear keeping two aspects in mind: using UI naming when possible, and follow the existing naming. A lot of the changes are related on making it so the required data is available from the variable evaluation functions. It wasn't really clear what the data would be, and the scope of the changes, so it is done together with the functional changes. It seems that there is some variable evaluation logic duplicated in the `bpy_rna_driver.c`. This change does not change it. It is not really clear why this separate code path with much more limited scope of supported target types is even needed. There is also a possible change in the behavior of the dependency graph: it is now using ID of the resolved path when building driver variables. It used to use the variable ID. In common cases they match, but when going into nested data-blocks it is actually correct to use relation to the resolved ID. Not sure if there was some code to ensure that, which now can be resolved. Also not sure whether it is still needed to ensure the ID specified in the driver target is build as well. Intuitively it is not needed. The simple demo file is attached in the PR.
Sergey Sharybin added this to the Animation & Rigging project 2023-02-23 15:51:42 +01:00
Sergey Sharybin added the
Module
Animation & Rigging
Interest
Animation & Rigging
labels 2023-02-23 15:51:49 +01:00
Sergey Sharybin requested review from Sybren A. Stüvel 2023-02-23 15:51:55 +01:00
Sergey Sharybin force-pushed driver_variable from 0eacd09101 to e30fad939c 2023-02-23 16:46:41 +01:00 Compare
Sybren A. Stüvel requested changes 2023-02-27 15:02:45 +01:00
Sybren A. Stüvel left a comment
Member

Overall I think the idea is fine. Do have some questions though, as inline comments.

Overall I think the idea is fine. Do have some questions though, as inline comments.
@ -60,2 +62,4 @@
struct ChannelDriver *fcurve_copy_driver(const struct ChannelDriver *driver);
/**
* Get property from which the specific property can be found found from.

Double "from"

Double "from"
Sergey marked this conversation as resolved
@ -62,0 +65,4 @@
* Get property from which the specific property can be found found from.
*
* For the Single Property the `r_prop` is a pointer to an ID, form which the target rna_path is
* accessed from.

form which the target rna_path is accessed from.

Typo & extra "from". Probably clearer to write something like "which is used to resolve the target rna_path".

> form which the target rna_path is accessed from. Typo & extra "from". Probably clearer to write something like "which is used to resolve the target rna_path".
Sergey marked this conversation as resolved
@ -62,0 +75,4 @@
* property for Active Scene, or a data property for Active View Layer.
*
* If the target property can not be resolved false is returned.
*/

For this comment block, I think it might be clearer to have a one-sentence introduction to explain that the behaviour depends on the dvar type. That way the "For the xxx Property" structure is easier to understand from the get-go.

For this comment block, I think it might be clearer to have a one-sentence introduction to explain that the behaviour depends on the `dvar` type. That way the "For the xxx Property" structure is easier to understand from the get-go.
Sergey marked this conversation as resolved
@ -62,0 +80,4 @@
struct Scene *scene;
struct ViewLayer *view_layer;
} DriverTargetContext;
bool driver_get_target_property(const DriverTargetContext *context,

I think that context isn't the best name, dt_context would already be better, because it's clear that it's not a regular bContext *.

I think that `context` isn't the best name, `dt_context` would already be better, because it's clear that it's not a regular `bContext *`.
Sergey marked this conversation as resolved
@ -76,0 +90,4 @@
return target_context;
}
/* Specialized implementation of driver_get_target_property() sued for the

sued -> used

sued -> used
Sergey marked this conversation as resolved
@ -76,0 +102,4 @@
return true;
case DTAR_CONTEXT_PROPERTY_ACTIVE_VIEW_LAYER: {
// XXX: Deeds implementation.

Why does this "deed" implementation? Above it just creates the RNA pointer, so why is this case different?

Why does this "deed" implementation? Above it just creates the RNA pointer, so why is this case different?
Author
Owner

Leftoevr from the WIP. I put XXX: Needs implementation in areas which is not the primary focus to get initial goal to be reached, so that it is easier to find missing bits in the patch later. Deeds I don't know how such typo happened.

The RNA property is created just the line below, so I just forgot to remove the comment.

Leftoevr from the WIP. I put `XXX: Needs implementation` in areas which is not the primary focus to get initial goal to be reached, so that it is easier to find missing bits in the patch later. `Deeds` I don't know how such typo happened. The RNA property is created just the line below, so I just forgot to remove the comment.
Sergey marked this conversation as resolved
@ -76,0 +113,4 @@
/* Reset to a NULL RNA pointer.
* This allows to more gracefully handle issues with unsupported configuration (forward
* compatibility. for example). */
/* TODO(sergey): Is there an utility function for this? */

I don't think there is a function for this. RNA_id_pointer_create(NULL, r_property_ptr); would work, but IMO that goes directly against the create in the function name.

I don't think there is a function for this. `RNA_id_pointer_create(NULL, r_property_ptr);` would work, but IMO that goes directly against the `create` in the function name.
Author
Owner

Ok, will modify the TODO to replace the code with a function call once that function is actually implemented :)

Ok, will modify the TODO to replace the code with a function call once that function is actually implemented :)
Sergey marked this conversation as resolved
@ -1203,0 +1216,4 @@
build_id(target_id);
build_driver_id_property(target_prop, dtar->rna_path);

I think removing some of the empty lines will make the code read a bit better:

BLI_assert(target_prop.owner_id);

ID *target_id = target_prop.owner_id;
build_id(target_id);
build_driver_id_property(target_prop, dtar->rna_path);
I think removing some of the empty lines will make the code read a bit better: ```cpp BLI_assert(target_prop.owner_id); ID *target_id = target_prop.owner_id; build_id(target_id); build_driver_id_property(target_prop, dtar->rna_path); ```
Author
Owner

To me the

BLI_assert(target_prop.owner_id);

ID *target_id = target_prop.owner_id;

build_id(target_id);
build_driver_id_property(target_prop, dtar->rna_path);

Is more readable. And is how it is structured in the relations builder.

To me the ``` BLI_assert(target_prop.owner_id); ID *target_id = target_prop.owner_id; build_id(target_id); build_driver_id_property(target_prop, dtar->rna_path); ``` Is more readable. And is how it is structured in the relations builder.
Sergey marked this conversation as resolved
@ -216,3 +217,3 @@
virtual void build_driver(ID *id, FCurve *fcurve, int driver_index);
virtual void build_driver_variables(ID *id, FCurve *fcurve);
virtual void build_driver_id_property(ID *id, const char *rna_path);
virtual void build_driver_id_property(const PointerRNA &target_prop, const char *rna_path);

I find the naming a bit confusing. Before I always understood this to be regarding the property pointed to by rna_path when resolving against id. Now target_prop seems to already point to a property, given its name, and not to an ID. But what does the rna_path point to, then? The same property?

In the implementation it seems that this patch just pulls the PointerRNA creation out of this function. But, in the mean time it also renames the local id_ptr variable to target_prop, so there must be some other change as well that I don't quite understand yet.

I find the naming a bit confusing. Before I always understood this to be regarding the property pointed to by `rna_path` when resolving against `id`. Now `target_prop` seems to already point to a property, given its name, and not to an `ID`. But what does the `rna_path` point to, then? The same property? In the implementation it seems that this patch just pulls the `PointerRNA` creation out of this function. But, in the mean time it also renames the local `id_ptr` variable to `target_prop`, so there must be some other change as well that I don't quite understand yet.
Author
Owner

rna_path is a path relative to a property. Before this change it was always relative to some explicitly provided ID, now it can be some implicitly calculated ID or a non-ID data (like view layer).

It doesn't seem i can attach image in the inlined comment, so I'll move this bit of discussion to the "real" comment.

`rna_path` is a path relative to a property. Before this change it was always relative to some explicitly provided ID, now it can be some implicitly calculated ID or a non-ID data (like view layer). It doesn't seem i can attach image in the inlined comment, so I'll move this bit of discussion to the "real" comment.
Sergey marked this conversation as resolved
Author
Owner

On the topic of the target_prop and rna_path.

I agree the naming is confusing. I was trying to stay within the existing concepts and their scope. Al lthose levels of indirections are not help here at all :(

Lets look at the attached screenshot.

The var is a variable (DriverVar in the C types, typical variable name dvar). It has a single target (DriverTarget in the C types, typical variable name dtar). The target has a Prop (as it is called in the interface, internally it is a combination of ID *id and int idtype;), and a RNA Path.

In the example on the image the Prop is camera, Path is the matrix_world[3][0].

This directly translates to the arguments of the build_driver_id_property:

  • target_prop is Prop, and in the example it is camera
  • rna_path is Path, and in the example it is matrix_world[3][0]

I guess what we can do is the following:

  • Add a plain text comment explaining that target_prop corresponds to the Prop in the interface
  • Rename rna_path to something like rna_path_from_target_prop
  • ...
  • Find a volunteer to apply this to the fcurve_driver.c ;)

Not sure how I feel about referring to the UI from the code. It is kinda fragile, but I can not think of a more clear way of explaining what it example is.

If you have other ideas please share them :)

On the topic of the `target_prop` and `rna_path`. I agree the naming is confusing. I was trying to stay within the existing concepts and their scope. Al lthose levels of indirections are not help here at all :( Lets look at the attached screenshot. The `var` is a variable (`DriverVar` in the C types, typical variable name `dvar`). It has a single target (`DriverTarget` in the C types, typical variable name `dtar`). The target has a `Prop` (as it is called in the interface, internally it is a combination of `ID *id` and `int idtype;`), and a RNA Path. In the example on the image the `Prop` is camera, `Path` is the `matrix_world[3][0]`. This directly translates to the arguments of the `build_driver_id_property`: - `target_prop` is `Prop`, and in the example it is `camera` - `rna_path` is `Path`, and in the example it is `matrix_world[3][0]` I guess what we can do is the following: - Add a plain text comment explaining that `target_prop` corresponds to the `Prop` in the interface - Rename `rna_path` to something like `rna_path_from_target_prop` - ... - Find a volunteer to apply this to the `fcurve_driver.c` ;) Not sure how I feel about referring to the UI from the code. It is kinda fragile, but I can not think of a more clear way of explaining what it example is. If you have other ideas please share them :)
Sergey Sharybin added 1 commit 2023-02-28 11:51:21 +01:00
0ad4d5eb5b Cleanup: spelling and formatting
Should address majority of the current notes from Sybren in the PR.

Thanks for the explanation. There are a lot of indirections and properties of variables of targets with pointers to ..... indeed.

I guess what we can do is the following:

  • Add a plain text comment explaining that target_prop corresponds to the Prop in the interface
  • Rename rna_path to something like rna_path_from_target_prop

👍 LGTM.

  • Find a volunteer to apply this to the fcurve_driver.c ;)

Hah yeah I can help with that.

Not sure how I feel about referring to the UI from the code. It is kinda fragile, but I can not think of a more clear way of explaining what it example is.

I agree on both counts (fragile + the best we have right now). I think it's already a big improvement over the current situation in the main branch.

Thanks for the explanation. There are a lot of indirections and properties of variables of targets with pointers to ..... indeed. > I guess what we can do is the following: > - Add a plain text comment explaining that `target_prop` corresponds to the `Prop` in the interface > - Rename `rna_path` to something like `rna_path_from_target_prop` :+1: LGTM. > - Find a volunteer to apply this to the `fcurve_driver.c` ;) Hah yeah I can help with that. > Not sure how I feel about referring to the UI from the code. It is kinda fragile, but I can not think of a more clear way of explaining what it example is. I agree on both counts (fragile + the best we have right now). I think it's already a big improvement over the current situation in the `main` branch.
Sergey Sharybin added 1 commit 2023-02-28 15:29:31 +01:00
Sybren A. Stüvel approved these changes 2023-03-03 14:47:12 +01:00
Sybren A. Stüvel left a comment
Member

👍 thanks! Much clearer now :)

:+1: thanks! Much clearer now :)
  • Type: Context Property
  • Context Property: Active Scene
  • Path: camera.matrix_world[3][0]

I'm a bit late here, but re camera specifically: can't it be overridden from the Video Sequencer? What happens in that case - does it change scene.camera temporarily, or work in some other way?

Basically, maybe you also need Active Camera that would handle that situation correctly.

> - Type: Context Property > - Context Property: Active Scene > - Path: camera.matrix_world[3][0] I'm a bit late here, but re camera specifically: can't it be overridden from the Video Sequencer? What happens in that case - does it change scene.camera temporarily, or work in some other way? Basically, maybe you also need Active Camera that would handle that situation correctly.
Author
Owner

That is an interesting point.

My thoughts on it:

  • I am not sure how praxctical it is for animation/rigging. Maybe it is, maybe not so much. I'd love to make smaller incremental steps and add support for more things later on.
  • There are already some assumptions in few places w.r.t active camera: i.e. the dependency graph builder assumes scene->camera is the source of truth for the active camera. So, the "Active Camera" accessor might end up in an interesting digging.
That is an interesting point. My thoughts on it: - I am not sure how praxctical it is for animation/rigging. Maybe it is, maybe not so much. I'd love to make smaller incremental steps and add support for more things later on. - There are already some assumptions in few places w.r.t active camera: i.e. the dependency graph builder assumes `scene->camera` is the source of truth for the active camera. So, the "Active Camera" accessor might end up in an interesting digging.

Another thought: I wonder if these (especially in the view layer mode) should support a default value option, which would be used if the property path cannot be resolved instead of causing an error. This could be used to implement settings that are intended to be overridden only for some layers.

Edit: I think the ability to detect if the property exists and provide defaults is one major thing where this does not fully cover the features of the View Layer mode of the material Attribute node. The node also does a layer -> scene -> world lookup fallback chain to support multiple levels of overrides implicitly. Obviously this all only makes sense for access to user-added custom properties, not builtin ones that always exist.

This could be another lookup mode, called `Active View Layer With Fallback' or something like that, which would try Layer -> Scene -> World -> the default value field in the variable.

Another thought: I wonder if these (especially in the view layer mode) should support a default value option, which would be used if the property path cannot be resolved instead of causing an error. This could be used to implement settings that are intended to be overridden only for some layers. Edit: I think the ability to detect if the property exists and provide defaults is one major thing where this does not fully cover the features of the View Layer mode of the material Attribute node. The node also does a layer -> scene -> world lookup fallback chain to support multiple levels of overrides implicitly. Obviously this all only makes sense for access to user-added custom properties, not builtin ones that always exist. This could be another lookup mode, called `Active View Layer With Fallback' or something like that, which would try Layer -> Scene -> World -> the default value field in the variable.

I'm a bit late here, but re camera specifically: can't it be overridden from the Video Sequencer?

I think there's two sources of cameras, then:

  • The scene
  • The VSE

If the VSE sets the scene camera, things should already work. If this should be implemented as special case, it could be done as a separate PR I think. It can then get an explicit test case to cover it, etc.

Another thought: I wonder if these (especially in the view layer mode) should support a default value option

This could be an interesting improvement, but IMO it's orthogonal to this patch.

> I'm a bit late here, but re camera specifically: can't it be overridden from the Video Sequencer? I think there's two sources of cameras, then: - The scene - The VSE If the VSE sets the scene camera, things should already work. If this should be implemented as special case, it could be done as a separate PR I think. It can then get an explicit test case to cover it, etc. > Another thought: I wonder if these (especially in the view layer mode) should support a default value option This could be an interesting improvement, but IMO it's orthogonal to this patch.

This could be an interesting improvement, but IMO it's orthogonal to this patch.

Sure, I'm just brainstorming suggestions here since this doesn't seem to be associated with a higher level design task.

Continuing, since fallbacks only make sense for custom properties, maybe this can even be enforced by having the mode be dedicated to them, e.g. adding a "Layer/Scene/World Custom Property" mode, which would have inputs for a custom property name (without the ["..."] quoting), channel index number (in case it's a vector), and default value field, and do the lookup in Layer -> Scene -> World.

> This could be an interesting improvement, but IMO it's orthogonal to this patch. Sure, I'm just brainstorming suggestions here since this doesn't seem to be associated with a higher level design task. Continuing, since fallbacks only make sense for custom properties, maybe this can even be enforced by having the mode be dedicated to them, e.g. adding a "Layer/Scene/World Custom Property" mode, which would have inputs for a custom property name (without the `["..."]` quoting), channel index number (in case it's a vector), and default value field, and do the lookup in Layer -> Scene -> World.
Author
Owner

Please leave brain-storming and discussion of things not directly related to the code and presentation of PR to either chat or a design task.

Doing those things in the PR does not help moving forward with an agreed-on incremental improvements.

Please leave brain-storming and discussion of things not directly related to the code and presentation of PR to either chat or a design task. Doing those things in the PR does not help moving forward with an agreed-on incremental improvements.
Sergey Sharybin merged commit c26566ad27 into main 2023-03-06 16:02:00 +01:00
Sergey Sharybin deleted branch driver_variable 2023-03-06 16:02:01 +01:00
Sybren A. Stüvel removed this from the Animation & Rigging project 2023-03-07 15:06:40 +01:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#105132
No description provided.