Drivers: Introduce the Context Properties #105132
No reviewers
Labels
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
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#105132
Loading…
Reference in New Issue
No description provided.
Delete Branch "Sergey/blender:driver_variable"
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?
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, butcamera.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:
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 whythis 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.
0eacd09101
toe30fad939c
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"
@ -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.
Typo & extra "from". Probably clearer to write something like "which is used to resolve the target rna_path".
@ -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.@ -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 regularbContext *
.@ -76,0 +90,4 @@
return target_context;
}
/* Specialized implementation of driver_get_target_property() sued for the
sued -> used
@ -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?
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.
@ -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 thecreate
in the function name.Ok, will modify the TODO to replace the code with a function call once that function is actually implemented :)
@ -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:
To me the
Is more readable. And is how it is structured in the relations builder.
@ -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 againstid
. Nowtarget_prop
seems to already point to a property, given its name, and not to anID
. But what does therna_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 localid_ptr
variable totarget_prop
, so there must be some other change as well that I don't quite understand yet.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.
On the topic of the
target_prop
andrna_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 namedvar
). It has a single target (DriverTarget
in the C types, typical variable namedtar
). The target has aProp
(as it is called in the interface, internally it is a combination ofID *id
andint idtype;
), and a RNA Path.In the example on the image the
Prop
is camera,Path
is thematrix_world[3][0]
.This directly translates to the arguments of the
build_driver_id_property
:target_prop
isProp
, and in the example it iscamera
rna_path
isPath
, and in the example it ismatrix_world[3][0]
I guess what we can do is the following:
target_prop
corresponds to theProp
in the interfacerna_path
to something likerna_path_from_target_prop
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 :)
Thanks for the explanation. There are a lot of indirections and properties of variables of targets with pointers to ..... indeed.
👍 LGTM.
Hah yeah I can help with that.
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! Much clearer now :)
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.
That is an interesting point.
My thoughts on it:
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.
I think there's two sources of cameras, then:
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.
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.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.