nutti / fake-bpy-module

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

All target object in constraints can be optional #243

Closed JonathanPlasse closed 4 weeks ago

JonathanPlasse commented 1 month ago

Expected behavior

bpy.types.CopyLocationConstraint.target can be None. In general, I think that all object type in constraints should be optional. We could refine the data type of all Object to Object | None in class with base class Constraint.

Description about the bug

Currently, all object types in constraints are not optional. bpy.types.CopyLocationConstraint.target has the type Object and not Object | None.

Related issue

animation_data can be None when there is no keyframe. animation_data should have the type AnimData | None not AnimData.

AnimData.action should have the type Action | None not Action.

nutti commented 1 month ago

Like this issue, we could not find the way to check if data type is optional or not. Is there any good solution? Using mod file is not good solution for this because the revising cost is high.

JonathanPlasse commented 1 month ago

Should the Blender documentation be updated with optionals?

nutti commented 1 month ago

Yes, I think that is a preferable solution.

Some documents explicitly show (optional) like this. https://docs.blender.org/api/current/bpy.ops.object.html#bpy.ops.object.add

radius (float in [0, inf], (optional)) – Radius

But, not all data type follows this rule.

Perhaps, we need to modify the rst file generation code. https://github.com/blender/blender/blob/main/doc/python_api/sphinx_doc_gen.py

nutti commented 1 month ago

For the function parameters, we can check (optional) or (never none) to find the optional argument. It may be difficult for the class attributes.

nutti commented 1 month ago

@JonathanPlasse

If changing rst generation code... If you can find the common law from the documents including class/attribute name, description, or data type string, it is possible without any mod file. Is there any idea about it?

nutti commented 1 month ago

With my quick investigation, it can be fixed from the generation code. If is_never_none is False, the property will be optional.

>>> attrs = getattr(bpy.types.CopyLocationConstraint.bl_rna, "properties")
>>> for a in attrs:
...     print(f"{a.identifier} : {a.is_never_none}")
...     
rna_type : False
name : True
type : False
is_override_data : False
owner_space : False
target_space : False
space_object : False
space_subtarget : True
mute : False
enabled : False
show_expanded : False
is_valid : False
active : False
influence : False
error_location : False
error_rotation : False
head_tail : False
use_bbone_shape : False
target : False                   <------ This is Optional.
subtarget : True
use_x : False
use_y : False
use_z : False
invert_x : False
invert_y : False
invert_z : False
use_offset : False
>>> attrs = getattr(bpy.types.AnimData.bl_rna, "properties")
>>> for a in attrs:
...     print(f"{a.identifier} : {a.is_never_none}")
...     
rna_type : False
nla_tracks : False
action : False                     <------ This is Optional.
action_extrapolation : False
action_blend_type : False
action_influence : False
action_tweak_storage : False
drivers : False
use_nla : False
use_tweak_mode : False
use_pin : False
action_binding_handle : False
action_binding_name : True
action_binding : False
>>> attrs = getattr(bpy.types.Armature.bl_rna, "properties")
>>> for a in attrs:
...     print(f"{a.identifier} : {a.is_never_none}")
...     
rna_type : False
name : True
name_full : True
id_type : False
session_uid : False
is_evaluated : False
original : False
users : False
use_fake_user : False
use_extra_user : False
is_embedded_data : False
is_missing : False
is_runtime_data : False
is_editable : False
tag : False
is_library_indirect : False
library : False
library_weak_reference : False
asset_data : False
override_library : False
preview : False
animation_data : False            <------ This is Optional.
...

I will check if this can be fixed by tweaking the rst file generation code.

JonathanPlasse commented 1 month ago

Awesome!

nutti commented 4 weeks ago

From the deep inspection, we don't need to modify the rst generation code. Instead, we modified the writer.

This issue is now fixed.

JonathanPlasse commented 4 weeks ago

This fix cause a lot of regressions on my code. Here is a non-exhaustive list of that became optional that I have never seen None.

nutti commented 4 weeks ago

We can revert this patch. Which is better for the use-case?

I think we could not distinguish the optional data type any more because it depends on the internal of Blender.

JonathanPlasse commented 4 weeks ago

We should revert, it is unusable as is with type checker like Pyright or MyPy.

nutti commented 4 weeks ago

OK, I will revert it. From the technical reason, I will close this issie as won'tfix for now. If there is a good solution, please let us know.

nutti commented 4 weeks ago

@JonathanPlasse BTW, is this kind of issue not many cases? If so, we can fix them by mod files with the low cost. We concern about the maintainance cost in future.

JonathanPlasse commented 4 weeks ago

OK, I will revert it. From the technical reason, I will close this issie as won'tfix for now. If there is a good solution, please let us know.

I understand

@JonathanPlasse BTW, is this kind of issue not many cases? If so, we can fix them by mod files with the low cost. We concern about the maintainance cost in future.

I take a deeper look on my codebase to list all detrimental optionals.

JonathanPlasse commented 4 weeks ago

The list is too long, nearly all attributes are set as optional.

https://github.com/nutti/fake-bpy-module/assets/13716151/8a0173dc-20ee-4e29-bd86-cf7bf0390090

nutti commented 4 weeks ago

Reverted the commit now.

@JonathanPlasse

Thank you for sharing. I think reverting the commit is good for the use-case.

BTW, is the original issue (like Object and not Object | None) only 3 cases?

JonathanPlasse commented 4 weeks ago

On my entire codebase, I only got the 3 issue I reported

nutti commented 4 weeks ago

@JonathanPlasse

I tried another strategy to fix this issue.

I uploaded the test module to TestPyPI (https://test.pypi.org/project/fake-bpy-module/20240608.dev12228/). Is it possible to try this?

pip install -i https://test.pypi.org/simple/ fake-bpy-module==20240608.dev12228
JonathanPlasse commented 3 weeks ago

Thank you, @nutti, for continuing looking into it. I got a lot of errors with this change. Screenshot from 2024-06-08 18-35-07 bpy.types.Context has duplicate attributes like scene 4 times.

https://github.com/nutti/fake-bpy-module/assets/13716151/48047621-a3bb-47e9-94ed-c42b10b9aa67

Also, the list of false positive optional is still too large.

At the moment, I am ok using cast where I need it, as it is not used extensively in my codebase. But, I still hope to find a solution that works.

nutti commented 3 weeks ago

@JonathanPlasse

Thank you. I improved the strategy again. How about this? https://test.pypi.org/project/fake-bpy-module/20240609.dev3345/

pip install -i https://test.pypi.org/simple/ fake-bpy-module==20240609.dev3345
JonathanPlasse commented 3 weeks ago

This version introduces 11 errors with Pyright on my codebase.

Handler list should not be optional Screenshot from 2024-06-09 12-52-48 BlendDataObject.new() object_data parameter should be optional Screenshot from 2024-06-09 12-54-17 List of objects should not be optional in Context. Screenshot from 2024-06-09 12-56-47

Also, none of #243, #247, or #248 is fixed with this version.

nutti commented 3 weeks ago

@JonathanPlasse

I found #247 is difficult to fix because there is no evidence to show the optional data type.

I uploaded the module generated by the newest strategy. Currently, fake-bpy-module-latest is not available. Could you test by using 4.1 instead for now?

pip install -i https://test.pypi.org/simple/ fake-bpy-module-4.1==20240610.dev12283
JonathanPlasse commented 3 weeks ago

Hi @nutti, the given pip command does not work ERROR: No matching distribution found for fake-bpy-module-4.1==20240610.dev12283.

nutti commented 3 weeks ago

@JonathanPlasse

How about below command?

pip install -i https://test.pypi.org/simple/ fake-bpy-module==20240610.dev122832

Now, we can use latest module.

JonathanPlasse commented 3 weeks ago

Hi @nutti, This is much better. It solves #243 and #248. The only problem I got is with bpy.types.Object.children which is a tuple[Object, ...] | None instead of tuple[Object, ...] like before.

nutti commented 3 weeks ago

@JonathanPlasse

Thank you for the confirmation. I also fixed that.

pip install -i https://test.pypi.org/simple/ fake-bpy-module==20240612.dev114053

I'm not sure this patch should be merged into the master branch. I could not find the side effect of this patch. How do you think this?

JonathanPlasse commented 3 weeks ago

This works for me, thank you.

nutti commented 3 weeks ago

This patch is merged now.

JonathanPlasse commented 3 weeks ago

Thank you