Feature/blender workfile out of date handling #4933
Conversation
ynbot
added
type: feature
|
Can @Tilix4 be added to reviewers please? |
Sharkitty
force-pushed
the
feature/blender_workfile_out_of_date_handling
branch
from
89f047f
to
6ddd418
Compare
|
I try to test it but I dont see the related validator and extractor show up in the publisher. |
Hello! Is there any error message? And does the |
Hi, nothing appears in my scene, I checked OP menu and OP launcher too. but it doesn't show. |
Can you provide logs so I can have more information about the cause of your issue please? |
|
|
@moonyuet Can you try running |
it shows the errror like this: |
It's odd... I've never had any import issue with this file. Any chance you could try a blender 3.3LTS? I don't think blender version is the issue but I'd like to rule that out, as I developed and tested this feature with blender 3.3. You can use one of the |
I used blender 3.2.1, it could be because of my version doesn't match the version you mentioned. Will update you later after reinstall the blender version you mentioned. |
@moonyuet could you please check |
Yes. It exists in the openpype/plugins folder in the build version of openpype_console. Builtin Modules: bpy, bpy.data, bpy.ops, bpy.props, bpy.types, bpy.context, bpy.utils, bgl, blf, mathutils from openpype.hosts.blender.plugins.publish import validate_workfile_up_to_date |
There was a problem hiding this comment.
I'm not sure this file is necessary.
There was a problem hiding this comment.
You're right, my bad.
This pretty much looks like a plugin loading issue more than a code one. Don't you have any setting error message at OP's initialization? |
No. If there is any setting error, you can't edit any project setting in Openpype setting. I can see and edit the setting. One thing I find is I tried to add
|
You could try to add settings for the validator as well, to confirm if it can be imported on your end. Then we should find why it's not loaded when there is no setting. I really wonder where the difference lies between environments we've been using this feature on, and your environment, if it's a setting or something else entirely 🤔 |
|
|
Sharkitty
force-pushed
the
feature/blender_workfile_out_of_date_handling
branch
from
f1ed64d
to
340a6c0
Compare
I updated the branch if you want to try again 😄 |
|
I have done simple test the code but not sure if I did something wrong. It seems that the button "check workfile up to date" doesn't really check the workfile version? 2023-05-16.17-01-48.mp4 |
Did you publish all the workfiles you're testing with? This system is tied to publish time and not version number. The reason for this choice is that local workfiles version number may not match published version number, as users have the ability to increment their workfiles as much as they want to, without affecting published version numbers. And I do think they should absolutely keep this ability. Also you will only get the expected behaviour once your workfiles has been published once with this system. This is due to the fact that datas must be set in the blend file by the update publish time plugin. So if a workfile has never been published with this workfile out of date handling system, then the data is just not there and the system can't make the difference between what's up to date what isn't. |
The video recording shows the script doesn't find any last published version |
|
If it's confusing for you, surely it will also be confusing for someone else. I'll add some much needed documentation then. |
|
@Sharkitty I am unable to make this work. I get the error sometimes when I open some old workfiles, but if I click on the error, or click to check again if it is out of date, it says that it's up to date. I tried then publishing a new version of the workfile, just to test with two versions that have been published with this PR, and for both it says they're up to date. Although, from my understanding, the older one, that was published 5 minutes earlier, should result as outdated, or am I missing something? |
Sharkitty
force-pushed
the
feature/blender_workfile_out_of_date_handling
branch
from
ee81d87
to
e9d2ba5
Compare
Hello, I rebased this branch and tried to look into your issue, although I'm unable to reproduce it. When I open an uncomformed workfile like |
openpype/hosts/blender/api/workio.py
Outdated
| # Get date and time of the latest published workfile | ||
| last_published_version = get_last_version_by_subset_name( | ||
| get_current_project_name(), | ||
| f"workfile{get_current_task_name()}", |
There was a problem hiding this comment.
The subset naming can be customized as far as I know through settings - as such, it wouldn't be entirely safe to assume the subset name matches exactly this.
There was a problem hiding this comment.
Good point! I'll try to improve this.
|
|
||
| def process(self, context): | ||
| scene = bpy.context.scene | ||
| scene["op_published_time"] = context.data["time"] |
There was a problem hiding this comment.
Storing into the file itself - doesn't that mean it'd basically become 'incorrect' as soon as someone would save the workfile into another asset (or another task) since the time stored in the scene wouldn't relate to that new context?
There was a problem hiding this comment.
I guess so but is there a use case for that? I don't think that would be a very good practice, especially as there are other metadatas that would also be incorrect if you did that. I've already seen artists appending workfiles or elements of a workfile and ending up with a broken scene that could not be published (regardless of this feature). I think the same would happen with the scenario you are suggesting.
There was a problem hiding this comment.
I guess so but is there a use case for that? I don't think that would be a very good practice, especially as there are other metadatas that would also be incorrect if you did that. I've already seen artists appending workfiles or elements of a workfile and ending up with a broken scene that could not be published (regardless of this feature). I think the same would happen with the scenario you are suggesting.
Could you elaborate on the other type of metadata? We re-use workfiles all the time for other shots. If there are features currently disallowing that I'd say that's a bug and potentially a massive design flaw.
Would love to see reports of issues faced with that.
There was a problem hiding this comment.
In the current implementation of blender, metadata are stored in instance collections under the avalon key, aka AVALON_PROPERTY. They contain data such as the family, asset name, subset name, task name and variant.
To have more precise information about whether or not that would be problematic in the current version of openpype, I made a simple workfile with just a model subset containing the default cube. The asset name for this workfile is Choochoo. I saved as in another asset (Pingoo) folder using the correct syntax for the filename. Then I closed blender and reopened it as that other asset and I checked the metadata of the modelMain I created:
>>> for key, value in C.active_object.get('avalon').items():
... print(f"{key}: {value}")
...
id: pyblish.avalon.instance
family: model
asset: Choochoo
subset: modelMain
active: True
variant: Main
task: Fabrication
You can see that my modelMain is still recognized as part of the Choochoo asset. On one hand this is true. On the other hand, an artist who decides to use this subset as a base for a model in Pingoo, could publish its modified model thinking this is fine because they clicked on Pingoo earlier, yet the modified model will go in Choochoo. And that can be kinda dangerous. So I did it, and while the workfile was published in Pingoo, the model was published in Choochoo. In this case what should be done is to create a new instance and not publish the one already present in the workfile. In the context of a shot the same could happen with an animation subset for instance.
Does that work differently in other hosts?
All that being said, if you think this should remain doable in blender without confusing the workfile up to date system, I can look into it. Out of the blue I think I could check the asset name to see if the workfile has been moved around and adjust the behavior accordingly.
…ad_and_save Refactor: Split process into download and save_as
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| last_workfile_path, last_published_time = download_last_workfile( | ||
| project_name, asset_name, task_name | ||
| ) | ||
| if local_workfile_path := save_as_local_workfile( |
There was a problem hiding this comment.
missing whitespace after ':'
multiple statements on one line (colon)
whitespace before ':'
There was a problem hiding this comment.
That line follows PEP8 convention afaik.
There was a problem hiding this comment.
I think hound doesn't recognise the walrus operator, ignore it
There was a problem hiding this comment.
Walrus operator was added in Python 3.8+
Are we still supporting any Blender versions that might be using a lower Python version?
There was a problem hiding this comment.
Also, this code is exclusively executed by Blender, which is now running only versions above py3.8
openpype/hosts/blender/api/lib.py
Outdated
| ) | ||
|
|
||
| # Match subset wich has `task_name` in its name | ||
| low_task_name = task_name.lower() |
There was a problem hiding this comment.
Why not use it directly like task_name.lower() in line 390?
There was a problem hiding this comment.
Why did I do this in the first place <.<
openpype/hosts/blender/api/lib.py
Outdated
| subset_id = filtered_subsets[0]["_id"] | ||
|
|
||
| if subset_id is None: | ||
| print( |
There was a problem hiding this comment.
Better to use the log (same for other prints)
| if scene.get("op_published_time"): | ||
| return last_published_time <= scene["op_published_time"] | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Wouldn't it be better to return True in this case? I guess this happens when users start using this feature in existing projects, and they have published workfiles already. I think it would be confusing to suddently get a message that their workfile is not Up to date.
There was a problem hiding this comment.
On one hand yes, and I agree that it could be confusing. On the other hand, in this fallback state where the system can't determine if the workfile is up to date or not, I think it's safer to wrongly mark it as out of date, in which case the worst thing that could happen is downloading a workfile while it wasn't necessary, than to mark it as up to date, in which case we would risk publishing an out of date workfile, which is all that this system is trying to prevent.
This is why, in my opinion, returning False is the way to go. In practice, as I think downloading last workfile is what should generally be recommended to artists, and there is no risk in doing it. Artists will most likely just click ok and not worry too much about it.
There was a problem hiding this comment.
I agree with Sharkitty. As this is a new feature, releasing a new OP version must come with information for artists about new behaviours they might encounter.
Also, if we set it to True, we may leave issues to go through, like some one creates by mistake a new workfile from scratch (build or whatever), this workfile won't contain the key, and then he tries to publish. The user will have no any warning or anything to think about what is going on.
The fix could be to register a handler at the file opening to set this key with the appropriate value if it doesn't exist.
|
Because we're splitting OpenPype into ayon-core and individual host addons, this PR would have to be re-created to target one of those. We're closing it down, but we'll be happy for a new PR to ynput/ayon-blender repository once it's up. That being said, this kind of functionality should rather be outside of hosts, to keep consistency across workfile behaviour. |
Changelog Description
Adding workfile out of date handling to Blender host. This feature is important to avoid users to work on an out of date version of their workfile without knowing it, then publishing. Which would imply that newly published subsets don't contain the latest changes their coworkers have made.
This comes with workfile up to date checks:
If the open workfile is the latest, a popup will be displayed to notify the user that their workfile is up to date (which is discarded as soon as the cursor is moved away from the popup. On launch, you may not see it if you are multitasking).
If the open workfile is out of date, a popup will be displayed to notify the user about it, and propose them 3 actions:
This popup is made to not reappear if cancelled, so users can't ignore it, as this is important and should not be missed.
A warning sign will appear in the OpenPype menu with the indication that your workfile is out of date, so you'll always have a reminder if you are using an out of date workfile. The warning is clickable and has the same behavior as the check workfile up to date button (except it only appears when the workfile is already marked as out of date).
During publish, a validator checks if the workfile is up to date, and fails if it isn't. This validator is optional, enabled by default. If at any point a rollback is necessary (or if the workfile out of date handling bugs and wrongly marks a workfile as out of date), this validator can be ticked off, allowing to publish an out of date workfile on purpose. I see this manipulation as a way to confirm that "I know what I'm doing, I want to proceed with the publish".
Additional info
This system stores last publish date in the workfile, so on a workfile that has been created without using this system, their may be issues with workfiles wrongly marked as out of date, which can be resolved by publishing once (since publish time is set in the workfile during integration). This implies that deploying this feature requires a conformation, but that can reasonably be done on the go as assets are being updated to meet production needs.
A publish failure during integration phase may cause the workfile to be marked as out of date. There are mitigations for this issue in place, but if it still happens it is safe to tick off the workfile up to date validator.
Testing notes:
As explained in additional info, if this is tested on an existing asset, a publish may be necessary to have the expected behavior (so the workfile is conformed for this feature).
If the workfile out of date popup appears:
If the workfile is up to date it should be seamless to users, no warning and the validator should pass.
After performing a rollback by ticking off the workfile up to date validator, the workfile should be considered up to date at the end of the publish.
Testing tip: in the workfiles menu, copy and open an older workfile if you need an out of date workfile for testing.