Asset Pipeline v2 #145
No reviewers
Labels
No Label
Kind
Breaking
Kind
Bug
Kind: Community
Kind
Documentation
Kind
Easy
Kind
Enhancement
Kind
Feature
Kind
Proposal
Kind
Security
Kind
Studio Request
Kind
Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: studio/blender-studio-tools#145
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "(deleted):feature/asset-pipeline-v2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is a re-design of the asset pipeline add-on. This Add-On was designed to allow multiple artists to collaborate while contributing to a common Asset. It enables simultaneous work on the same asset by multiple artists. The add-on works by tracking what data each artist contributes to the asset and merges the assets together into a final "Published" asset. This published asset is marked to be discovered by Blender's asset manager.
Demo
Tracking for Further Issues
studio/blender-studio-pipeline#170
Stuff that works:
Transferable Data (only matching topo for now)
No new transfer data foundtoNo new local transfer data foundTODO
UI Changes
Must Haves (High)
Get an MVP of each of these done, then move them to lower priorities
Medium
RIG.name == MOD.nameLow
For Next Production
BUGS
3756abad65to1c2693725ffind_drivers9cc346d8c2get_other_ids()b79b5965ddget_other_ids()874069ff2d_gen_transfer_data_map237acc7f52Very cool to see this taking shape! Really like how much more transparent this is already looking than what we previously had.
Small Disclaimer:
I haven't gone into a lot of detail with the testing yet. I simply used Met's example rig setup to get a feeling for the base functionality but I want to already give a batch of feedback and we can go from there. Later it would be good to start a character setup from scratch and go into more complicated details of potential merge conflicts.
Some initial feedback:
I'm not 100% sure what aspects aren't ready for feedback yet, so take it with a grain of salt.
Probably not that relevant right now, but I'd stick to the file naming scheme we had. E.g.
riggingrather thanrig. The file contains more than just the rig, it sounds a bit misleading to me and should be rather task oriented than data oriented.There were some nice aspects of keeping the ownership as an explicit prefix for the modifier name. It makes it clear to see on first glance who owns which modifier. That is a lot more intuitive than hunting it down in the ownership Inspector and then having to remember it. Even having a context based operator to check the ownership wouldn't be as convenient and immediate as that.
Additionally by engraving the prefix in the name, we avoid potential name collisions between modifiers of different task layers. This is the same issue we might have in other places as well (e.g. generic attributes), but for modifiers it is a lot more common to leave them at a default name after adding for example a
Subdivisionmodifier. So name collisions can happen quite easily.The existence of the update ownership button is a bit unclear to me. Shouldn't the ownership just be updated either explicitly by the user, or automatically whenever there is an exchange between task layer and publish data? So I don't see why this button would be necessary. I guess it could make sense to have a
Validate Ownershipbutton that can expose conflicts which can then be resolved manually.It bothers me a bit that the push and pull buttons are always greyed out unless you just saved the file. You end up just always saving before pressing the button. Which is the intended behaviour, but it would make more sense to me on a UX level to just keep the buttons available and give the user a dialog to save.
I already do get the strong feeling that we should not deviate from the idea of datablock level ownership as we had planned it initially. Having the ownership information of attributes, vertex groups and such defined on the object level rather than the mesh data-block feels quite wrong to me. I also think that this is a quite elegant way of solving the pending issue with de-duplicating shared data-blocks with users from different task layers. I don't think you explained to me why you opted to go for that approach instead, would be nice to go over that in detail.
The message
No new transfer data foundon pull seems a bit misleading/not very intuitive imo. I would specifylocal.I like how on push you show what new data will be pushed to the publish. Would be nice to do the symmetrical thing of what new data has been pulled.
And a few questions:
Hey @SimonThommes,
Thank you for your detailed feedback, I have gone ahead and updated the TODO with new tasks based on your feedback, for the simpler suggestions.
No new transfer data foundtoNo new local transfer data foundFor things that require further discussion I have posted responses to each below
Ownership
The Update Ownership button is mainly used for testing and we will likely remove it for the final version, it is just so that a user can see what new things they own without pushing/pulling. This ownership update process also occurs automatically before Push/Pull so this button is redundant.
The idea to have a
Validate Ownershipbutton is a good idea, there is some complexity, as we need to append in the external files to read it's ownership, but it is a good feature to consider, I have added it to the TODOs to consider.Saving
The reason we grey out the operators right now is a suggestion by met. We want the user to manually save because we need them to save their images as well as the .blend file. We are trying to take advantage of this dialogue
From Met:
"When texture painting, it needs to be saved explicitly before opening a new file, or it's gonna get lost. I would say don't implicitly save it on Push or Pull, but interrupt and
return {'CANCELLED'}and force user to manually save their shit."ID Level Transfer Data
To answer what I assume is the most pressing question in regards to why we didn't go for an ID-Level ownership for everything I decided to go that way mainly for simplicity for both the UI and the transfer process it self, it was a lot easier to implement, showing the user ever ID they created may be a bit confusing/overwhelming and there are more a lot more edge cases to consider if we go down that route, my main concern is that I am trying to make sure we are on time for the beginning of the next production with this add-on but I am willing to consider reverting back to the original design if met agrees with your concerns, for me it's mainly a time based thing. We can discuss this further on a call.
Show New Transfer Data on Pull
This is just a technical problem, related to the above
validate ownershipsuggestion we would need to append in the asset to read this data before transferring which may be a lot of data to load slowing down the UI. I am willing to consider the feature, just not decided on how to achieve it yet. I have added it to the TODOs to consider.Questions
Just to make sure that's clear. We have to pick and choose a bit here. For attributes, vertex groups and such we can't do this unfortunately, since the names actually matter (outside of API). Modifiers and Constraints are fine though
That could be nice as well, I was mainly thinking about just a version that shows you what has been added after the fact.
I'm not suggesting to just silently implicitly save, I think it's good to keep that explicit. But if the buttons are basically always greyed out it kind of defeats their purpose. Is there no way to invoke that dialogue with python? That would be preferable imo.
Without thinking it through in detail, I have the suspicion that datablock level ownership would also mean we can define the linking of objects to a collection as owned by the collection's owner. If we are fully generic on any level that should fit in nicely and give all the necessary flexibility if I'm not mistaken.
Maybe I'm misunderstanding something, but When an object is created by modeling, I'd personally find the need to explicitly claim ownership over material assignment, UV maps and such in the shading task layer quite tedious. Especially considering that I can't simply claim it, but it needs to be given by modeling in that case. (iirc)
Glad we were able to settle most of the above discussion on a call @SimonThommes, looking forward to our next call with @Mets and I will continue to update the TODO section with the changes we discuss.
Below I have a quick demo video to document where the add-on currently stands.
Asset Pipeline Demo
I watched your demo and have some additional feedback. Beyond the things that we've already been discussing this mainly just brings up questions about the asset initialization process.
Some of these things are mainly low priority design topics, as they are not a hard requirement to work with the addon.
Some additional topics I had to think of:
Glad we covered these topics in a call, I have updated the TODO with all the points we discussed, I will tag you when the next demo is ready!
Couple of solution proposals.
scene.task_layer_nameis changed to a CollectionProperty that gets populated by the Initialize operator. There's an entry for all task layers that are in the current config, and each entry has a boolean for enabled state. (Allow re-ordering this list!)Vertex Groups->Rigging,UV Maps->Shading.When adding new data:
At first I thought of a hard-coded task layer type which is "All", but that runs into issues with removal being difficult.
Let's indeed go with a boolean like you guys were saying. I think it works fine. Each ownership info has this
is_surrenderedbool. Then we can differentiate data that is undergoing an ownership change from other data, which should be all we need to enable an ownership change workflow.So, it could go something like this:
is_surrenderedflag of Subsurf modifiers toTrueand push.is_surrenderedflag with theupdate()callback. Then push.is_surrendered==True, so we can resolve the conflict, and that one loses. Let's call this a "soft-conflict".Other cases:
Stupid case:
So in my head this is a very small amount of code; The flag bool, some UI for that flag, an update() callback on the ownership enum/string, and an if statement somewhere in the source/target mapping, or wherever conflicts are currently checked for.
@Mets just to clarify: The solution for the ownership surrendering that you're proposing is the fancier method, which we put on low prio, right?
(also, shouldn't that flag be invisible to the user? It can bet set and cleared automatically as part of the merge process. If everything works the user shouldn't have to set it. Just change the ownership of some data you own to something else and the rest should happen automatically, right?)
Hey guys so here is a new demo here is what's changed... @SimonThommes @Mets
Creating Files
Collections now can be assigned to a task layer by user (Top Level children of asset collection only)
Custom Task Layers
Ownership Defaults
Multiple Task Layers Per File
Note: I still haven't made material ownership a property that exists even if there are no slots, that is being added as a seperate TODO. Please let me know ur feedback on the current state of things, might be good to have a debrief call to get feedback on these existing changes so I can note down my next steps.
Asset Pipeline Demo
I think maybe during the meeting we thought we needed a fancier solution, but I'd say this ends up being quite non-fancy, while also covering all we need, so I'd say we can nuke the TODO entry from the low prio set, and keep the other one? Having two feels redundant now that I have a clear idea of how it could potentially work.
In my proposal you can't change data's ownership to a task layer that you don't own, you can only put it "up for grabs". I think this results in fewer potentially confusing cases.
For example, if you could set the ownership of some data to another task layer which you don't own, you can also undo that, which makes it appear like you can take ownership of some data you don't own. But that won't work on other data, it only works on this because of a hidden flag that got automagically set previously. And if that happened a long time ago, it won't be clear in the UI why one piece of data is behaving differently than another.
Looks great!
I guess for assigning attribute defaults, the name must match exactly atm, so something like "UVMap.001" won't work anyways... But solving that would probably be quite a hassle. I think it would involve breaking up attributes to separate data types for UVMaps, Vertex Colors, etc, and everything else. Maybe something for the far future / low prio list.
We are just using the names of the attributes for the special attribute ownership defined in the JSON file.
So in theory you can mess this up if you create a new object, then rename the UVMap before updating the ownership it won't be assigned to 'Shading' just whatever the general attribute default is, which is "Rigging" in the default character JSON.
Hey guys another update here, so I will need feedback on the previous update plus this one in our next feedback session.
I have set material ownership to work how we planned so it now always gets assigned to the default task layer even if there are no materials yet assigned.
I'll be honest, I don't love this behaviour I think it makes this a little confusing and it's just another thing to explain, but im happy to do it as you both agree this is something you want.
Material Ownership Demo
So, IIRC, the initial problem we're trying to solve is that "Material Data", unlike other transferable data, includes not only list elements, which don't always exist, but also properties, which do always exist, and therefore would need to be owned by somebody. The same could be said for "Parenting Data", since the parent pointer and parent type are also properties.
So, the problem boils down to "when do we define ownership data when that ownership data includes properties".
I agree the current solution can be confusing and clutters the UI, but I would be fine with it if it could be implemented it in such a way that it's configurable at a production level, rather than hard baked into the add-on.
Alternatively or on top of that, we could also implement a lazy-initialization pattern for such ownership data, where we check whether the relevant properties are on their default values, and only create ownership data for them when that is not the case.
So for example, if I change the Auto-Smooth settings, or the Object Color, then ownership data for Materials gets created even if there are no materials yet.
The default value of an RNA property can be accessed like this btw:
bpy.types.Mesh.bl_rna.properties['use_auto_smooth'].defaultSome feedback on the current version according to what we talked about:
UI:
.->-)Other:
get_name_with_task_layer_prefix()2935ed5aecOk here is another update, I didn't tackle the ownership surrendering, but I will be starting on that tomorrow.
One important issue @Mets that we need to handle is a bug in 4.0 where Vertex Group transfer fails, this only happens on push when we change the context to another file. May need your help debugging this. I think we should resolve that before we move onto non-topo matching transfer.
The demo mainly covers things that I discussed with @SimonThommes as new features and fixes to little paper cuts.
Various Updates Demo
The Changes are as follows...
Awesome! These changes add a lot in terms of UI/UX!
Since the name prefixes are only a visual thing and not actually used for the merge process, shouldn't this also just happen on push/pull (whenever ownership is being validated/updated). I don't really see myself pressing this button ever, if it isn't required for the merge process to work properly.
Everything else I have no comments on, looking good 👍
naming.py5b6717227ftask_layer_prefix_name_get()5550ac9780Thank you for the feedback @SimonThommes I have added a TODO to update Prefixes during the merge process.
See the task under Medium priority
Update Prefix during Merge ProcessIn this update I have for @Mets mainly, I have completed the work for the surrender ownership button, as a proper feature considered in the merge process,
Surrendering Ownership Operator/UI. Below I have a demo of that feature...Surrender Ownership Demo
@Mets thank you for sending me that vertex transfer code from easy weights, I implemented it in this commit
019c610f78now we have vertex transfer working again, and it works for non -matching topology so thats two more TODOs cleared!Hey @Mets we have batch operations now, you can use it to surrender the ownership of objects or transfer data, I have added some filtering for this
Batch Operator Filters
You can batch update the ownership to both a local task layer and all task layers (use all option with care)
Batch Operator Demo
@SimonThommes I have added a commit
4a110ca68eso prefixes will now be updated before the merge process begins as requested in this comment studio/blender-studio-pipeline#145 (comment)Great work, mate. I think now we can start working on assets without stressing too much about what ownerships we assign when, since we'll be able to relatively easily change ownership of things later. Just some minor UI suggestions:
Surrender:
Batch Op:
@Mets I have updated the batch operator UI per your notes here and on the call we had
TRANSFER_DATA_TYPES415d9e3993is_requiredwhen sorting atributes e5aebf294doverride_obj_visability()2b949d51e3draw_task_layer_selection()11a23c7e64get_name_with_asset_prefix()toasset_prefix_name_get()a743446945My eyeballs flow peacefully from top to bottom. <3
@Mets As we discussed I have updated the
Create New AssetOperator to optionally re-use the current directory/file and keep the assets from that file or create a new directory with a blank file.Also I found a bug that is now possible based on some things we changes..
The Change was: We only remove orphan objects under the top level collection
The Bug: Now (this is case for Mikassa) if you parent object is outside the asset (which use to be invalid), the parent relationship will break down during sync, we need to consider this in the parent relationship trasnfer data property, and return an error if the parent object is outside the asset col or go back to all objects needing to be part of the asset col.
_gen_transfer_data_map()d791d920d4Hey @Mets @SimonThommes here is the new UI we discussed for the surrendered data.
The heart icon now indicated if the data is surrendered or not. We also have the claim button, renamed from update surrendered.
@Mets @SimonThommes to recap our last discussion, in addition to the UI updates I completed above we also discussed removing the
Default Ownershipfeature and replacing it with the ability toauto surrendersome transfer data items when they are created (configurable via JSON). I have added that functionality, demo is below.Auto Surrender Demo
WIP: Asset Pipeline 2to Asset Pipeline v2Closed by
1af2d41bdcTracking for incomplete TODOs at studio/blender-studio-pipeline#170
Pull request closed