search

getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
218 stars 49 forks source link

Merge Context Manager + Work Files (Fix #412) #442

Closed BigRoy closed 4 years ago

BigRoy commented 5 years ago

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?

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.

mkolar commented 5 years ago

Just realised that I commented on the issue rather than here. So for completeness here's clarification on my early comments https://github.com/getavalon/core/issues/412#issuecomment-530479801

tokejepsen commented 4 years ago

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.

BigRoy commented 4 years ago

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 workspace.mel.

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.

jasperges commented 4 years ago

@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.

BigRoy commented 4 years ago

Here's a preview of the latest functionality: avalon_workfiles_merged_simple_gif

Download zipped .mp4: avalon_workfiles_merged_simple.zip

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.

tokejepsen commented 4 years ago

Looks good!

Initialize Work directory might sound a bit too techy/cody. Maybe Create work area

davidlatwe commented 4 years ago

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.

BigRoy commented 4 years ago

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).

davidlatwe commented 4 years ago

Ah, I meant "New" for set context + open new scene.

I assume that one would open up the tool only for following cases :

  1. Changing work space to start something new.
  2. Changing work space to open up old scene.
  3. Save the file, but change work space if other asset/task selected on save.

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.

BigRoy commented 4 years ago

With https://github.com/getavalon/core/pull/442/commits/7ad657a075856b38dd95c21fcfbcbe8fa0060a4c 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:

jasperges commented 4 years ago

@BigRoy This new version almost works fine in Blender. If you open a workfile the Qt interface is locked and the only way to get it back or close it to open it again from within Blender. There used to be an explicit self.close(). Did you remove that on purpose? Is the UI supposed to stay open or should it close, like it used to?

To keep it working, I quickly added module.window.close().

Update: The problem is only on Linux, on Windows it works fine (it pains me to say this... :p)

BigRoy commented 4 years ago

Regarding the issue @jasperges mentioned for Blender. I checked it briefly with him. This isn't actually a problem with this particular interface but with the way the Qt interfaces are running there in general. Whenever any Qt interface is open with the Modal Operator and you open a file then it'll hang the Qt application. As such, on scene open with this Work Files tool it shows that issue.

Did you remove that on purpose? Is the UI supposed to stay open or should it close, like it used to?

In this case I did leave it open on purpose and maybe wanted to add a toggle "autoclose" or some setting. But I'm happy to also force either of the one. It's just much easier to debug without the auto-closing, and additionally some Artists mumbled it a couple of times whether it could be worth looking into just having it open as "explorer" when they need to check some scene from different assets, so I removed the automatic closing on purpose for now.

BigRoy commented 4 years ago

I've been working on tweaking the interface for the Work Files some more: afbeelding

This is based on some of @mkolar 's comments on #489 and in particular implementing point 3-5. Specifically having date modified there too, allow sorting by it and having a filename filter at the top.

I've also moved the Save As button to be next to the other ones, this allows for more screen space for listing files and makes the button sizes more consistent. @jasperges do you think this looks better?

I'll need to do some more changes to the code to have the file open/save functionality working again as I just quickly threw together the UI changes but haven't yet accounted for the change to Model+View as opposed to the previous QListWidget for the files. I think I can push it Monday.

BigRoy commented 4 years ago

I just pushed https://github.com/getavalon/core/pull/442/commits/d7130433f2afd2c7f32643d35f25267cf5e551ed which implements the UI as shown in this comment.

Artists on our end also requested to add a Filesize column. Any remarks regarding that? Might it get too detailed or cluttered? Any reasons not to add it?

Like before the open TODOs:

jasperges commented 4 years ago

@BigRoy Yes, I think this is better. Every button the same size and a bit more room for the files list.

About adding a filesize column: I don't mind it that much, although it will get a bit cluttered. Maybe you can add the functionality so users can choose which columns to show (right mouse menu), just like in a regular file browser. Then everybody can choose for themselves what to show.

BigRoy commented 4 years ago

About adding a filesize column: I don't mind it that much, although it will get a bit cluttered. Maybe you can add the functionality so users can choose which columns to show (right mouse menu), just like in a regular file browser. Then everybody can choose for themselves what to show.

That could definitely be nice, but it does add some more complexity which I currently think we don't need yet. Because it'd mean we'd need to store the preferences of the user too (otherwise they always need to set their settings again). Easiest would be then to store per machine, but then we might want to have it per user - where there is no User-specific yet in Avalon. I think for now it's best if we can manage to have these tools as simple as possible as it'll be easier to maintain.

jasperges commented 4 years ago

That could definitely be nice, but it does add some more complexity which I currently think we don't need yet. Because it'd mean we'd need to store the preferences of the user too (otherwise they always need to set their settings again). Easiest would be then to store per machine, but then we might want to have it per user - where there is no User-specific yet in Avalon. I think for now it's best if we can manage to have these tools as simple as possible as it'll be easier to maintain.

I completely agree with keeping it simple. The problem for me with more columns is, that it is already about preference. Not everybody has the same preference. So how do you determine what is a good baseline for everybody and when it's really a user preference? Apart from adding the functionality to view extra columns, can't you easily store the preferences in QSettings?

Again: for me it's fine to add the filesize column, but I would definitely draw the line there. If an artist wants to see more, they can always use the 'Browse' button and get more info there...

BigRoy commented 4 years ago

can't you easily store the preferences in QSettings?

Yes, you can. They would however be per machine and not per 'avalon user' (which is not a thing yet I suppose).

The problem for me with more columns is, that it is already about preference. Not everybody has the same preference. So how do you determine what is a good baseline for everybody and when it's really a user preference?

Agreed. I'll leave it as is without the filesize column. If it keeps coming up with the team then we could also look into our options for a future issue.

jasperges commented 4 years ago

@BigRoy This new version almost works fine in Blender. If you open a workfile the Qt interface is locked and the only way to get it back or close it to open it again from within Blender. There used to be an explicit self.close(). Did you remove that on purpose? Is the UI supposed to stay open or should it close, like it used to?

To keep it working, I quickly added module.window.close().

To come back to this: I changed the way Blender calls QApplication.processEvents(). This is not an issue anymore.

BigRoy commented 4 years ago

Initialize Work Area

Here's the Initialize Work Area in action. Does this look like the way it should be? It doesn't feel as smooth as I'd like but I can't think of anything that's more clear than this to the end user.

@jasperges @mkolar @davidlatwe any pointers?

workfiles_init_workarea

This resolves the Initialize workdir: TODO

Remove Set Context functionality

I have removed the "double click on task" functionality to explicitly set the current api.Session to another context than the current work file. Instead, just saving or opening in another context will first switch the context.

That way accidentally switching contexts is near impossible, and also avoids the complexity for the end user. @mkolar would that still allow all your use cases to happen?

This current avoids the need for Set Context functionality: TODO

This is now in a usable state.

tokejepsen commented 4 years ago

Any reason why work area creation cant happen on Save As?

BigRoy commented 4 years ago

Any reason why work area creation cant happen on Save As?

@tokejepsen Good question. Technically it could. Then before work area initialization it would just show no files (and maybe the model would just have a non-selectable placeholder "Work Area does not exist" and open and browse get disabled. It might be somewhat "confusing" though if you find out that the folder doesn't exist at all yet.

Would you want that to happen instead? What do others think? It's not a bad idea.

Aside of clarity the only reason I'm thinking of is that I'm not too sure how well App Work Directory initialization will work for others who launch with a different means than the Launcher. Specifically because it requires the "Application" name to be available and allow to trigger a .toml file for it.

tokejepsen commented 4 years ago

It might be somewhat "confusing" though if you find out that the folder doesn't exist at all yet.

Have noticed that artists hardly navigate the folder structure anymore when Workfiles is working.

Would you want that to happen instead?

Personally I would like to see necessary actions like work area creation hidden from artists, and simplify/speed up their workflow > Less button presses.

BigRoy commented 4 years ago

Personally I would like to see necessary actions like work area creation hidden from artists, and simplify/speed up their workflow > Less button presses.

Agreed. Here's the updated version.

workfiles_init_workarea_v2

davidlatwe commented 4 years ago

Will have a good look and test later today !

davidlatwe commented 4 years ago

So I have tested, and great work !

But, I would prefer having an extra button for initializing work area as the one in this comment. :relaxed: Because if I am starting from an empty scene, it's a bit weird to save anything for getting a workspace.

Or, how about just hide that "work area does not exists..." item, and auto create work area when Save As is pressed. If the Browse button is pressed, pop a warning dialog.

Is there any reason that one need to explicitly initialize work area ?

Oh and, would be better if the task view can display "Task" in header instead of "name". :)

BigRoy commented 4 years ago

Oh and, would be better if the task view can display "Task" in header instead of "name". :)

Good point.

Is there any reason that one need to explicitly initialize work area ?

Just so it doesn't constantly create folders as you browse. And in some scenarios the folders must exist as you work, e.g. with Maya's workspace.mel.

But, I would prefer having an extra button for initializing work area as the one in this comment. ☺️ Because if I am starting from an empty scene, it's a bit weird to save anything for getting a workspace. Or, how about just hide that "work area does not exists..." item, and auto create work area when Save As is pressed.

Wouldn't that still cause the issue that initially you'd be saving an empty work file? Not until you've saved you're still in the other context for your current scene, even when in an empty scene. You're just browsing other areas.

@davidlatwe @tokejepsen what if we just swap the bottom buttons Open, Browse, Save with one big button "Create Work Directory" at the bottom. That way the change to UI is not as odd as in my first draft? Or maybe just the "Save As" to "Create Work Directory" (even though it's likely too small to fit).

@mkolar what's your thought on this subject?

tokejepsen commented 4 years ago

I would avoid any unnecessary mouse clicks from the user. The main point of making the Workfiles app, was to make it faster to load/save your workfiles so the user's preference wasn't the DCCs native load/save workflow.

Because if I am starting from an empty scene, it's a bit weird to save anything for getting a workspace.

I'm not sure I follow this @davidlatwe ? If you have an empty scene and need to create a workspace, then wouldnt you have to save your file?

davidlatwe commented 4 years ago

If you have an empty scene and need to create a workspace, then wouldnt you have to save your file?

Yeah, that's true. I think I kept mixing the usage of context manager app when I imaging the workflow of workfile app...

The main point of making the Workfiles app, was to make it faster to load/save your workfiles so the user's preference wasn't the DCCs native load/save workflow.

I think you are right. Yeah, I could go with "Save As" to init work area if not exists. :)

mkolar commented 4 years ago

I actually think that it's very clear in this implementation. If the folder doesn't exists, save as will create it alongside the actual file. quick and clean. No extra clicks.

BigRoy commented 4 years ago

Yeah, I could go with "Save As" to init work area if not exists. :)

@davidlatwe does that mean the current implementation is good to go?

@mkolar @tokejepsen did you try the code on your end too? Or was it just a comment based on the GIF?

If everyone approves I'd still like to do the following before merging

tokejepsen commented 4 years ago

did you try the code on your end too? Or was it just a comment based on the GIF?

Based on the GIF.

If everyone approves I'd still like to do the following before merging

Sounds like a good clean. Nice!

davidlatwe commented 4 years ago

One little thing, the "Work area does not exists.." menu item is able to show context ment on right click, which shouldn't be. If clicked the menu's "Duplicate" action, error raised.

BigRoy commented 4 years ago

"Work area does not exists.." menu item is able to show context ment on right click, which shouldn't be. If clicked the menu's "Duplicate" action, error raised.

Well caught. Will need to patch that up, thanks!

I also noticed that, because it's a regular Item, whenever you would have a name filter set top right that you could be filtering this placeholder item too. Not a major issue but something to watch out for. Probably will need to see if we can easily disable the filter then.

jasperges commented 4 years ago

Great work Roy! Can't wait for this getting merged... :nerd_face:

Just a heads up: I opened #512 and made a PR (#513) for that. If people agree on that, these lines need to be changed from:

    if debug:
        api.Session["AVALON_ASSET"] = "Mock"
        api.Session["AVALON_TASK"] = "Testing"

to:

    if debug:
        import traceback
        sys.excepthook = lambda typ, val, tb: traceback.print_last()
jasperges commented 4 years ago

@BigRoy Any update on this? Can I help in any way?

BigRoy commented 4 years ago

Any update on this? Can I help in any way?

Thanks for tagging me! :) I'm a bit swamped with other stuff so haven't been able to put more time into this. You should be able to push into this PR (or if not set up a PR to my branch) to fix any outstanding issues that you think you can easily adapt for.

It's likely I won't have the time until end of next week to continue myself.

jasperges commented 4 years ago

No rush. I was just wondering. Just merge this to our production branch so we can finally start using Avalon! :rocket:

Personally I'm not aware of issues (apart from the excepthook, but that's part of another discussion.

I'll be patient or if I encounter issues, I will see what I can do. :)

iLLiCiTiT commented 4 years ago

I wonder, what is missing and needs to be done?

mottosso commented 4 years ago

If I'm not mistaken, it looks like what's missing is someone hitting that Merge pull request button.

Any additional fixes or features seem well suited for an additional PR. First one to press it wins 10 units of Avalon currency! Oh and you must also make a release for the new version it makes.

BigRoy commented 4 years ago

This would be on the to do (or a merge and posted as an Issue). And the bottom bullet points of this comment.

Most seem like small notes. Feel free to do a quick fix or otherwise put them in a separate PR if you consider the current state safe enough.

Go get them Avalon currency y'all.

davidlatwe commented 4 years ago

Context label has been removed in commit be07276, was there a replacement ? 0.0

iLLiCiTiT commented 4 years ago

@davidlatwe True that's my fault. But where to put context label when Edit Context.. action was removed?

davidlatwe commented 4 years ago

Can we change them into normal menu item instead ? Or keep the context manager for now, until artists get familiar with new workfile app ?

iLLiCiTiT commented 4 years ago

I'm not user so my word is not much valuable at this point, but I would rather add empty action with context label and without callback instead of keep context manager, because me as user wouldn't understand why he is there if I can do it through workfiles.

jasperges commented 4 years ago

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.) I think it's a good idea to show the current context somewhere, but inside a menu feels like the wrong place for that. Not sure where it should go, though...

tokejepsen commented 4 years ago

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. Maybe its taking too much responsibility on, but should the user be able to create tasks on assets through this UI?

jasperges commented 4 years ago

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.

BigRoy commented 4 years ago

So, does this mean it's ready to merge? 🔥🚀

Or is the Hound still an issue? Since I see a red ❌ on the checks.

tokejepsen commented 4 years ago

So, does this mean it's ready to merge?

Except for the hound violations?