PyAPI: extend save/load handlers, optionally take a filepath argument #104769

Campbell Barton · 2023-02-15T07:13:15+01:00

Campbell Barton commented

2023-02-15 07:13:15 +01:00

Add a filepath argument to load/save pre/post.
Also add save_post_failed and load_post_failed handlers so it's always
possible to for the pre handlers to run a matching post action.

This makes it possible to know the filepath of the blend file mean
loaded/saved as well as supporting running an action when load/save
operations fail.

When loading and saving the startup-file, the path argument is set to
an empty string.

Details:

New RNA types were added to support storing primitive values in
PointerRNA. Primitive{String/Int/Float/Boolean}RNA. These will likely
only be used in some limited cases, in the case of BKE_callback_exec it
allows strings to be included as part of the PointerRNA **pointers
argument.

Original Submission for Context (the scope of this patch has been reduced)

Add the following handlers.

save_init, load_init which run before file-path checks have run.
save_fail, load_fail which run if save or load fail.

All of the save/load callbacks now take a filepath argument,
without this there was no way of knowing the file path the user intended
to load - this was especially a problem for the save_pre/load_pre handlers
as it wasn't possible to know the name of the file being loaded or saved.

When loading and saving the startup-file, the path argument is set to
an empty string.

Note that existing handlers that don't take any arguments will continue
to work as before.

Resolves #88493, includes contributions from D11422 by @filedescriptor.

Details:

New RNA types were added to support storing primitive values in
PointerRNA. Primitive{String/Int/Float/Boolean}RNA. These will likely
only be used in some limited cases, in the case of BKE_callback_exec it
allows strings to be included as part of the PointerRNA **pointers
argument.

A while ago Falk submitted D11422 to address #88493, while in principle the handlers made sense, they didn't resolve the issue reported by the user as there was no way to know the path of the file being loaded.

This patch includes the handlers from D11422 and adds support for passing string/int/float/boolean values via BKE_callback_exec functions.
I'm not totally happy with having to add new RNA structs for this however the code is simple and the alternative options aren't great either - they might involve wrapping the PointerRNA **pointers argument in a container that can also reference non-RNA types or having a different calling convention that supports both PointerRNA and primitive types, however mixing them together is likely to be awkward.

Further work:

Out of scope for this patch but worth looking into:

We could pass arguments for other handlers (the name of the undo action to undo_{pre/post} for example), it seems reasonable and was requested here although in that particular case a more spesific solution might be better.
If handlers could support different kinds of arguments - it's probably good to discourage calling BKE_CB_EVT_* directly and have functions which take the appropriate arguments.

Add a filepath argument to load/save pre/post. Also add save_post_failed and load_post_failed handlers so it's always possible to for the pre handlers to run a matching post action. This makes it possible to know the filepath of the blend file mean loaded/saved as well as supporting running an action when load/save operations fail. When loading and saving the startup-file, the path argument is set to an empty string. Details: New RNA types were added to support storing primitive values in PointerRNA. Primitive{String/Int/Float/Boolean}RNA. These will likely only be used in some limited cases, in the case of BKE_callback_exec it allows strings to be included as part of the PointerRNA **pointers argument. ---- #### Original Submission for Context (the scope of this patch has been reduced) Add the following handlers. - save_init, load_init which run before file-path checks have run. - save_fail, load_fail which run if save or load fail. All of the save/load callbacks now take a filepath argument, without this there was no way of knowing the file path the user intended to load - this was especially a problem for the save_pre/load_pre handlers as it wasn't possible to know the name of the file being loaded or saved. When loading and saving the startup-file, the path argument is set to an empty string. Note that existing handlers that don't take any arguments will continue to work as before. Resolves #88493, includes contributions from D11422 by @filedescriptor. Details: New RNA types were added to support storing primitive values in `PointerRNA`. Primitive{String/Int/Float/Boolean}RNA. These will likely only be used in some limited cases, in the case of BKE_callback_exec it allows strings to be included as part of the `PointerRNA **pointers` argument. --- A while ago Falk submitted D11422 to address #88493, while in principle the handlers made sense, they didn't resolve the issue reported by the user as there was no way to know the path of the file being loaded. This patch includes the handlers from D11422 and adds support for passing string/int/float/boolean values via `BKE_callback_exec` functions. I'm not totally happy with having to add new RNA structs for this however the code is simple and the alternative options aren't great either - they might involve wrapping the `PointerRNA **pointers` argument in a container that can also reference non-RNA types or having a different calling convention that supports both `PointerRNA` and primitive types, however mixing them together is likely to be awkward. Further work: Out of scope for this patch but worth looking into: - We could pass arguments for other handlers (the name of the undo action to undo_{pre/post} for example), it seems reasonable and was requested [here](https://devtalk.blender.org/t/access-to-the-history-stack/27384/14) although in that particular case a more spesific solution might be better. - If handlers could support different kinds of arguments - it's probably good to discourage calling `BKE_CB_EVT_*` directly and have functions which take the appropriate arguments.

Campbell Barton requested review from Bastien Montagne 2023-02-15 07:14:54 +01:00

Campbell Barton requested review from Falk David 2023-02-15 07:14:55 +01:00

Campbell Barton requested review from Brecht Van Lommel 2023-02-15 07:18:39 +01:00

Brecht Van Lommel commented

2023-02-15 07:48:35 +01:00

As you noted in #88493, this will only work for .blend files and Blender can save and load many file types.

So is this really intended to solve the version control add-on use case, and if it's an unreliable solution then who do we expect to use this?

I'd like to hear more motivation from @filedescriptor regarding why he implemented D11422, if this was working for an actual add-on or just done with the idea that it could be useful without using it in production.

For reference, USD has the concept of resolvers for this type of purpose though implementing that would be a major undertaking.
https://graphics.pixar.com/usd/release/wp_ar2.html

As you noted in #88493, this will only work for .blend files and Blender can save and load many file types. So is this really intended to solve the version control add-on use case, and if it's an unreliable solution then who do we expect to use this? I'd like to hear more motivation from @filedescriptor regarding why he implemented [D11422](https://archive.blender.org/developer/D11422), if this was working for an actual add-on or just done with the idea that it could be useful without using it in production. For reference, USD has the concept of resolvers for this type of purpose though implementing that would be a major undertaking. https://graphics.pixar.com/usd/release/wp_ar2.html

Campbell Barton force-pushed pr-save-load-handlers-with-filepath from 0fee8a0338 to 31f719e38f

2023-02-15 07:58:56 +01:00

Compare

Brecht Van Lommel added this to the Python API project 2023-02-15 09:51:37 +01:00

Falk David commented

2023-02-15 09:57:20 +01:00

@brecht afaik, this was never used. So I don't have a concrete example to show. Nevertheless, this still seems like a useful thing to have.

Brecht Van Lommel commented

2023-02-15 10:05:06 +01:00

I'd rather not have a halfway solution, if we already know we'll have to deprecate these handlers when we add a complete solution. We can't just add additional file types to these ones without breaking compatibility, and the naming would also be confusing then.

Kathryn Long commented

2023-02-16 23:16:32 +01:00

First-time contributor

Started trying to implement simple Perforce integration for our studio for Blender and ran into the same problem as #88493. This PR would give me everything I need to do the integration to the level we need.

In particular, I don't care about other referenced files in Blender being saved or loaded. I DO care about .blend files, and want to do basic Perforce integration for these.

I would rather see the functionality in this PR sooner rather than waiting a long time for a more comprehensive solution for all referenced files. Yes that would be nice long term, but as it stands right now, the existing handlers don't support referenced files, but the existing handlers are also useless for many use cases I can think of without the changes in this PR. It also seems unlikely you would have to deprecate the handlers of this PR for a solution that handles referenced files, unless it also requires you to deprecate your existing save/load handlers.

Anyway, please get the functionality in this PR in as soon as possible.

Started trying to implement simple Perforce integration for our studio for Blender and ran into the same problem as #88493. This PR would give me everything I need to do the integration to the level we need. In particular, I don't care about other referenced files in Blender being saved or loaded. I DO care about .blend files, and want to do basic Perforce integration for these. I would rather see the functionality in this PR sooner rather than waiting a long time for a more comprehensive solution for all referenced files. Yes that would be nice long term, but as it stands right now, the **existing** handlers don't support referenced files, but the existing handlers are also useless for many use cases I can think of without the changes in this PR. It also seems unlikely you would have to deprecate the handlers of this PR for a solution that handles referenced files, unless it also requires you to deprecate your **existing** save/load handlers. Anyway, please get the functionality in this PR in as soon as possible.

Campbell Barton commented

2023-02-17 01:34:41 +01:00

To give some context, this was a "fun Wednesday project", it's something that has bothered me for a while that bpy.app.handers can't accept string/int/float/boolean arguments, in other systems I've used callbacks receive relevant arguments so this seemed like an arbitrary limitation in Blender's Python API.

On the other hand, as Brecht points out, as a full solution to #88493 that works across different file-types isn't resolved by this PR (if save writes modified images or exports files for e.g.).

Coming up with a solution that works in more situations pushes us to consider the big-picture and means we're less likely to end up with something that doesn't solve the end goal.

@KathrynLong, re:

In particular, I don't care about other referenced files in Blender being saved or loaded. I DO care about .blend files, and want to do basic Perforce integration for these.

While it's useful to know you're not personally needing support for other file types, it's still worth considering if this should be supported more generally - for other users & use-cases. Although I think the topic of supporting the perforce workflow would be better handled separately.

Suggestion for reducing the scope of this PR:

Postpone adding handlers until a concrete use-case with examples is available and we have time to spend ensuring the changes to handlers work as intended.
Add a filepath argument to existing load/save handlers. Taking the filepath of the content being loaded/saved seems like an obvious/uncontroversial improvement which we would already support if passing string arguments had been possible when these handlers were added.

This is a low-friction change that allows for a solution to #88493 to be evaluated separately.

To give some context, this was a _"fun Wednesday project"_, it's something that has bothered me for a while that `bpy.app.handers` can't accept string/int/float/boolean arguments, in other systems I've used callbacks receive relevant arguments so this seemed like an arbitrary limitation in Blender's Python API. On the other hand, as Brecht points out, as a full solution to #88493 that works across different file-types isn't resolved by this PR (if save writes modified images or exports files for e.g.). Coming up with a solution that works in more situations pushes us to consider the big-picture and means we're less likely to end up with something that doesn't solve the end goal. ----- @KathrynLong, re: > In particular, I don't care about other referenced files in Blender being saved or loaded. I DO care about .blend files, and want to do basic Perforce integration for these. While it's useful to know you're not personally needing support for other file types, it's still worth considering if this should be supported more generally - for other users & use-cases. Although I think the topic of supporting the perforce workflow would be better handled separately. ---- **Suggestion for reducing the scope of this PR:** - Postpone adding handlers until a concrete use-case with examples is available and we have time to spend ensuring the changes to handlers work as intended. - Add a `filepath` argument to existing load/save handlers. Taking the `filepath` of the content being loaded/saved seems like an obvious/uncontroversial improvement which we would already support if passing string arguments had been possible when these handlers were added. This is a low-friction change that allows for a solution to #88493 to be evaluated separately.

Brecht Van Lommel commented

2023-02-17 15:16:30 +01:00

I'm ok with having these handlers if there is a practical use case for them. I can see how it avoids manual lock/unlock for common cases when using Perforce, even if it's not all cases.

We could still consider making this more future proof having file handlers like this instead of tying it to the existing save handlers.

file_read_pre(filepath)
file_read_post(filepath, failed)

file_write_pre(filepath)
file_write_post(filepath, failed)

Which could be extended for other file types or other cases where .blend files are saved (e.g. libraries unpack or draft assets) in the future. Though it's not entirely obvious to me if these two handlers would provide sufficient information for Perforce and similar version control, or if it matters to know that the main .blend file is being saved.

I'm ok with having these handlers if there is a practical use case for them. I can see how it avoids manual lock/unlock for common cases when using Perforce, even if it's not all cases. We could still consider making this more future proof having file handlers like this instead of tying it to the existing save handlers. ``` file_read_pre(filepath) file_read_post(filepath, failed) file_write_pre(filepath) file_write_post(filepath, failed) ``` Which could be extended for other file types or other cases where .blend files are saved (e.g. libraries unpack or draft assets) in the future. Though it's not entirely obvious to me if these two handlers would provide sufficient information for Perforce and similar version control, or if it matters to know that the main .blend file is being saved.

Kathryn Long commented

2023-02-17 23:35:14 +01:00

First-time contributor

@ideasman42 re:
While it's useful to know you're not personally needing support for other file types, it's still worth considering if this should be supported more generally - for other users & use-cases. Although I think the topic of supporting the perforce workflow would be better handled separately.

My point wasn't to oppose forward-compatible changes, such as @brecht proposed file_read/file_write handlers, my point was that there is a use-case for which handlers are sufficient (and similar handler-like callback features in other DCC tools such as Maya, Substance, etc. are what I've long used in my career for perforce integration). A future improved workflow specifically for supporting VCS like perforce shouldn't stop a handler-based solution here, which wouldn't need to be deprecated for a VCS-specific solution.

I admit to being slightly frustrated finding out that Blender has handler load/save callbacks but without the necessary functionality (filepath and a pre-save that still gets called for readonly files, not having file close callback, etc.) that nearly every other scriptable DCC tool has, and that affected my post.

@brecht re:
Though it's not entirely obvious to me if these two handlers would provide sufficient information for Perforce and similar version control, or if it matters to know that the main .blend file is being saved.

From my career experience, this is mostly sufficient, and the path of the saved blend file isn't necessary for the VCS side of things, although it may be useful for other uses.

A handler for when a file is "closed" would also be useful (for example, one Perforce workflow is to checkout a file when you open the file, and undo the checkout if the file is closed without being saved). For blend files this can just be worked around using save handlers, but if you want a more general future solution for referenced data files, something like file_unreferenced_pre/post handler might make sense than a file_close_pre/post.

The only other functionality I would have is if the file_read_pre/post handler would be called when reading other referenced library blend files other than the one currently opened for edit. If so, I would add an argument to the handler that flags if the file is being opened for editing (and therefore possible future saving). Afaik Blender only allows editing data in current blend file and not referenced libraries?

> @ideasman42 re: > While it's useful to know you're not personally needing support for other file types, it's still worth considering if this should be supported more generally - for other users & use-cases. Although I think the topic of supporting the perforce workflow would be better handled separately. My point wasn't to oppose forward-compatible changes, such as @brecht proposed `file_read`/`file_write` handlers, my point was that there *is* a use-case for which handlers are sufficient (and similar handler-like callback features in other DCC tools such as Maya, Substance, etc. are what I've long used in my career for perforce integration). A future improved workflow specifically for supporting VCS like perforce shouldn't stop a handler-based solution here, which wouldn't need to be deprecated for a VCS-specific solution. I admit to being slightly frustrated finding out that Blender has handler load/save callbacks but without the necessary functionality (filepath and a pre-save that still gets called for readonly files, not having file close callback, etc.) that nearly every other scriptable DCC tool has, and that affected my post. > @brecht re: > Though it's not entirely obvious to me if these two handlers would provide sufficient information for Perforce and similar version control, or if it matters to know that the main .blend file is being saved. From my career experience, this is mostly sufficient, and the path of the saved blend file isn't necessary for the VCS side of things, although it may be useful for other uses. A handler for when a file is "closed" would also be useful (for example, one Perforce workflow is to checkout a file when you open the file, and undo the checkout if the file is closed without being saved). For blend files this can just be worked around using save handlers, but if you want a more general future solution for referenced data files, something like `file_unreferenced_pre/post` handler might make sense than a `file_close_pre/post`. The only other functionality I would have is if the `file_read_pre/post` handler would be called when reading other referenced library blend files other than the one currently opened for edit. If so, I would add an argument to the handler that flags if the file is being opened for editing (and therefore possible future saving). Afaik Blender only allows editing data in current blend file and not referenced libraries?

Brecht Van Lommel requested changes 2023-02-23 10:57:03 +01:00

Brecht Van Lommel left a comment

A handler for when a file is "closed" would also be useful (for example, one Perforce workflow is to checkout a file when you open the file, and undo the checkout if the file is closed without being saved). For blend files this can just be worked around using save handlers, but if you want a more general future solution for referenced data files, something like file_unreferenced_pre/post handler might make sense than a file_close_pre/post.

The only other functionality I would have is if the file_read_pre/post handler would be called when reading other referenced library blend files other than the one currently opened for edit. If so, I would add an argument to the handler that flags if the file is being opened for editing (and therefore possible future saving). Afaik Blender only allows editing data in current blend file and not referenced libraries?

Maybe these handlers would then cover everything? written would be called when Blender wrote to a file but may still be using it. post would be called Blender stops using the file altogether.

file_reference_pre(filepath, for_write)
file_reference_written(filepath)
file_reference_post(filepath)

A tricky thing is that we don't really know in general if a user is going to use a file for writing in advance. A user might open a model with a bunch of image textures, we don't know if they're going to be painted on in advance. It's the same for .blend files really, someone might only open a .blend file for viewing.

This could be done just-in-time when entering paint mode or something, but it gets complicated.

The other thing is, if the file is already locked by another user, is there anything Blender should do about that? I guess it's reasonable to leave this to the add-on, to display a warning or something like that.

Suggestion for reducing the scope of this PR:

Postpone adding handlers until a concrete use-case with examples is available and we have time to spend ensuring the changes to handlers work as intended.

To me it's clear what the use cases are: version control lock/unlock and automatic staging, as well as sparse checkouts of production files. Where it gets less clear is what the exact handlers should to solve those cases. So indeed best part as done as a separate PR.

Add a filepath argument to existing load/save handlers. Taking the filepath of the content being loaded/saved seems like an obvious/uncontroversial improvement which we would already support if passing string arguments had been possible when these handlers were added.

Doing that part first seems fine in any case.

> A handler for when a file is "closed" would also be useful (for example, one Perforce workflow is to checkout a file when you open the file, and undo the checkout if the file is closed without being saved). For blend files this can just be worked around using save handlers, but if you want a more general future solution for referenced data files, something like `file_unreferenced_pre/post` handler might make sense than a `file_close_pre/post`. > > The only other functionality I would have is if the `file_read_pre/post` handler would be called when reading other referenced library blend files other than the one currently opened for edit. If so, I would add an argument to the handler that flags if the file is being opened for editing (and therefore possible future saving). Afaik Blender only allows editing data in current blend file and not referenced libraries? Maybe these handlers would then cover everything? `written` would be called when Blender wrote to a file but may still be using it. `post` would be called Blender stops using the file altogether. ``` file_reference_pre(filepath, for_write) file_reference_written(filepath) file_reference_post(filepath) ``` A tricky thing is that we don't really know in general if a user is going to use a file for writing in advance. A user might open a model with a bunch of image textures, we don't know if they're going to be painted on in advance. It's the same for .blend files really, someone might only open a .blend file for viewing. This could be done just-in-time when entering paint mode or something, but it gets complicated. The other thing is, if the file is already locked by another user, is there anything Blender should do about that? I guess it's reasonable to leave this to the add-on, to display a warning or something like that. > **Suggestion for reducing the scope of this PR:** > > - Postpone adding handlers until a concrete use-case with examples is available and we have time to spend ensuring the changes to handlers work as intended. To me it's clear what the use cases are: version control lock/unlock and automatic staging, as well as sparse checkouts of production files. Where it gets less clear is what the exact handlers should to solve those cases. So indeed best part as done as a separate PR. > - Add a `filepath` argument to existing load/save handlers. Taking the `filepath` of the content being loaded/saved seems like an obvious/uncontroversial improvement which we would already support if passing string arguments had been possible when these handlers were added. Doing that part first seems fine in any case.

Campbell Barton force-pushed pr-save-load-handlers-with-filepath from 31f719e38f to 3e13d86285

2023-03-01 03:41:24 +01:00

Compare

Campbell Barton force-pushed pr-save-load-handlers-with-filepath from 3e13d86285 to 134bbc50fb

2023-03-01 03:46:25 +01:00

Compare

Campbell Barton commented

2023-03-01 03:51:11 +01:00

Updated the patch:

Add filepath argument to existing load/save pre/post handlers.
Add failed argument for post handlers.
Use event spesific functions as generic exec functions are no longer practical with multiple arguments of different types.
Removed init/failed handlers.

Updated the patch: - Add filepath argument to existing load/save pre/post handlers. - Add `failed` argument for `post` handlers. - Use event spesific functions as generic exec functions are no longer practical with multiple arguments of different types. - Removed init/failed handlers.

Campbell Barton requested review from Brecht Van Lommel 2023-03-01 03:51:19 +01:00

Brecht Van Lommel approved these changes 2023-03-01 15:56:02 +01:00

Falk David refused to review 2023-03-02 12:10:57 +01:00

Campbell Barton force-pushed pr-save-load-handlers-with-filepath from 134bbc50fb to fdfbccf012

2023-03-09 02:01:51 +01:00

Compare

Campbell Barton referenced this issue from a commit

2023-03-09 02:14:30 +01:00

PyAPI: extend save/load handlers, optionally take a filepath argument

Campbell Barton commented

2023-03-09 02:15:08 +01:00

Committed 46be42f6b1, closing.