search

getavalon / core

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

Support silo assets #372

Closed mkolar closed 4 years ago

mkolar commented 5 years ago

Somewhat related to #308

New Functionality

This PR turns silos into full assets with option to hold tasks as a first step towards hopefully eradicating them altogether at some point :).

Implementation

We propose simply considering every asset with silo data member set to None as a silo itself. That solution is not 100% foolproof, but it's certainly a straightforward and simple one. It's fully backwards compatible as it simply combines any 'asset silos' with previously defined 'attribute silos' in all the GUIs.

To be able to display silo tasks, we had to tweak the display logic a bit. To display tasks on the silo (which presents itself as a tab in the GUI) user simply needs to deselects all in the treeview. This behaviour overrides the previous where you would see project tasks by default.

BigRoy commented 5 years ago

Went over the code, seemed alright - but at first sight it might need a clarification here or there on how it works.

We propose simply considering every asset with silo data member set to None as a silo itself.

This helps with explaining it though - it's just that the code took me a bit to grasp why all the if statements were needed.

mottosso commented 5 years ago

Sorry, just saw this PR.

It isn't clear to me what problem was this solving? There seems to be some confusion about what "silo" is and why it exists; it's a filter, much like families for Pyblish plug-ins. It was put in place to avoid separating between what was a shot and what was an asset (or episode, or sequence etc.) such that every object in the database could be just an "asset". But the user still needed to visualise shots differently from assets, and so silo was born.

I've seen screenshots now of additional silos, like editorial which makes a lot of sense I think. In theory, we could have lots of filters for the various GUIs, the visualParent is another one. Things that only apply to GUIs and not to our pipeline tools.

Dunno, does that change anything about this PR? Do we still want silos to be assets? (seems very strange to me!) Maybe we should rename them filters?

mkolar commented 5 years ago

As we work with this many more problems with silos actually arose for us. This is a first step to solving them.

  1. Yes silo is a virtual concept that is technically just a filter. However when we try to relate to them in the overall project hierarchy we're constantly hitting walls. The main one was the fact we need to have tasks on the top level hierarchy members...so technically silos. This is for editorial, but also some development tasks. Hence we needed to add this possibility and the only way to do it is to turn silos in to normal asset.

That being said. We're not saying silo should be an asset, but rather "the highest asset in hierarchy should act as silo in the GUI. so effectively we're ignoring silo and just didn't want to do a rewrite of all the GUIs to support not having them.

  1. Second step would be dropping them completely. Starting using filters as you said so we can build GUIs that visually reorganize it how we want. The tabs we have for silos now for example are only useable if you have 2-3 silos. Our projects regularly have 4-5 and more top folders, so it's suddenly not very user friendly.

Long story short I absolutely agree that all these filters (including silo) should only affect GUI. However because we have to act fairly fast with production requirement, we need to take these features step by step to make clients happy. In this case for example dropping silo gave us the ability to make sure that if we redo this categorisation later on, everything can be made backwards compatible very easily as we only added new assets to db.

tokejepsen commented 5 years ago

the highest asset in hierarchy

Sorry, could you clarify what you mean by this, since the asset are in a flat hierarchy on the dB? Do you mean the visualParent hierarchy?

mottosso commented 5 years ago

I have the same question.

Don't you already have a concept of (visual) hierarchy using the visualParent attribute, and could you not use that in-place of how you currently use silo?

mkolar commented 5 years ago

Yes I mean visual hierarchy. And yes we do have it from visualParents.

And frankly Id be happier is we removed reference to Silo from this and instead found assets without visualParent and used those as tabs...

mottosso commented 5 years ago

I think that could work!

It'd be compatible with the Mongo documents already, and look and act identically to silos as far as users are concerned (since it's basically just moving a value from silo to another attribute visualParent), plus it doesn't eliminate the possibility for filtering by things other than hierarchy, like arbitrary tags.

The only addition is I would probably keep visualParent in those "top-level" documents, but let it be None. That way, it's clear that they are meant to be part of a hierarchy, and that they don't have a parent and must be on top.

mkolar commented 5 years ago

Yes keeping as none is the way to go.

Were doing that with silo now.

tokejepsen commented 5 years ago

The only addition is I would probably keep visualParent in those "top-level" documents, but let it be None

Would that mean that visualParent is turning into a required attribute of the schema?

mottosso commented 5 years ago

Would that mean that visualParent is turning into a required attribute of the schema?

Good question; there's probably no rush to do so, we can see how things play out the way things are currently.

But if it does end up in the schema, I wouldn't want it to refer to anything visual. Visuals should interpret abstract data, not the other way around. So we may want to consider renaming it to e.g parentHint or the like, and interpret that as a "visual hierarchy" wherever it's used. Just so we don't start blurring the line between data and what is strictly related to drawing that data.

mkolar commented 5 years ago

Even though it's technically not required now, Without it none of the GUIs make any sense, so it's almost sorta required already ;)

mottosso commented 5 years ago

Ok, so this issue is about making the tabs of the Loader GUI refer to top-level assets with visualParent set to None. A GUI change, effectively, rather than anything in the DB or application logic.

We could then technically keep silo, for the time being, even though it wouldn't be represented anywhere.

Does that capture the requirement fully? If so, let's either update the description or make a new issue.

mkolar commented 5 years ago

yes. for us that captures it and would be preferable to us. We'll update this PR and the code to reflect.

mottosso commented 5 years ago

Happy to pull the trigger on this to keep the ball rolling.

mkolar commented 5 years ago

Happy to pull the trigger on this to keep the ball rolling.

Same for me. Just a confirmation that it doesn't currently break anything in their current project from @BigRoy would be good

BigRoy commented 5 years ago

Sorry, unable to test right away - but possibly near end of day or tomorrow. Bit crazy days here. Am keeping an eye on this though, hopefully soon I can join the Russian Roulette.

mkolar commented 5 years ago

after large cleanup, we decided to make this PR again, but a bit cleaner. to properly get rid of silos, we first need to deal with restructuring GUIs to more flexible organisation and remove silos after, to avoid any ugly surprises. We'll PR fresh very soon.

functionality will stay the same.

mkolar commented 4 years ago

I'm closing this PR as it is not relevant anymore. We'll follow up with a much simpler and backwards compatible one.