Dependent/Linked CustomData layers #97174

New Issue

Martijn Versteegh · 2022-04-08T16:32:47+02:00

Martijn Versteegh commented

2022-04-08 16:32:47 +02:00

When moving from an AoS to an SoA paradigm and mving compound structs to separate CD_PROP_* generic attribute layers, there needs to be a system to tie the various layers that used to be combined into a single struct together.

I'll describe the problem with the CD_MLOOPUV datalayer as an example, but I think it would be nice to solve this in a generic way because other parts of blender will likely have the same need to store 'structs' of related information in attribute layers.

First I'll desribe the original situation:

 The CD_MLOOPUV datalayer  consists of an array of MLoopUV structs. The MLoopUV struct contains 4 parts of information. a float[2] vector and 3 booleans. 
 There can be multiple UV layers, distinguished by their name.

It is easy to access all the information elementwise. Accessing a single element gives you all information in an MLoopUV struct.

Now the new situation:

 The complete uv datalayer consists of 4 separate generic attribute layers: One CD_PROP_FLOAT2 layer for the actual uv imformation and 3 CD_PROP_BOOL layers containing the 3 bool flags.
 Now there is a naming problem. Those 4 layers can't have the same name, we need a way to get at them separately. Based on a sing;e 'UV layer name" and some sort of 'subname'. For example if we need to get at the vertsel or edgesel bools for a specific facecorner we need to get at those bool layers based on the name of the uvmap (or the active uv map) and an extra specifier.

Possible solutions (ideas welcome):
A) Just tag on the extra specifier to the name, using some sort of reserved coupling character.

Advantage: no changes to the datastructures
Disadvantage: needs extra care on renaming and deleting layers.
Disadvantage: the current maximum layername length is 64 chars. This is already quite a restriction (I've seen requests to make it larger for workflows with standardized layer names) tagging on extra stuff in the same space takes even more space away there.
B) Add an extra list of dependent layers to the CustomDataLayer struct .
Advantage: No extra care needed on renaming. Still on deleting (also delete the depndent layers?)
Possible disadvantage: leads to a hierarchical structure, you need 1 main layer and various sub layers. (I don't think this really matters much, could even be an advantage)
Disadvantage: Needs changes to the CustomDataLayer struct. More complicated on saving.
Disadvantage layers can't have the same name, so sublayers still need some way of giving them a distinct name.

C) Add an extra sub-name field in the CustomDataLayer struct.

Advantage: simpler than B, doesn't take away space in the main layer name. Very similar to A
Disadvantage: need to save an extra name compared to A)

The prototype in D14365 more or less does A , but it doesn't handle renaming/deleteing.

When moving from an AoS to an SoA paradigm and mving compound structs to separate CD_PROP_* generic attribute layers, there needs to be a system to tie the various layers that used to be combined into a single struct together. I'll describe the problem with the CD_MLOOPUV datalayer as an example, but I think it would be nice to solve this in a generic way because other parts of blender will likely have the same need to store 'structs' of related information in attribute layers. First I'll desribe the original situation: ``` The CD_MLOOPUV datalayer consists of an array of MLoopUV structs. The MLoopUV struct contains 4 parts of information. a float[2] vector and 3 booleans. There can be multiple UV layers, distinguished by their name. ``` It is easy to access all the information elementwise. Accessing a single element gives you all information in an MLoopUV struct. Now the new situation: ``` The complete uv datalayer consists of 4 separate generic attribute layers: One CD_PROP_FLOAT2 layer for the actual uv imformation and 3 CD_PROP_BOOL layers containing the 3 bool flags. Now there is a naming problem. Those 4 layers can't have the same name, we need a way to get at them separately. Based on a sing;e 'UV layer name" and some sort of 'subname'. For example if we need to get at the vertsel or edgesel bools for a specific facecorner we need to get at those bool layers based on the name of the uvmap (or the active uv map) and an extra specifier. ``` Possible solutions (ideas welcome): A) Just tag on the extra specifier to the name, using some sort of reserved coupling character. - Advantage: no changes to the datastructures - Disadvantage: needs extra care on renaming and deleting layers. - Disadvantage: the current maximum layername length is 64 chars. This is already quite a restriction (I've seen requests to make it larger for workflows with standardized layer names) tagging on extra stuff in the same space takes even more space away there. B) Add an extra list of dependent layers to the CustomDataLayer struct . - Advantage: No extra care needed on renaming. Still on deleting (also delete the depndent layers?) - Possible disadvantage: leads to a hierarchical structure, you need 1 main layer and various sub layers. (I don't think this really matters much, could even be an advantage) - Disadvantage: Needs changes to the CustomDataLayer struct. More complicated on saving. - Disadvantage layers can't have the same name, so sublayers still need some way of giving them a distinct name. C) Add an extra sub-name field in the CustomDataLayer struct. - Advantage: simpler than B, doesn't take away space in the main layer name. Very similar to A - Disadvantage: need to save an extra name compared to A) The prototype in [D14365](https://archive.blender.org/developer/D14365) more or less does A , but it doesn't handle renaming/deleteing.

Martijn Versteegh commented

2022-04-08 16:32:47 +02:00

Added subscribers: @Baardaap, @HooglyBoogly

Hans Goudey commented

2022-04-11 18:06:47 +02:00

I think some solution that maintains CustomData's flat array of layers would be preferable. Any sort of hierarchy is likely more complex and more costly to traverse, which I think is the case to optimize for rather than removal.

For the selection and pinning layers, my hunch is that some other sort of attribute identifier similar to AnonymousAttributeID would be helpful, since I see them more as UI data. On the other hand, I'm not sure we've decided explicitly that such UI data shouldn't show up somehow in the attributes list.
A seam layer seems more useful to control procedurally.

Here's another solution:

D) Add an array of "child/dependent attributes" to the parent custom data layer

Advantage: Users could edit the list, and it could potentially be used more generically in the future.
Advantage: Relatively simple to view the sub-attributes, while maintaining the flat list
Possible disadvantage: Still requires the "sub-attributes" to have unique names/identifiers
Disadvantage: Storing attribute names more than once unnecessarily

Maybe some combination of these ideas makes sense, we'll see.

I think some solution that maintains `CustomData`'s flat array of layers would be preferable. Any sort of hierarchy is likely more complex and more costly to traverse, which I think is the case to optimize for rather than removal. For the selection and pinning layers, my hunch is that some other sort of attribute identifier similar to `AnonymousAttributeID` would be helpful, since I see them more as UI data. On the other hand, I'm not sure we've decided explicitly that such UI data shouldn't show up somehow in the attributes list. A seam layer seems more useful to control procedurally. Here's another solution: D) Add an array of "child/dependent attributes" to the parent custom data layer * Advantage: Users could edit the list, and it could potentially be used more generically in the future. * Advantage: Relatively simple to view the sub-attributes, while maintaining the flat list * Possible disadvantage: Still requires the "sub-attributes" to have unique names/identifiers * Disadvantage: Storing attribute names more than once unnecessarily Maybe some combination of these ideas makes sense, we'll see.

Martijn Versteegh commented

2022-04-11 20:10:28 +02:00

To be clear: your D) is what I meant wit my B).

I wasn't talking about actually nesting layers. Just linking them via a list of dependent layers. Which conceptually creates a hierarchy (but still is stored as a a flat list).

If we make them anonymous layers that would make it easier I guess, as then the naming problem doesn't really exist.

To be clear: your D) is what I meant wit my B). I wasn't talking about actually nesting layers. Just linking them via a list of dependent layers. Which conceptually creates a hierarchy (but still is stored as a a flat list). If we make them anonymous layers that would make it easier I guess, as then the naming problem doesn't really exist.

Martijn Versteegh commented

2022-04-15 14:43:46 +02:00

About: https://hackmd.io/@II9-Bkl4TJifCqGL2jgbUw/BJkCE9BVq

In general I like the proposal a lot. Keep it simple.

How do you propose to handle this:

there can be multiple UV layers
each UV layer needs it's own set of dependent layers (>select_vertex, >uv_pinned, >select_edge )

Just append the name of the original UV layer?

How hard would it be (backwards compatibility-wise) to increase the max size of the layers names? With these sort of longish prefixes the 64char limit can get rather tight I think.

About: https://hackmd.io/@II9-Bkl4TJifCqGL2jgbUw/BJkCE9BVq In general I like the proposal a lot. Keep it simple. How do you propose to handle this: - there can be multiple UV layers - each UV layer needs it's own set of dependent layers (>select_vertex, >uv_pinned, >select_edge ) Just append the name of the original UV layer? How hard would it be (backwards compatibility-wise) to increase the max size of the layers names? With these sort of longish prefixes the 64char limit can get rather tight I think.

Hans Goudey commented

2022-04-15 15:19:16 +02:00

Yeah, that's the suggestion for handling those layers.
We talked about changing the layer name to char * instead of an array. Then arbitrary lengths could be supported.
DNA could detect that change and apply it automatically when loading a file (as another eReconstructStepType possibly).

Yeah, that's the suggestion for handling those layers. We talked about changing the layer name to `char *` instead of an array. Then arbitrary lengths could be supported. DNA could detect that change and apply it automatically when loading a file (as another `eReconstructStepType` possibly).

Martijn Versteegh commented

2022-04-18 00:52:57 +02:00

There's one piece of the puzzle still missing from this proposal. (Though maybe that's out of the scope of this specific proposal?)

Optional attributes.

For some attributes it would be very nice if they could return a default value when reading from a layer that doesn't exist. And it would be even nicer if the layer would be created on demand when trying to write to certain non-existent layers.

This way we could prevent uv_vertsel,uv_pinned and uv_edgesel layers being created for each and every FLOAT2 layer even though they never will get used.

The first property (reading giving a default value when the layer doesn't exists) could maybe be handled with a specialized BM_CD_ELEM_GET_*_OPTIONAL() macro which would return a default value if the base offset is -1.

For the writing I don't really see how it could be done directly from the access macros. But that's not too bad because writing happens way less than reading. It would be nice if there were a set of CustomData_get_or_add_named_layer_* () functions which would create the layer if it doesn't yet exists or return it when it does.

There's one piece of the puzzle still missing from this proposal. (Though maybe that's out of the scope of this specific proposal?) **Optional attributes.** For some attributes it would be very nice if they could return a default value when reading from a layer that doesn't exist. And it would be even nicer if the layer would be created on demand when trying to write to certain non-existent layers. This way we could prevent uv_vertsel,uv_pinned and uv_edgesel layers being created for each and every FLOAT2 layer even though they never will get used. The first property (reading giving a default value when the layer doesn't exists) could maybe be handled with a specialized BM_CD_ELEM_GET_*_OPTIONAL() macro which would return a default value if the base offset is -1. For the writing I don't really see how it could be done directly from the access macros. But that's not too bad because writing happens way less than reading. It would be nice if there were a set of CustomData_get_or_add_named_layer_* () functions which would create the layer if it doesn't yet exists or return it when it does.

Hans Goudey commented

2022-04-18 19:51:43 +02:00

I think reading attributes with a default value should be handled by the reader of the attribute, so with a macro like you suggest for BMesh.
In the C++ attribute API we already support this, with an optional default value argument to functions like attribute_get_for_read.

Adding a CustomData function to add an attribute only if it doesn't exist sounds good.
Though for Mesh, I think we should move towards using the C++ attribute API rather than the CustomData API (that doesn't apply to BMesh).

I think reading attributes with a default value should be handled by the reader of the attribute, so with a macro like you suggest for BMesh. In the C++ attribute API we already support this, with an optional default value argument to functions like `attribute_get_for_read`. Adding a `CustomData` function to add an attribute only if it doesn't exist sounds good. Though for `Mesh`, I think we should move towards using the C++ attribute API rather than the `CustomData` API (that doesn't apply to BMesh).

Jacques Lucke commented

2022-04-19 16:12:51 +02:00

Added subscriber: @JacquesLucke