Merge Context Manager + Work Files (Fix #412) #442
Conversation
- The root is now not passed to the Window anymore, but always retrieved using the Work Files API of the host with host.work_root()
|
Just realised that I commented on the issue rather than here. So for completeness here's clarification on my early comments #412 (comment) |
|
Have recently been using the Context Switcher more, and I immediately go to the Workfiles app afterwards, so merging the two would make it very streamlined. |
|
I've been recently fiddling with the Work Files API to allow the tool to list the files even without requiring to switch the context prior to it. Admittedly it's a bit hacky, especially with the root dir in Maya being dependent of the Anyway, in the end it seemed to be fast and work quite well. I think from there it's a matter of cleaning up and simplifying again. I hope to share some bits of it in about two or three days as my schedule this week is quite busy. Just wanted to let you know I am back on this PR. |
|
@BigRoy That's awesome! This would also help in adopting 'Allzparkalon'. And like @tokejepsen is saying: most of the time when you switch context you open the Workfiles app anyways. |
|
Here's a preview of the latest functionality: Download zipped The little log window at the bottom is Maya's script editor and is not part of the Work Files UI. To-do:
Any first thoughts are more than welcome. |
|
Looks good!
|
|
I like the ideas in #489, will they get implemented in this PR, or in another one ? How about adding a "New" button, so there should be pretty much what artists will do when they open up context manager. Then I think we won't need double clicking to set context and the Breadcrumb. |
@davidlatwe Sorry I'm not sure I get what you mean. Whenever you launch the host it should have already initialized the current application. Or at least that is what Avalon currently does as it launches the host. So for the current context you would never have to "create work area" since it already exists. It would by default just show an empty folder on the right hand side. Where I'd want to add a single non-selectable greyed out entry in the list stating: "No files. Use Save As to save your first file." The tricky thing is that as soon as you click on another Asset or Task you're operating in an area that is not related to you active Context/Session. Previously it came up that there could be reasons to force yourself into the workspace context, even without Saving/Opening a file there which @mkolar mentioned before. It would basically be, setting that context and doing some work prior to saving there if I understood correctly. Exactly that is what the "buttons" would allow, to if you want set the context. However, in most scenarios I assume an Artist doesn't care and will just open the scene (which will always first ensure the context is set appropriately). |
|
Ah, I meant "New" for set context + open new scene. I assume that one would open up the tool only for following cases :
So if there's a "New" button for opening new scene, I think there wouldn't need double clicking to set context and the Breadcrumb. And in the end, I think artist will no need to be aware of the concept of "context" when using this tool as long as there's only "New", "Open", "Save" these there buttons being presented. |
# Conflicts: # avalon/tools/workfiles/app.py Note this changes the fix from getavalon#465 to not store the widgets as explicit variables but by setting the parent widget
| @@ -39,11 +39,10 @@ def current_file(): | |||
| return current_filepath | |||
|
|
|||
|
|
|||
| def work_root(): | |||
| from avalon import Session | |||
| def work_root(session): | |||
There was a problem hiding this comment.
Each Work files API now requires session for the work_root to be able to compute the Work Root for a non-active Session object (e.g. not api.Session)
| changes["AVALON_SILO"] = asset_document["silo"] | ||
|
|
||
| # Update hierarchy | ||
| parents = asset_document['data'].get('parents', []) |
There was a problem hiding this comment.
This code has moved and fixes a BUG where previously if asset had not changed with the call then asset_document would not have been a set variable.
| @@ -926,61 +926,103 @@ def get_representation_context(representation): | |||
| return context | |||
|
|
|||
|
|
|||
| def update_current_task(task=None, asset=None, app=None): | |||
| """Update active Session to a new task work area. | |||
| def compute_session_changes(session, task=None, asset=None, app=None): | |||
There was a problem hiding this comment.
I've made this a new function to compute the changes to the current session required to become a new valid session. Before we only had update_current_task which did the same but would always change the active session. That computing of the required changes is now moved to this separate function.
We might want to come up with a better name and maybe expose it in the API?
There was a problem hiding this comment.
Or not expose compute_session_changes in API, but add a new arg called dry_run=False to update_current_task ?
If dry_run set to True, then return early without update.
There was a problem hiding this comment.
And maybe need to add one more internal arg called _changes=None to update_current_task, so we can pass the changes that returned from compute_session_changes into update_current_task to avoid double work.
There was a problem hiding this comment.
The dry_run argument does sound like a decent alternative to having this in a separate function. @mkolar @mottosso any preference on this one? Separate functions like this or make one and add dry_run argument?
I'm not too big a fan of adding the _changes internal argument. It seems like a premature optimization that for now seemingly adds redundant complexity.
|
|
||
| # Update silo and hierarchy when asset changed | ||
| if asset: | ||
| if isinstance(asset, dict): |
There was a problem hiding this comment.
This has now additionally been allowed to be passed a database document for an asset as opposed to only working by its name. This allows to avoid the additional query to the database.
avalon/tools/workfiles/app.py
Outdated
| # Refresh breadcrumbs | ||
| #self.widgets["header"].set_session(session) |
There was a problem hiding this comment.
Disabled widget, see comments above.
| def show(root=None, debug=False, parent=None, use_context=True): | ||
| """Show Work Files GUI""" | ||
| # todo: remove `root` argument to show() |
There was a problem hiding this comment.
Since now the Work Files tool can browse to different assets and tasks setting an explicit root makes no sense, since the root is computed based on asset/task work root.
As such, we can remove the root argument. Should we preserve this for API compatibility?
There was a problem hiding this comment.
Just pop a warning if root in kwargs ?
|
With 7ad657a I've pushed the new prototype functionality. It's not flawless yet but it works to some extent. I've tried to comment on most of the important/big changes or odd choices above. Hope that helps reviewing what's there so far. Regarding earlier TODO:
There's a lot to potentially check:
|
|
I'm also not a user, but I think it's a good thing it has been removed, because I don't think it's ever needed to switch the context without loading or saving a work file. (Although we probably have the need to publish several character rigs from one base rig. So inside the file we have to run the Creator for different assets... but that is an exception, not the rule.) |
|
Just tested this out myself and it works nicely! One question did pop up. If there arent any task assigned to the asset, you wont be able to save a workfile, so you would have to open the project manager to create one. |
|
Personally I don't think so. I think most of the time this is not something an artist should do (a production manager/coordinator should do this or automated from Shotgun/FTrack/Kitsu) and definitely not in the workfiles app. Just my two cents. |
|
So, does this mean it's ready to merge? 🔥🚀 Or is the Hound still an issue? Since I see a red ❌ on the checks. |
Except for the hound violations? |
|
Ahh, fair enough. Ready to merge! |
|
But what about the context label ? :O I can agree that context label is not good to be in side the menu, but it's a place that work for most of the DCC hosts for now to indicate current context, right ? |
|
How about changing the window title? |
That did cross my mind but should be in a separate issue/PR to discuss. Anyway, if t's just me, then maybe it's okay to be merged. Either add back context label as a normal menu item, or merge it as is, both work for me now. Still, IMHO, deprecating context manager with new workfile App is one thing, removing context indicator altogether is another, so I think this kind of UI change should be brought up earlier. 💡 |
|
I've tried to add not enabled action item to menu PR: BigRoy#5 |
Totally agree that these shouldn't be taken lightly. I just envisioned that to be part of it and assumed the discussion was already debating that too. Anyway, shall we include BigRoy#5 and then open an additional issue to track any better ways to approach that @davidlatwe ? |
That would be great :D Just pulled and tested that branch, and only require one tiny fix and good to go for me ! |
Just pulled fix. Can you check please? |
|
Perfect !! 🚀 |
added context item to avalon menu
|
Ready for take-off! |
|
I've set So I do get Am I missing something? |
|
If you are using Avalon Launcher, maybe you need to head back to first page or restart the launcher so the environment vars get updated from those |
|
Just realized small detail... Will that context label change on context change? |
|
Yeah, it did change in my test, and still does. |
|
@jasperges is everything working there now ? |
|
@davidlatwe On Monday I can check again. I don't have access to Maya at home. Will let you know. |
|
@davidlatwe All is good! I still have some small issues, but I'm pretty sure that's because of the Avalon configuration. So, go, go, go! 🚀 |
|
Merging this ! |
This is a work in progress implementation for issue #412 that describes to merge the Context Manager and Work Files tool into one single tool that handles both, as usually the actions performed with these happen in relation to each other.
What's changed?
app.pyaccording to the contribution guidelines, however I think this will introduce quite some discussion and pointers on how to improve - as such I'm opening this PR In an early stage to ensure as much as possible this can be discussed and I can improve it as needed.Note: This is a work-in-progress and I'm opening a PR to kickstart a discussion on code style, but I'm fully aware this is not as clean as it can be. However, even at this stage, code style ideas or notes according to the contribution guidelines are very welcome.
Note 2: I've added some todos that are more notes to self as I'm developing and changing some minor things purely for testing/development sake. Feel free to be captain obvious and point out if you have questions - just wanted to highlight that many are reminders to myself.