blender/blender

WIP: Python API: Add link/append pre/post handlers #122365

Draft

Philipp Oeser wants to merge 1 commits from lichtwerk/blender:122357 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Philipp Oeser commented

2024-05-28 12:49:48 +02:00

Member

These can be useful in many scenarios (e.g. handling local unique
identifiers).

This PR adds these handlers, whereas the PRE gives us the ID name that
is tried and the POST gives us the actual ID that was processed.

NOTE: !113658 is very similar (but tied to asset drag & drop), whereas
this PR is more general (these could probably live hand-in-hand / side-
by-side)

Resolves #122357

These can be useful in many scenarios (e.g. handling local unique identifiers). This PR adds these handlers, whereas the PRE gives us the ID name that is tried and the POST gives us the actual ID that was processed. NOTE: !113658 is very similar (but tied to asset drag & drop), whereas this PR is more general (these could probably live hand-in-hand / side- by-side) Resolves #122357

Philipp Oeser added 1 commit 2024-05-28 12:49:58 +02:00

Python API: Add link/append pre/post handlers

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

These can be useful in many scenarios (e.g. handling local unique
identifiers).

This PR adds these handlers, whereas the PRE gives us the ID name that
is tried and the POST gives us the actual ID that was processed.

NOTE: !113658 is very similar (but tied to asset drag & drop), whereas
this PR is more general (these could probably live hand-in-hand / side-
by-side)

Resolves #122357

Philipp Oeser added this to the Pipeline, Assets & IO project 2024-05-28 12:50:21 +02:00

Philipp Oeser added the

label 2024-05-28 12:50:28 +02:00

Philipp Oeser requested review from Bastien Montagne 2024-05-28 12:50:35 +02:00

Philipp Oeser requested review from Campbell Barton 2024-05-28 12:50:43 +02:00

Philipp Oeser requested review from Julian Eisel 2024-05-28 12:50:49 +02:00

Philipp Oeser commented

2024-05-28 12:51:36 +02:00

Author

Member

Code to test:

import bpy

@bpy.app.handlers.persistent
def on_append_pre(name):
    print("trying to append", name)

@bpy.app.handlers.persistent
def on_append_post(id_appended):
    print("appended", id_appended)

@bpy.app.handlers.persistent
def on_link_pre(name):
    print("trying to link", name)

@bpy.app.handlers.persistent
def on_link_post(id_linked):
    print("linked", id_linked)

bpy.app.handlers.append_pre.append(on_append_pre)
bpy.app.handlers.append_post.append(on_append_post)
bpy.app.handlers.link_pre.append(on_link_pre)
bpy.app.handlers.link_post.append(on_link_post)

Code to test: ```python import bpy @bpy.app.handlers.persistent def on_append_pre(name): print("trying to append", name) @bpy.app.handlers.persistent def on_append_post(id_appended): print("appended", id_appended) @bpy.app.handlers.persistent def on_link_pre(name): print("trying to link", name) @bpy.app.handlers.persistent def on_link_post(id_linked): print("linked", id_linked) bpy.app.handlers.append_pre.append(on_append_pre) bpy.app.handlers.append_post.append(on_append_post) bpy.app.handlers.link_pre.append(on_link_pre) bpy.app.handlers.link_post.append(on_link_post) ```

Philipp Oeser referenced this pull request

2024-05-28 12:53:16 +02:00

Asset Browser: Add pre+post handlers to asset drag & drop #113658

Nika Kutsniashvili commented

2024-05-28 12:54:32 +02:00

Contributor

Can you kick off package build please?

Can you kick off package build please?

Philipp Oeser commented

2024-05-28 12:55:04 +02:00

Author

Member

CC @Mets
CC @Laurens-Corijn
CC @GregZaal
CC @iluvblender

CC @Mets CC @Laurens-Corijn CC @GregZaal CC @iluvblender

Philipp Oeser commented

2024-05-28 12:55:41 +02:00

Author

Member

@blender-bot package

@blender-bot package

Blender Bot commented

2024-05-28 12:55:44 +02:00

Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122365) when ready.

Philipp Oeser commented

2024-05-28 13:08:27 +02:00

Author

Member

One thing to note is that when appending, the linking callback will also fire (at least in the current state of the PR) as internally linking and appending are very similar (appending is more or less just linking followed by making local...).

I guess it would be possible to change that (so the linking callback does not fire when appending, not totally sure if we want this though).

One thing to note is that when appending, the linking callback will also fire (at least in the current state of the PR) as internally linking and appending are very similar (appending is more or less just linking followed by making local...). I guess it would be possible to change that (so the linking callback does **not** fire when appending, not totally sure if we want this though).

Julian Eisel reviewed 2024-05-28 13:14:43 +02:00

Julian Eisel left a comment

Member

Quite sure this is useful, and long requested.

The main question for me is when exactly the callbacks should fire. I guess the post callbacks should fire only after data is instantiated, and after versioning. So really at the end of BKE_blendfile_append()/BKE_blendfile_link(). Technically operators may still perform further work as part of the linking/appending, but practically I think this would be fine.

Might also be worth noting in the API documentation that users can link/append multiple IDs at once, and that this will fire the callbacks for each.

Quite sure this is useful, and long requested. The main question for me is when exactly the callbacks should fire. I guess the _post_ callbacks should fire only after data is instantiated, and after versioning. So really at the end of `BKE_blendfile_append()`/`BKE_blendfile_link()`. Technically operators may still perform further work as part of the linking/appending, but practically I think this would be fine. Might also be worth noting in the API documentation that users can link/append multiple IDs at once, and that this will fire the callbacks for each.

Laurens-Corijn commented

2024-05-28 14:00:49 +02:00

First-time contributor

Awesome, will use this for sure.

Now .. for handlers on_delete_pre and on_delete_post ;)

Awesome, will use this for sure. Now .. for handlers on_delete_pre and on_delete_post ;)

Demeter Dzadik commented

2024-05-29 13:14:22 +02:00

Member

It's great to see somebody working on this, I think this could already result in a lot of nice QoL stuff for productions. But I did run into some issues pretty quickly! I hope they won't be too much of a pain to iron out.

Run per operation vs per ID

When linking, the handlers only run once, passing the root ID that we linked.
When appending however, the handler runs for each ID that gets dragged along with the root through dependencies.

I think the former is better. If user wants to loop through all the dependent IDs, they can use some extra code:

import bpy
from bpy_extras.id_map_utils import get_id_reference_map, get_all_referenced_ids

@bpy.app.handlers.persistent
def on_link_post(id_linked):
    referenced_ids = get_all_referenced_ids(id_linked, get_id_reference_map())

Pass context?

Can and should bpy.context be passed to the handler? I don't really know when it is and when it isn't safe to reach out for the global variable. I just know that global variables make me uneasy.

For example, to make an add-on that saves you the hassle of overriding linked assets, one needs to pass a desired scene and view layer to the override_hierarchy_create function, which are pretty much only possible to grab from the context, or by looping and searching all scenes and viewlayers for our id_linked.

Crash on override_hierarchy_create

One obvious use case would be to immediately create overrides after linking an asset (a collection), which would be neat since each team could create overrides the way they want. But for me, this is crashing, even with a simple asset and without referencing bpy.context:

import bpy

@bpy.app.handlers.persistent
def on_link_post(id_linked):
    if type(id_linked) == bpy.types.Collection:
        scene = bpy.data.scenes[0]
        override_coll = id_linked.override_hierarchy_create(scene, scene.view_layers[0])

bpy.app.handlers.link_post.append(on_link_post)

Asset (Cube with 1 bone in a collection): cube_asset.blend

Reference to the instancer empty?

When linking in a collection, Blender automatically creates an Empty for us in the active scene and view layer, and sets its instance_collection to the linked collection. Right now, this seems to happen after link_post, which makes it impossible to operate on this instancer, eg. to delete it. I think that's bad.

It's great to see somebody working on this, I think this could already result in a lot of nice QoL stuff for productions. But I did run into some issues pretty quickly! I hope they won't be too much of a pain to iron out. --------------------------- ### Run per operation vs per ID - When linking, the handlers only run once, passing the root ID that we linked. - When appending however, the handler runs for each ID that gets dragged along with the root through dependencies. I think the **former is better**. If user wants to loop through all the dependent IDs, they can use some extra code: ```python import bpy from bpy_extras.id_map_utils import get_id_reference_map, get_all_referenced_ids @bpy.app.handlers.persistent def on_link_post(id_linked): referenced_ids = get_all_referenced_ids(id_linked, get_id_reference_map()) ``` --------------------------- ### Pass context? Can and should bpy.context be passed to the handler? I don't really know when it is and when it isn't safe to reach out for the global variable. I just know that global variables make me uneasy. For example, to make an add-on that saves you the hassle of overriding linked assets, one needs to pass a desired scene and view layer to the `override_hierarchy_create` function, which are pretty much only possible to grab from the context, or by looping and searching all scenes and viewlayers for our `id_linked`. ---------------------- ### Crash on override_hierarchy_create One obvious use case would be to immediately create overrides after linking an asset (a collection), which would be neat since each team could create overrides the way they want. But for me, this is crashing, even with a simple asset and without referencing bpy.context: ```python import bpy @bpy.app.handlers.persistent def on_link_post(id_linked): if type(id_linked) == bpy.types.Collection: scene = bpy.data.scenes[0] override_coll = id_linked.override_hierarchy_create(scene, scene.view_layers[0]) bpy.app.handlers.link_post.append(on_link_post) ``` Asset (Cube with 1 bone in a collection): [cube_asset.blend](/attachments/45a0416e-4f15-4577-8fc0-051a18d1f16b) -------------------- ### Reference to the instancer empty? When linking in a collection, Blender automatically creates an Empty for us in the active scene and view layer, and sets its `instance_collection` to the linked collection. Right now, this seems to happen after `link_post`, which makes it impossible to operate on this instancer, eg. to delete it. I think that's bad.

cube_asset.blend

691 KiB

Campbell Barton commented

2024-05-30 06:19:23 +02:00

Owner

Reviewing this implementation however I think a different approach is worth investigating (I'll propose this separately).

Generally useful functionality although I think it would be good to have concrete examples of what this would be used for, otherwise it's difficult to evaluate different solutions.

Checking on this PR.

The 2 letter ID codes are part of the ID name (used in the "pre" callback). As far as I'm aware this is currently an internal detail that is never exposed to the Python API and intentionally so. Including these in names means we'll have to document them as part of the Python API. Avoiding this likely involves extending callbacks.
A possible "simple" solution could be to construct a string that replaced the ID-code with the plural of the ID type (see BKE_idtype_idcode_to_name_plural). e.g. "collections:Collection Name" instead of "GRCollecton Name", then the first part of the name can be used to do look-ups on bpy.data for example.
The post callbacks look to run too early (before versioning for example). As far as I can see these should run last to avoid handlers interfering with link/append logic as well as potentially not having access to the "final" state which scripts may require - depending on the use-case.
There should be a {append/link}_post_fail handler, meaning a pre callback may run without any post callback which can make the state difficult to manage.
Having pre/post callbacks run on an each ID means script authors can't deal with with the "result" of appending ID's (at least it's quite difficult).

This could be resolved with handlers for the append/link action:
```
append_pre(blend_file_name)
    append_id_pre(id_name)
    ... the ID is linked ...
    append_id_{post/post_fail}(id_name)
append_post(blend_file_name)
```
Although it still seems clunky, a workaround for only supporting primitives arguments to handlers.
At least it allows script authors to collect data and operate on it.

Reviewing this implementation however I think a different approach is worth investigating _(I'll propose this separately)._ Generally useful functionality although I think it would be good to have concrete examples of what this would be used for, otherwise it's difficult to evaluate different solutions. Checking on this PR. - The 2 letter ID codes are part of the ID name (used in the "pre" callback). As far as I'm aware this is currently an internal detail that is never exposed to the Python API and intentionally so. Including these in names means we'll have to document them as part of the Python API. Avoiding this likely involves extending callbacks. A possible "simple" solution could be to construct a string that replaced the ID-code with the plural of the ID type (see `BKE_idtype_idcode_to_name_plural`). e.g. `"collections:Collection Name"` instead of `"GRCollecton Name"`, then the first part of the name can be used to do look-ups on `bpy.data` for example. - The post callbacks look to run too early (before versioning for example). As far as I can see these should run last to avoid handlers interfering with link/append logic as well as potentially not having access to the "final" state which scripts may require - depending on the use-case. - There should be a `{append/link}_post_fail` handler, meaning a `pre` callback may run without any post callback which can make the state difficult to manage. - Having `pre/post` callbacks run on an each ID means script authors can't deal with with the "result" of appending ID's (at least it's quite difficult). This could be resolved with handlers for the append/link action: ``` append_pre(blend_file_name) append_id_pre(id_name) ... the ID is linked ... append_id_{post/post_fail}(id_name) append_post(blend_file_name) ``` Although it still seems clunky, a workaround for only supporting primitives arguments to handlers. At least it allows script authors to collect data and operate on it.

Campbell Barton commented

2024-05-30 06:26:19 +02:00

Owner

Suggestion for an alternative approach

Pass an RNA wrapped BlendfileLinkAppendContext to {append/link}_{pre/post} which are only called once (not for each ID).
Expose the following members:
- A collection of BlendfileLinkAppendContextItem.
- Linking params: scene, view_layer, 3D viewport... (see LibraryLink_Params).

Pros:

Ability to access all ID's at once.
Avoids ugly string ID code issue (the RNA can expose this as: id_type & name).
Reduced overhead when importing many items.
Access to more information (the scene, view_layer being appended into).
Ability to extend the RNA to expose more details if necessary.

Cons:

Exposes Link/Append internals to RNA, although we could limit what's exposed to avoid internals leaking into the API too much.
The API is more involved.

This addresses some of the concerns by @Mets, I'd be interested in what @mont29 thinks of this.

**Suggestion for an alternative approach** - Pass an RNA wrapped `BlendfileLinkAppendContext` to `{append/link}_{pre/post}` which are only called once (not for each ID). - Expose the following members: - A collection of `BlendfileLinkAppendContextItem`. - Linking params: `scene`, `view_layer`, `3D viewport`... (see `LibraryLink_Params`). Pros: - Ability to access all ID's at once. - Avoids ugly string ID code issue (the RNA can expose this as: `id_type` & `name`). - Reduced overhead when importing many items. - Access to more information (the scene, view_layer being appended into). - Ability to extend the RNA to expose more details if necessary. Cons: - Exposes Link/Append internals to RNA, although we could limit what's exposed to avoid internals leaking into the API too much. - The API is more involved. --- This addresses some of the concerns by @Mets, I'd be interested in what @mont29 thinks of this.

Campbell Barton referenced this pull request

2024-05-30 07:05:29 +02:00

Asset Browser: Add pre+post handlers to asset drag & drop #113658

Philipp Oeser commented

2024-05-30 10:24:39 +02:00

Author

Member

Thank you for checking on this and for the very helpful feedback!

On first sight, the suggested alternative approach looks very attractive, will investigate this first (since it would solve most of the other issues mentioned).

Thank you for checking on this and for the very helpful feedback! On first sight, the suggested alternative approach looks very attractive, will investigate this first (since it would solve most of the other issues mentioned).

Bastien Montagne commented

2024-05-30 12:38:05 +02:00

Owner

I also like @ideasman42 suggestion a lot. I don't think exposing the BlendfileLinkAppendContext system is a bad thing, this has been very stable and reliable since its introduction many years ago, so I would not expect any drastic changes in it any time soon.

Bonus point, the BlendfileLinkAppendContextItem items know whether they were explicitly linked/appended, or as a dependency (LINK_APPEND_TAG_INDIRECT tag).

I also like @ideasman42 suggestion a lot. I don't think exposing the `BlendfileLinkAppendContext` system is a bad thing, this has been very stable and reliable since its introduction many years ago, so I would not expect any drastic changes in it any time soon. Bonus point, the `BlendfileLinkAppendContextItem` items know whether they were explicitly linked/appended, or as a dependency (`LINK_APPEND_TAG_INDIRECT` tag).

Demeter Dzadik commented

2024-05-30 14:37:22 +02:00

Member

it would be good to have concrete examples of what this would be used for

My list of use cases that I'd like to implement one day:

Quicker asset (collection) linking for animation: Asset can be automatically library overridden, assigned to the correct collections for the production, rigs can be made editable, and rig scripts can be executed. It saves a lot of clicking.
Quick asset joining for animation: Same as above, but an asset can also store information about how it wants to hook up to another asset, and then a post-link handler can do that automatically. For example, constraining a sword to a character's hand.
Drag & Drop bone widgets for rigging: Select some bones, link/append/drag&drop a widget to apply it to the selected bones and assign it to an appropriate collection, even if that collection wasn't the selected one. Smoother workflow, less clicking.
Drag&Drop metarig elements for systems like Rigify/CloudRig/etc: Similar as above, we could load Armature objects and merge them into the one that we're working on, and parent it to the active bone. Drag&Drop limbs type of thing.

> it would be good to have concrete examples of what this would be used for My list of use cases that I'd like to implement one day: - Quicker asset (collection) linking for animation: Asset can be automatically library overridden, assigned to the correct collections for the production, rigs can be made editable, and rig scripts can be executed. It saves a lot of clicking. - Quick asset joining for animation: Same as above, but an asset can also store information about how it wants to hook up to another asset, and then a post-link handler can do that automatically. For example, constraining a sword to a character's hand. - Drag & Drop bone widgets for rigging: Select some bones, link/append/drag&drop a widget to apply it to the selected bones and assign it to an appropriate collection, even if that collection wasn't the selected one. Smoother workflow, less clicking. - Drag&Drop metarig elements for systems like Rigify/CloudRig/etc: Similar as above, we could load Armature objects and merge them into the one that we're working on, and parent it to the active bone. Drag&Drop limbs type of thing.

Philipp Oeser commented

2024-07-18 09:55:39 +02:00

Author

Member

Sorry this has been on hold (it might be on hold a bit longer even, but still targeting 4.3..., will mark as WIP for now)

(just a note to myself: since 83373002d2 and a473107489, #124794 might be relevant?)

Sorry this has been on hold (it might be on hold a bit longer even, but still targeting 4.3..., will mark as WIP for now) (just a note to myself: since 83373002d210c0d315666104f495234cb09da4e8 and a47310748953623702f71aab7dea0db43400b499, #124794 might be relevant?)

Philipp Oeser changed title from ~~Python API: Add link/append pre/post handlers~~ to WIP: Python API: Add link/append pre/post handlers

2024-07-18 09:55:53 +02:00

Bastien Montagne commented

2024-07-18 10:22:28 +02:00

Owner

(just a note to myself: since 83373002d2 and a473107489, #124794 might be relevant?)

If you go through RNA system, #124794 should not be relevant here. We already have quite a lot of non-POD C++ data exposed through RNA.

> (just a note to myself: since 83373002d210c0d315666104f495234cb09da4e8 and a47310748953623702f71aab7dea0db43400b499, #124794 might be relevant?) If you go through RNA system, #124794 should not be relevant here. We already have quite a lot of non-POD C++ data exposed through RNA.

Patrick W. Crawford commented

2024-08-14 22:04:50 +02:00

First-time contributor

Just wanted to chime in that with this being treated as a prereq for the asset browser pre+post drop hooks, having both of these features would unlock asset browser integration for a couple different addon projects I'm involved in, which so far have been stuck with custom rolled asset browsing experiences which predate the native Blender asset browser.

Most of it is to do with addon-managed objects/materials loaded from disk, and needing the addon to modify properties or configurations post import based on current scene settings, or to enable special redo last settings specific to the addon/asset (which could be triggered an operator call directly from the post import hook, even though that's a little hacky).

Just wanted to chime in that with this being treated as a prereq for the asset browser [pre+post drop hooks](https://projects.blender.org/blender/blender/pulls/113658), having both of these features would unlock asset browser integration for a couple different addon projects I'm involved in, which so far have been stuck with custom rolled asset browsing experiences which predate the native Blender asset browser. Most of it is to do with addon-managed objects/materials loaded from disk, and needing the addon to modify properties or configurations post import based on current scene settings, or to enable special redo last settings specific to the addon/asset (which could be triggered an operator call directly from the post import hook, even though that's a little hacky).

Bastien Montagne referenced this pull request

2024-08-22 14:38:15 +02:00

Regression: Append (reuse data) for objects with a Geometry Nodes modifier does not work as expected #125647

Bastien Montagne added this to the 4.3 milestone 2024-08-22 15:06:35 +02:00

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

This pull request is marked as a work in progress.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 122357:lichtwerk-122357

git checkout lichtwerk-122357

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

9 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#122365

No description provided.