FBX IO: Add attributes utility functions #104645

Thomas Barlow · 2023-05-29T05:01:13+02:00

Thomas Barlow commented

2023-05-29 05:01:13 +02:00

These are used by subsequent patches to update mesh data access to use
attributes.

The ability to specify the foreach_get/set Python attribute in attribute
utility functions is not currently used, but is to facilitate color
attributes which might want to by accessed with "color_srgb" instead of
the default of "color".

These are used by subsequent patches to update mesh data access to use attributes. The ability to specify the foreach_get/set Python attribute in attribute utility functions is not currently used, but is to facilitate color attributes which might want to by accessed with "color_srgb" instead of the default of "color".

Thomas Barlow referenced this pull request

2023-05-29 05:41:54 +02:00

FBX IO: Material index access with attributes #104646

Thomas Barlow referenced this pull request

2023-05-29 05:42:10 +02:00

FBX IO: Vertex position access with attributes #104647

Thomas Barlow referenced this pull request

2023-05-29 05:42:23 +02:00

FBX IO: Corner vert/edge and edge verts access with attributes #104648

Thomas Barlow referenced this pull request

2023-05-29 05:42:33 +02:00

FBX IO: Edge/face sharp access with attributes #104649

Thomas Barlow requested review from Bastien Montagne 2023-05-29 05:44:45 +02:00

Bastien Montagne reviewed 2023-06-09 12:43:24 +02:00

Bastien Montagne left a comment

Besides note below, this (and following dependent patches) LGTM.

io_scene_fbx/fbx_utils.py Outdated

						
				@ -543,0 +558,4 @@

				}

				def attribute_get(attributes, name, data_type, domain):

Bastien Montagne commented

2023-06-09 12:06:06 +02:00

I wonder if it would not be simpler if attributes was a dict with name, type, domain as keys?

I wonder if it would not be simpler if `attributes` was a dict with `name, type, domain` as keys?

Thomas Barlow commented

2023-06-21 19:27:11 +02:00

For only the purpose of getting existing attributes I think it would work well, but sometimes the attributes need to be created if they don't exist at which point the attributes (AttributeGroup) is needed. I think that to support both getting and creating like this would either mean passing both attributes and a new attributes_dict to most of the attributes helper functions or creating a dict from attributes each time attribute_get is called. I'm not a fan of either option, but I can make changes if you want.

There has been some recent discussion between Hans/HooglyBoogly and ideasman42 in #blender-coders on blender.chat about a similar function that would be useful for script authors such as

@\HooglyBoogly attr = data.attributes.ensure(name, data_type, domain) <- anything against this?
I expect it would be useful for script authors in general.

or

mesh.attributes.ensure(name, data_type, domain, replace=True, coerce=True) where replace removes any existing layers (when the type/domain doesn't match) and coerce converts between types where possible.

so I'm hopeful that this area of the Python API will see some improvements at some point and then the new function(s) can replace some of the helper functions added by this patch.

We could push for more discussion/development on this API feature. The main requirements from my perspective that might be missing from what has been discussed is having an equivalent 'get' function when there's no need to create the attribute if it doesn't exist, and ensuring that whatever functions we do get in the Python API have well-defined behaviour when there are name collisions in the attributes.

There are already some helper functions in bpy_types that the Mesh.edge_creases and Mesh.vertex_creases properties use for getting/ensuring/removing attributes, though they are currently private:

		scripts/modules/bpy_types.py
		Line 536 in b779c6ebe1
	
				def _name_convention_attribute_get(attributes, name, domain, data_type):

For only the purpose of getting existing attributes I think it would work well, but sometimes the attributes need to be created if they don't exist at which point the `attributes` (`AttributeGroup`) is needed. I think that to support both getting and creating like this would either mean passing both `attributes` and a new `attributes_dict` to most of the attributes helper functions or creating a dict from `attributes` each time `attribute_get` is called. I'm not a fan of either option, but I can make changes if you want. --- There has been some recent discussion between Hans/HooglyBoogly and ideasman42 in #blender-coders on blender.chat about a similar function that would be useful for script authors such as > @\HooglyBoogly `attr = data.attributes.ensure(name, data_type, domain)` <- anything against this? I expect it would be useful for script authors in general. or > `mesh.attributes.ensure(name, data_type, domain, replace=True, coerce=True)` where replace removes any existing layers (when the type/domain doesn't match) and coerce converts between types where possible. so I'm hopeful that this area of the Python API will see some improvements at some point and then the new function(s) can replace some of the helper functions added by this patch. We could push for more discussion/development on this API feature. The main requirements from my perspective that might be missing from what has been discussed is having an equivalent 'get' function when there's no need to create the attribute if it doesn't exist, and ensuring that whatever functions we do get in the Python API have well-defined behaviour when there are name collisions in the attributes. There are already some helper functions in `bpy_types` that the `Mesh.edge_creases` and `Mesh.vertex_creases` properties use for getting/ensuring/removing attributes, though they are currently private: https://projects.blender.org/blender/blender/src/commit/b779c6ebe15305204867e2574586234b42e9673e/scripts/modules/bpy_types.py#L536

Bastien Montagne commented

2023-06-22 10:46:54 +02:00

Yeah I would have expected a thin class wrapper (maybe even a simple named tuple) around attributes, storing both the blender data itself and the mapping...

But indeed if better helpers are scheduled in Blender API itself to handle these tasks, then current code is fine by me for now.

Yeah I would have expected a thin class wrapper (maybe even a simple named tuple) around `attributes`, storing both the blender data itself and the mapping... But indeed if better helpers are scheduled in Blender API itself to handle these tasks, then current code is fine by me for now.