studio/blender-studio-tools
13
32
Fork 19
Code Issues 21 Pull Requests 4 Projects Releases 1 Activity

Asset Pipeline: Add Custom Properties #291

Merged
Nick Alberelli merged 10 commits from TinyNick/blender-studio-pipeline:feature/asset-pipeline-add-custom-properties into main 2024-05-06 17:08:47 +02:00
Conversation 13 Commits 10 Files Changed 9 +122 -12
Nick Alberelli commented 2024-05-03 19:17:50 +02:00
Member
Copy Link

What's Changed?

  • Adds custom properties to Asset Pipeline Add-On
  • Only User custom properties are transferred (skips props defined by API from any add-on)
  • Adds CUSTOM_PROP key to task layer files BREAKING CHANGE
## What's Changed? - Adds custom properties to Asset Pipeline Add-On - Only User custom properties are transferred (skips props defined by API from any add-on) - Adds `CUSTOM_PROP` key to task layer files **BREAKING CHANGE**
Nick Alberelli added 7 commits 2024-05-03 19:17:56 +02:00
Nick Alberelli added 1 commit 2024-05-03 19:18:56 +02:00
Nick Alberelli added 2 commits 2024-05-03 20:03:59 +02:00
Nick Alberelli commented 2024-05-03 20:14:27 +02:00
Author
Member
Copy Link

@Mets @SimonThommes

We do have an issue with this new feature.

So I was clever and the way the code works is that it skips any custom property that is API defined (and existing on every object). This avoids copying data that add-ons store on the object level. The goal is to limit this transfer to only properties that the user has added explicitly.

I also have made the transfer code very generic thanks to good tools in the Python API for custom properties, so I can support all Custom Property types including dicts and pointer properties.

The issue is some add-ons like cloud rig make "non API Defined" properties. These are properties parked directly on the object as opposed to being registered to bpy.types.Object and will stick around even if the add-on that added them is not present. Which is good feature for rigging stuff, but we need to consider how this impacts our Asset Pipeline workflow (personally I think its fine but I wanted to double check).

Leaving this the way it is, means whomever generates that data via an add-on (for example rigging creates cloudrig properties) will own that custom prop, and any change made from the rigging file will be sent to the other files (assuming you haven't surrendered the data)

For an example in ballen_wrasse file the object RIG-ballen_wrasse has the following properties.

image

The software will treat the cloudrig properties the same as the user's "my_prop" property, because they are all not API defined.

Is this the behaviour we want?

@Mets @SimonThommes We do have an issue with this new feature. So I was clever and the way the code works is that it [skips any custom property that is API defined](https://projects.blender.org/studio/blender-studio-pipeline/pulls/291/files#:~:text=%23%20Skip%20if%20prop,continue) (and existing on every object). This avoids copying data that add-ons store on the object level. The goal is to limit this transfer to only properties that the user has added explicitly. I also have made the [transfer code](https://projects.blender.org/studio/blender-studio-pipeline/pulls/291/files#:~:text=prop%20%3D%20source_obj,(prop)) very generic thanks to good tools in the Python API for custom properties, so I can support all Custom Property types including dicts and pointer properties. The issue is some add-ons like cloud rig make "non API Defined" properties. These are properties parked directly on the object as opposed to being registered to bpy.types.Object and will stick around even if the add-on that added them is not present. Which is good feature for rigging stuff, but we need to consider how this impacts our Asset Pipeline workflow (personally I think its fine but I wanted to double check). Leaving this the way it is, means whomever generates that data via an add-on (for example rigging creates cloudrig properties) will own that custom prop, and any change made from the rigging file will be sent to the other files (assuming you haven't surrendered the data) For an example in `ballen_wrasse` file the object `RIG-ballen_wrasse` has the following properties. ![image](/attachments/a3a17786-938f-4462-a908-577be9214ffc) The software will treat the cloudrig properties the same as the user's "my_prop" property, because they are all not API defined. **Is this the behaviour we want?**
image.png
23 KiB
Demeter Dzadik commented 2024-05-03 20:37:00 +02:00
Member
Copy Link

Note that any disabled/non-present add-on's properties stick around, and will no longer be flagged as API-defined, so it's not a CloudRig thing. Otherwise you'd lose all your Cycles settings when you disable and re-enable Cycles, for example. Add-ons come and go, but their data stays. (Except for preferences, which is dumb.)

But regardless, you probably don't want to transfer custom properties of type idprop.types.IDPropertyGroup, so you can check against that and skip those, I think. Although un-registered CollectionProperties just become plain lists, which is a bit awkward. But, the entries of that list are once again idprop.types.IDPropertyGroup.

So I imagine it like this:

for key in obj.keys():
    value = obj[key]
    if (type(value) == idprop.types.IDPropertyGroup) or # Catch PropertyGroups.
       (type(value) == list and value!=[] and type(value[0]) == idprop.types.IDPropertyGroup): # Catch CollectionProperties of PropertyGroups
        # <transfer the prop>
Note that any disabled/non-present add-on's properties stick around, and will no longer be flagged as API-defined, so it's not a CloudRig thing. Otherwise you'd lose all your Cycles settings when you disable and re-enable Cycles, for example. Add-ons come and go, but their data stays. (Except for preferences, which is dumb.) But regardless, you probably ~~don't~~ want to transfer custom properties of type `idprop.types.IDPropertyGroup`, ~~so you can check against that and skip those, I think~~. Although un-registered CollectionProperties just become plain lists, which is a bit awkward. But, the entries of that list are once again `idprop.types.IDPropertyGroup`. So I imagine it like this: ``` for key in obj.keys(): value = obj[key] if (type(value) == idprop.types.IDPropertyGroup) or # Catch PropertyGroups. (type(value) == list and value!=[] and type(value[0]) == idprop.types.IDPropertyGroup): # Catch CollectionProperties of PropertyGroups # <transfer the prop> ```
Nick Alberelli commented 2024-05-03 20:44:08 +02:00
Author
Member
Copy Link

@Mets thanks for the note, there is definetly a challenege in filtering the correct properties, I will try out your suggestion.

But just to follow up on my original question, assuming the data is left behind by another add-on, we don't want to transfer it right?

@Mets thanks for the note, there is definetly a challenege in filtering the correct properties, I will try out your suggestion. But just to follow up on my original question, assuming the data is left behind by another add-on, we don't want to transfer it right?
Demeter Dzadik commented 2024-05-03 21:12:47 +02:00
Member
Copy Link

Oh, right, I didn't think about that part. Crossed out some parts of my previous comment.

If I understand correctly, "skipping these properties" means "let these properties be owned by the object owner". And that's how all properties are right now.

If I want to conjure up a use case where a specific task layer might want to own specific custom properties, but NOT the object itself, the main one that comes to mind is actually an add-on custom property, specifically obj['cycles']. An object could be owned by modeling, while Cycles settings can be owned by shading. So yeah, I guess if we want to have custom property ownership as a feature, including add-on properties in that would be good.

But I have good news too. Transferring add-on PropertyGroups is easy peasy:
target_obj['cycles'] = source_obj['cycles'].to_dict() (You can test this with eg. Object->Motion Blur toggle. Need to enable Render->Motion Blur first.)
This works whether the add-on is registered or not.

I also realized CloudRig does actually create some semi-arbitrary custom props on generated rigs, most importantly a big ass dictionary that populates the rig UI. So you weren't wrong about that either, but only cause you got lucky!

Oh, right, I didn't think about that part. Crossed out some parts of my previous comment. If I understand correctly, "skipping these properties" means "let these properties be owned by the object owner". And that's how all properties are right now. If I want to conjure up a use case where a specific task layer might want to own specific custom properties, but NOT the object itself, the main one that comes to mind is actually an add-on custom property, specifically `obj['cycles']`. An object could be owned by modeling, while Cycles settings can be owned by shading. So yeah, I guess if we want to have custom property ownership as a feature, including add-on properties in that would be good. But I have good news too. Transferring add-on PropertyGroups is easy peasy: `target_obj['cycles'] = source_obj['cycles'].to_dict()` (You can test this with eg. Object->Motion Blur toggle. Need to enable Render->Motion Blur first.) This works whether the add-on is registered or not. ----------------------------------------- I also realized CloudRig does actually create some semi-arbitrary custom props on generated rigs, most importantly a big ass dictionary that populates the rig UI. So you weren't wrong about that either, but only cause you got lucky!
Nick Alberelli commented 2024-05-03 21:24:38 +02:00
Author
Member
Copy Link

But I have good news too. Transferring add-on PropertyGroups is easy peasy:

I was copying properties like this, didn't realize I could do it with the dict, I wonder if there is a difference between doing it via dict or my method.

# Create New Prop
target_obj[prop_name] = source_obj[prop_name]

# Get ID Properties for New and Old Props
prop = source_obj.id_properties_ui(prop_name)
new_prop = target_obj.id_properties_ui(prop_name)

# Update New Prop wit Existing Prop Data
new_prop.update_from(prop)

So yeah, I guess if we want to have custom property ownership as a feature, including add-on properties in that would be good.

And that is for any property, even the API defined ones? Because lets say in your example the cycles, we want to replace the modeling cycles prop with the owner's (shadings) version of it? Correct?

> But I have good news too. Transferring add-on PropertyGroups is easy peasy: I was copying properties like this, didn't realize I could do it with the dict, I wonder if there is a difference between doing it via dict or my method. ```python # Create New Prop target_obj[prop_name] = source_obj[prop_name] # Get ID Properties for New and Old Props prop = source_obj.id_properties_ui(prop_name) new_prop = target_obj.id_properties_ui(prop_name) # Update New Prop wit Existing Prop Data new_prop.update_from(prop) ``` > So yeah, I guess if we want to have custom property ownership as a feature, including add-on properties in that would be good. And that is for any property, even the API defined ones? Because lets say in your example the cycles, we want to replace the modeling `cycles` prop with the owner's (shadings) version of it? Correct?
Demeter Dzadik commented 2024-05-03 21:55:28 +02:00
Member
Copy Link

even the API defined ones?

Yeah!

> even the API defined ones? Yeah!
Nick Alberelli commented 2024-05-03 22:02:10 +02:00
Author
Member
Copy Link

even the API defined ones?

Yeah!

We can do that, it's just that you will have even more ownership data (slowing the transfer process) for each object. I thought this feature was about just transferring the custom properties that users add.

I do think copying API defined properites can be dangerous, as we could also be moving add-on data around that the add-ons dont expect.

Personally I think that cycles render settings should be defined in the animation files not the asset files, but if @SimonThommes agrees we can implement the feature as you described.

> > even the API defined ones? > > Yeah! We can do that, it's just that you will have even more ownership data (slowing the transfer process) for each object. I thought this feature was about just transferring the custom properties that users add. I do think copying API defined properites can be dangerous, as we could also be moving add-on data around that the add-ons dont expect. Personally I think that cycles render settings should be defined in the animation files not the asset files, but if @SimonThommes agrees we can implement the feature as you described.
Demeter Dzadik commented 2024-05-06 14:44:07 +02:00
Member
Copy Link

cycles render settings should be defined in the animation files not the asset files

We can always override stuff ofc, but they are inherently stored on the asset by Cycles, it's not something we control our end. But I don't know how much we even use these settings, maybe not at all. Was just a hypothetical example, after all.

You're right in that currently, we just want to add primitive custom props on objects, like int/float/bool/color. If we want to only support that, for now, to keep things simple and efficient, that's fine by me.

> cycles render settings should be defined in the animation files not the asset files We can always override stuff ofc, but they are inherently stored on the asset by Cycles, it's not something we control our end. But I don't know how much we even use these settings, maybe not at all. Was just a hypothetical example, after all. You're right in that currently, we just want to add primitive custom props on objects, like int/float/bool/color. If we want to only support that, for now, to keep things simple and efficient, that's fine by me.
Simon Thommes commented 2024-05-06 16:01:37 +02:00
Member
Copy Link

I didn't read through the entire conversation about the addon created properties, but imo they should ideally be treated just like any other. Whether the user creates a property explicitly or implicitly shouldn't matter imo. There are several other examples with Tools in Blender creating data implicitly that needs to be transferred to keep the setup functional.

Since the ownership this way is made explicit, I think this shouldn't create any issues in general. The only potential problem I could see is with pointer properties, but I kind of hope the user remapping we have in place would already take care of that (?)

I didn't read through the entire conversation about the addon created properties, but imo they should ideally be treated just like any other. Whether the user creates a property explicitly or implicitly shouldn't matter imo. There are several other examples with Tools in Blender creating data implicitly that needs to be transferred to keep the setup functional. Since the ownership this way is made explicit, I think this shouldn't create any issues in general. The only potential problem I could see is with pointer properties, but I kind of hope the user remapping we have in place would already take care of that (?)
Nick Alberelli commented 2024-05-06 16:09:13 +02:00
Author
Member
Copy Link

ok so just to confirm @SimonThommes, when I started this projectI suggested that we only focus on properties that the user explicitly adds from the UI. But now we also want to handle any custom property that could exist on an object even if it was added by an add-on, which will include property groups.

Correct?

I can look into the pointers and if they are even necessary if this is the case.

ok so just to confirm @SimonThommes, when I started this projectI suggested that we only focus on properties that the user explicitly adds from the UI. But now we also want to handle any custom property that could exist on an object even if it was added by an add-on, which will include property groups. Correct? I can look into the pointers and if they are even necessary if this is the case.
Simon Thommes commented 2024-05-06 16:56:31 +02:00
Member
Copy Link

I'm not sure what this means about the scope of the task. Ideally yes, I think it would be best if all custom properties could be treated the same.

But I do think the way it works right now is a totally valid first step to get there, so I'm perfectly fine merging this PR as for what it is and then investigating the more general case including API defined data separately. But if that turns out to be more of a hassle I'd also be fine with dropping that.

If I want to conjure up a use case where a specific task layer might want to own specific custom properties, but NOT the object itself

@Mets to fill in the context of why I asked for this feature: I need to add custom props to all sorts of objects all the time for shading purposes. This is the last main thing that requires me to still touch other task layer's working files.

Is that fine with both of you @Mets @TinyNick ?

I'm not sure what this means about the scope of the task. Ideally yes, I think it would be best if all custom properties could be treated the same. But I do think the way it works right now is a totally valid first step to get there, so I'm perfectly fine merging this PR as for what it is and then investigating the more general case including API defined data separately. But if that turns out to be more of a hassle I'd also be fine with dropping that. > If I want to conjure up a use case where a specific task layer might want to own specific custom properties, but NOT the object itself @Mets to fill in the context of why I asked for this feature: I need to add custom props to all sorts of objects all the time for shading purposes. This is the last main thing that requires me to still touch other task layer's working files. Is that fine with both of you @Mets @TinyNick ?
👍 1
Demeter Dzadik commented 2024-05-06 17:08:11 +02:00
Member
Copy Link

Yeah, I agree, we can merge this as a start, and then if we want to add support for API defined properties, that can come separately.

Yeah, I agree, we can merge this as a start, and then if we want to add support for API defined properties, that can come separately.
👍 1
Nick Alberelli merged commit 323dd8ebe0 into main 2024-05-06 17:08:47 +02:00
Nick Alberelli deleted branch feature/asset-pipeline-add-custom-properties 2024-05-06 17:08:47 +02:00
Nick Alberelli commented 2024-05-06 17:08:53 +02:00
Author
Member
Copy Link

Ok merged

Ok merged
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Kind
Breaking
Breaking change that won't be backward compatible

  
Kind
Bug
Something is not working

  
Kind: Community

Issues opened by members of the community / outside the studio

  
Kind
Documentation
Documentation changes

  
Kind
Easy
Simple to complete

  
Kind
Enhancement
Improve existing functionality

  
Kind
Feature
New functionality

  
Kind
Proposal
A proposal or idea that requires feedback

  
Kind
Security
This is security issue

  
Kind
Studio Request
Requested by a user at the Blender-Studio

  
Kind
Testing
Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: studio/blender-studio-tools#291
No description provided.
Delete Branch "TinyNick/blender-studio-pipeline:feature/asset-pipeline-add-custom-properties"

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?