nutti / fake-bpy-module

Fake Blender Python API module collection for the code completion.
MIT License
1.38k stars 97 forks source link

Incorrect definitions for certain bpy.props constructor parameters. #121

Closed angavrilov closed 1 year ago

angavrilov commented 1 year ago

System Information

Description about the bug

Definitions for certain parameters in bpy.props are too restrictive.

For example:

Also:

All these issues were found while fixing Rigify source code to produce no warnings in PyCharm.

nutti commented 1 year ago

@angavrilov

The bpy.types.bpy_prop_array class is completely missing.

Is there any documents of this class? We could not understand the specification.

angavrilov commented 1 year ago

@nutti

It is an implementation detail, similar to bpy_prop_collection: any 'vector' properties not covered by mathutils types (Vector, Quaternion, Euler, Matrix) are actually using bpy_prop_array. I expect it should conform to the MutableSequence interface: dir() returns __bool__, __contains__, __delitem__, __doc__, __getattribute__, __getitem__, __iter__, __len__, __repr__, __setitem__, foreach_get, foreach_set (but some like __delitem__ look like they may be dummies that error out). The purpose of it is that it is still linked to the underlying C array, so you can do e.g. bone.layers[1] = True, and it will update the single entry.

However in practice my main complaint is that it doesn't exist at all in the module - Rigify imports it in one source file for an isinstance check, and the import is highlighted as an unknown identifier.

Btw:

angavrilov commented 1 year ago

Also:

nutti commented 1 year ago

@angavrilov

Thanks, but I need your help because handling these special cases increase the maintenance costs. I think these mismatch comes from the inconsistent description over the documents. So, we need to rethink the approach to fix your issue.

all vector properties should use typing.Sequence[T] | bpy_prop_array[T], not List

It seems that your suggestion is far from the official document. Handling these cases (like Vector) increases the maintenance cost. Do you have an idea to consistent between the official document and fake-bpy-module?

Various fields in ThemeView3D should be mathutils.Color instead of vector.

Is there any good solution to distinguish between Vector or Color from the official document? From the official document, it seems that there is no difference between Vector and Color.

angavrilov commented 1 year ago

It seems that your suggestion is far from the official document.

Well, the document won't describe the true type, because it is an implementation detail. However no wrapper for a Blender C structure (any subclass of bpy_struct) would ever have a true 'list' attribute because of purely technical reasons: it instead will accept assigning any sequence (or at least list and tuple), and return bpy_prop_array. Basically, it is intended for the field to appear to be a list, but for implementation reasons the actual type is different. I think this should be a global rule, similar to how any Vector function parameters should also accept Sequence[float] because of how C API functions parse parameters, or AnyType being replaced with typing.Any.

angavrilov commented 1 year ago

Reporting one more issue:

nutti commented 1 year ago

@angavrilov

I think these issues should be reported separately. We could not track your issues only this issue. The title and the issues itself do not match any more.

Btw, are you willing to contribute to this project? You can make a pull request if you fix the issue by yourself.

angavrilov commented 1 year ago

I could try if you point me where to look for fixing each of the kinds of issue here? (there are issues with specific classes and methods, and also generic issues like AnyType and Vector parameters, which are obviously very different)

nutti commented 1 year ago

@angavrilov

Thanks.

there are issues with specific classes and methods, and also generic issues like AnyType and Vector parameters, which are obviously very different

The reason I want to separate issue is this. We could not track each issue if we summary all issues in one issue.

Various fields in ThemeView3D should be mathutils.Color instead of vector.

If you try to tackle one of these issues, I want you to give me the idea how to distinguish between Vector and Color from the document.

Color example: https://docs.blender.org/api/current/bpy.types.ThemeView3D.html#bpy.types.ThemeView3D.act_spline Vector example (see parameter "location"): https://docs.blender.org/api/current/bpy.ops.object.html#bpy.ops.object.add

angavrilov commented 1 year ago

If you try to tackle one of these issues, I want you to give me the idea how to distinguish between Vector and Color from the document.

It seems the documentation is deficient here. You can however get it from blender itself:

>>> bpy.types.ThemeView3D.bl_rna.properties['wire'].subtype
'COLOR_GAMMA'

>>> bpy.types.Object.bl_rna.properties['color'].subtype
'COLOR'

>>> bpy.types.Object.bl_rna.properties['rotation_quaternion'].subtype
'QUATERNION'

>>> bpy.types.Object.bl_rna.properties['rotation_euler'].subtype
'EULER'

>>> bpy.types.Object.bl_rna.properties['rotation_axis_angle'].subtype
'AXISANGLE'

>>> bpy.types.Object.bl_rna.properties['location'].subtype
'TRANSLATION'

>>> bpy.types.Object.bl_rna.properties['scale'].subtype
'XYZ'
angavrilov commented 1 year ago

I think I split off all issues that need 'generic' fixes. Only random issues about specific methods or classes should remain here.

nutti commented 1 year ago

@angavrilov

Thanks for splitting the issues. It is much clearer than before.

nutti commented 1 year ago

@angavrilov

KDTree methods on the other hand require a tuple (and of one float instead of 3!), but should really be Vector | Sequence.

Should this fix the document by using Vector instead of float triplet? https://docs.blender.org/api/current/mathutils.kdtree.html

bpy.data.objects.new(name=name, object_data=None) is valid - it creates an Empty (i.e. object_data type should be ID | None)

Do you have an idea to find whether the argument accepts the None or not?

angavrilov commented 1 year ago

Should this fix the document by using Vector instead of float triplet? https://docs.blender.org/api/current/mathutils.kdtree.html

Probably. As I mentioned in the issue about vectors, the C api functions actually are very permissive and basically accept Sequence[float], and even Iterable[float] in some cases, whenever they want a vector parameter (with all mathutils types being a valid 'sequence' for this purpose). However Vector obviously represents the intent best (i.e.: "the methods want Vector, and will implicitly cast anything similar"), and the permissive part is subject of issue #124.

Do you have an idea to find whether the argument accepts the None or not?

Well, it's mentioned in the description part of the parameter documentation. The actual nullability condition may be another flag of the internal blender property metadata, like with the subtype field that determines the mathutils type usage.

angavrilov commented 1 year ago

@nutti Are you making a docs patch for KDTree? If not, maybe I should? The return types there could also use clarifying - there are mentioned in the human-readable description, but not in the actual type.

Btw, is there any reason why you are using typing.Union[typing.Tuple[...], ...] etc instead of more compact tuple[...] | ...?

nutti commented 1 year ago

C api functions actually are very permissive and basically accept Sequence[float], and even Iterable[float]

If this matches for all of APIs, it is easy to address from the fake-bpy-module side. But if there is an exception, changing the document seems better.

it's mentioned in the description part of the parameter documentation.

Actually, it needs an additional cost to parse the description part as you may think. If this is described in the "data type" part, we can handle with low cost. BTW, I can find the (never None) description in the "data type" part. https://docs.blender.org/api/current/bpy.types.BlendDataObjects.html#bpy.types.BlendDataObjects Is it useful for checking whether data type accepts None or not.

Are you making a docs patch for KDTree? If not, maybe I should?

I haven't tackled it. If possible, it is helpful for me to make a patch.

angavrilov commented 1 year ago

I haven't tackled it. If possible, it is helpful for me to make a patch.

I think it's probably better for you to do it, since you know what type definitions you can parse. Basically:

Generic aspects of the behavior of vector parameters is best discussed in #124

nutti commented 1 year ago

Matrix.Scale: the axis parameter should truly be optional (Scale(2.0, 4) is valid).

It looks strange to me. The axis parameter of Matrix.Scale has already marked as typing.Optional. Is there any issue about the generated module?

angavrilov commented 1 year ago

It looks strange to me. The axis parameter of Matrix.Scale has already marked as typing.Optional. Is there any issue about the generated module?

Marking the type Optional merely allows None, and does not actually make the parameter optional - you need a default value for that. 'Nullable' probably would have been a better name for Optional in this sense. In case of quad_method etc I think they aren't Optional, but should have a default value to be optional.

nutti commented 1 year ago

bmesh.ops.triangulate: the quad_method and ngon_method parameters are optional.

About this issue, it seems good from the generated module.

def triangulate(bm: 'bmesh.types.BMesh',
                faces: typing.List['bmesh.types.BMFace'] = [],
                quad_method: typing.Union[str, int] = 'BEAUTY',
                ngon_method: typing.Union[str, int] = 'BEAUTY') -> typing.Dict:

bmesh.ops.split: the dest parameter is optional, and the geom parameter is a list.

The former part seems ok from the generated module. Of course, we need to fix the latter part.

def split(bm: 'bmesh.types.BMesh',
          geom: 'bmesh.types.BMEdge' = [],
          dest: 'bmesh.types.BMesh' = None,
          use_only_faces: bool = False) -> typing.Dict:

Is my insight correct for above cases?

angavrilov commented 1 year ago

Since I've been manually fixing the files simultaneously to reporting these things, I'm still using (edited) 3.3-20221006 files, so it's feasible some reports may be obsolete. Maybe it's time to update and recheck, but I'm mostly holding out for #124. On the other hand, with the edited files, I achieved zero PyCharm warnings in Rigify source code, and it feels nice :).

Those default values are probably ok.

nutti commented 1 year ago

Thanks. I will mark these tasks as resolved for now. I also fixed the geom parameter of split function. Please let me know if there are still issues.

nutti commented 1 year ago

All issues are now fixed. Thanks for reporting them.