Make Silos optional - Githubissues

mkolar commented 4 years ago

Problem

The concept of silos becomes problematic as projects grow larger and longer. The problem is visual where it's uncomfortable to browse through them with tabs, but also it's a bit limiting in terms of project structure.

Suggested solution

We suggest dropping them completely and instead consider any asset witch has visualParent set to Null as highest hierarchy member to be shown directly under the project in the GUIs. We've run this at all our sites for around 6 months now, so the key question is backwards compatibility, which I hope we achieved.

This is what the loader for example looks like without the silo tabs.

This is a revival of an old PR #372 that was closed after we discovered more caveats to this. General consensus seemed to be towards this change though so hopefully this will work for everyone too.

If someone could give this a shot it would help a lot. Unfortunately, our setups have quite some dependency on Pype and it's hard to be sure that the backwards compatibility doesn't catch on something somewhere on projects of others.

davidlatwe commented 4 years ago

I like this change !

The concept of silos becomes problematic as projects grow larger and longer.

This also emerged in a couple of big projects here.

so the key question is backwards compatibility, which I hope we achieved.

At first glance, I think you guys nailed it, I quickly checkout this branch and looks great !

The silo icon is different than yours, but I think what I see here is the correct one.

![image](https://user-images.githubusercontent.com/3357009/77141567-57ee8e80-6ab8-11ea-84ad-8ce215e2a3e0.png)

I have done some model publishes in both new and old assets, seems working just fine. :)

One problem though, Loader's subset tree gets reseted when the refresh button is pressed, the only way to get that tree updated is switching to other asset and back. Any idea ?

davidlatwe commented 4 years ago

Oh and, I haven't review all the code, need more time to do that. ☺️

iLLiCiTiT commented 4 years ago

The silo icon is different than yours, but I think what I see here is the correct one.

The "database" icon is for "silo" which we don't use at all. We use only visualParent hierarchy.

One problem though, Loader's subset tree gets reseted when the refresh button is pressed, the only way to get that tree updated is switching to other asset and back. Any idea ?

You mean that there is missing refresh button for subsets tree? We agreed that there is no reason for crubling gui with another button with ephemeral advantage. Refresh button in asset tree can refresh both with minimal performance affect. But can be changed if you don't agree :)

iLLiCiTiT commented 4 years ago

We've realized that parenting and stylesheet issue should be resolved in new PR. So I've move stylesheet setters where they were and removed parenting.

mkolar commented 4 years ago

I believe all the comments we resolved by this last commit now. Could do you guys think @BigRoy @davidlatwe ? Can we move forward with this?

All our other PRs are quite dependent on this one, so we'll wait with further work until this is approved and merged.

mottosso commented 4 years ago

Style wise I think this looks good, and the PR is well focused and to the point, thanks for that. I'll leave it to David and Roy to judge the functionality.

BigRoy commented 4 years ago

First off, great work on this PR. I pulled it and it instantly seemed to work on our existing projects with backwards compatibility. Good stuff.

I do have some additional notes and issues after testing it - some are related to the new style projects (and some are notes for others trying to adopt on how they might need to tweak their studio config):

Whenever you work in a project that had silos in use then the Asset Create dialog will open up with the "silo" text field added which needs to be filled in manually. However, when trying to add a child asset underneath a parent this gets extra confusing since that should just inherit the "silo" from the parent (if it has any). Technically otherwise I could also add an asset underneath silo asset like characters/hero and set its hero silo to: film, a world of ambiguity and confusion will open up. What about disallowing the setting of "silo" if it has a parent AND if it has a parent with silo to inherit that silo. That way it should be fully backwards compatible on that front.
Clicking refresh on the Asset Widget in the Loader does seem to reselect the original asset that I had selected but it seems to deactivate and clear the Subset/Publish Widget. Shouldn't that reselect/refresh also reshow the publishes for the current asset. It's a bit confusing that it does keep the asset selected but doesn't keep its publishes on the right hand side. (Not sure if this is a new bug) This same issue is apparent when the Loader was opened previously and you reopen it. It uses the same Loader application that was still in memory and forces a refresh, it reselects the asset on left hand side but show no published subsets on right hand set. Only clicking into another asset and back into the current asset refreshes it manually.
On creating a new project by using an empty older project template I ran into this error on creating a new asset.:

afbeelding

Did I do something wrong or should I be setting something specific on my project to make this work? This seems to be an issue with the Project managers avalon.tools.projectmanager.lib.create_asset() which is trying to use the old 2.0 asset schema. Should this be updated to 3.0 when silo is NOT required? Also the docstring for that function seems outdated since it states it always requires silo. Anyway, changing that to asset-3.0 allowed me to create assets.

Then the next problem is that the Avalon Launcher is unable to show these assets that have no silo. It will just list an empty project due to it explicitly listing silos as opposed to top-level assets. Unfortunately a backwards compatible PR would then also need to be made there I suppose.

With a little hack I then "went into that project" inside the avalon pipeline without the launcher. However, then I noticed the Work Files tool started messing up things on switching between assets:

# Traceback (most recent call last):
#   File "D:\git_pipeline\avalon\development\git\core\avalon\tools\workfiles\app.py", line 631, in on_save_as_pressed
#     self._enter_session()   # Make sure we are in the right session
#   File "D:\git_pipeline\avalon\development\git\core\avalon\tools\workfiles\app.py", line 486, in _enter_session
#     api.update_current_task(asset=self._asset, task=self._task)
#   File "D:\git_pipeline\avalon\development\git\core\avalon\pipeline.py", line 1057, in update_current_task
#     os.environ.update(changes)
#   File "C:\Program Files\Autodesk\Maya2020\bin\python27.zip\os.py", line 461, in update
#   File "C:\Program Files\Autodesk\Maya2020\bin\python27.zip\os.py", line 422, in __setitem__
# TypeError: must be string, not None

I believe that might be due to this setting it to None and then that being passed to os.environ.update() later down the line. Potentially that should install fall back to an empty string .get("silo", "")? That seemed to do the trick for me. Or potentially even better set it to asset_document.get("silo") or "" so that if the database returned a value of "None" stored for silo that it wouldn't run into the same issue.

Then I noticed that our project template included {silo} to decide where to save its assets. As opposed to giving an error on "requiring silo" in the project template it just created the asset without it.. this resulted in a path on disk like {project}/{silo}/{asset} turning into test/None/foobar on disk which is both unexpected and unwanted. I would stress that it should force the project to "require silo" of the project template uses it. Yes? --> However, after a full restart of Maya it instead resulted in a path like: P:\Projects/test2//dev_test/work/model/maya due to {silo} turning into an empty string. I'd then still argue that it should error on the template requiring silo to be a non-empty value.

Then of course I noticed some of our publishing plug-ins in our config required the silo data and information, including our integrator. Updating that to asset.get("silo") showed thatNone` is not a valid value for silo anyway. In particular on the Representation schema in its context data.


Traceback (most recent call last):
File "D:\git_pipeline\avalon\development\git\pyblish-base\pyblish\plugin.py", line 517, in __explicit_process
runner(*args)
File "D:\git_pipeline\avalon\development\git\colorbleed-config\colorbleed\plugins\global\publish\integrate.py", line55, in process
File "D:\git_pipeline\avalon\development\git\colorbleed-config\colorbleed\lib.py", line 433, in process
self.register(instance)
File "D:\git_pipeline\avalon\development\git\colorbleed-config\colorbleed\lib.py", line 483, in register
version=version
File "D:\git_pipeline\avalon\development\git\colorbleed-config\colorbleed\lib.py", line 739, in create_representations
schema.validate(representation)
File "D:\git_pipeline\avalon\development\git\core\avalon\schema.py", line 59, in validate
resolver=resolver)
File "D:\git_pipeline\avalon\development\git\core\avalon\vendor\jsonschema\validators.py", line 428, in validate
cls(schema, *args, **kwargs).validate(instance)
File "D:\git_pipeline\avalon\development\git\core\avalon\vendor\jsonschema\validators.py", line 117, in validate
raise error
ValidationError: None is not of type u'string'

Failed validating u'type' in schema[u'properties'][u'context'][u'properties'][u'silo']: {u'type': u'string'}

On instance[u'context'][u'silo']: None

This last one was resolved by only inlcuding that data when silo was set:
```python
            silo = asset.get("silo")
            if silo:
                representation["context"]["silo"] = silo

Sorry for the extensive block of text. I just wanted to make sure I accurately wrote down the issues as how they occurred to me.

Unfortunately in its current state this PR is unusable in production for us due to the issues described above.

mkolar commented 4 years ago

Sorry for the extensive block of text. I just wanted to make sure I accurately wrote down the issues as how they occurred to me.

no worries, this is exactly the testing we need. All our projects either don't use silo, or are temporarily created without it, but it's really hard to catch there errors without testing on existing project with silo, so thanks for that!

We'll go through it and get back here, but it all seems very fixable.

davidlatwe commented 4 years ago

It looks like most of the issues @BigRoy mentioned have been resolved ! 🎉

But I think instead of querying database with io.distinct.("silo") to define the project is silo required or not, how about just looking into project's publish path template to see if keyword {silo} exists ?

iLLiCiTiT commented 4 years ago

how about just looking into project's publish path template to see if keyword {silo} exists ?

I like it. We can't use that because we don't set templates in project's document, but that's our issue we can easily solve. What others think about it?

davidlatwe commented 4 years ago

Also, should we make Project Manager App to add ["data"]["parents"] field into asset document while creating asset in project that has silo opt-out ? (to adopt "AVALON_HIERARCHY")

One additional question, when using AVALON_HIERARCY, does that produce a folder structure that work and publish dirs may live with other child asset folder ? Like:

    characters
     └ boys
        ├ boyA
        │   ├ publish
        │   └ work  # Working on Modeling, Rig, LookDev
        ├ boyB
        │   ├ publish
        │   └ work  # Working on Modeling, Rig, LookDev
        ├ publish
        └ work  # Working on LookDevs or Crowd Assets

mkolar commented 4 years ago

One additional question, when using AVALON_HIERARCY, does that produce a folder structure that work and publish dirs may live with other child asset folder ? Like:

yes that is possible if you have task on boys you'll get work and publish folder there. We actually use it for editorials and similar types of work.

iLLiCiTiT commented 4 years ago

how about just looking into project's publish path template to see if keyword {silo} exists

Looking "only" into publish template, or all templates set in project's config? Still is possible that workfile has "{silo}" inside. (small chances but possible)

mkolar commented 4 years ago

Anyone had time to give this another shot? We have other stuff waiting for being put into PRs but all of it's is dependent on this one.

mottosso commented 4 years ago

Sorry this is taking so long! How about we merge this as-is, and deal with issues as they arise? It's a little too large to be entirely confident ahead of time that nothing would break, but it looks like a step in the right direction.

davidlatwe commented 4 years ago

Sounds good, better to move forward ! Should we make a release before merging this one ?

mottosso commented 4 years ago

Sounds like a good idea, could you take care of it?

davidlatwe commented 4 years ago

Done. 👍 https://github.com/getavalon/core/releases/tag/5.7.15

mottosso commented 4 years ago

Bam!

getavalon / core

Make Silos optional #535

Problem

Suggested solution