cats-blender-plugin icon indicating copy to clipboard operation
cats-blender-plugin copied to clipboard

Cleanup proposals

Open Mysteryem opened this issue 2 years ago • 1 comments

Organisation

  • [ ] Move all functions in tools/common.py that do not depend on any Cats code into a separate 'utils' module. I have already run into cyclic dependency issues when I only want to import a specific function instead of the whole module. This should also help break up the huge file into smaller parts.
  • [ ] If, after moving all non-Cats-dependent functions to a different module, there are still way too many functions in common, split it up further into submodules with the functions grouped by what they do or what types of objects they act on. (similarly split up the 'utils' module if that ends up huge)
  • [ ] Remove all unused code unless it's been intentionally temporarily disabled. If any is needed again, the history of the repository is always available.
  • [ ] Move exporter code out of importer.py into new file exporter.py.
  • [ ] Move all Cats added properties in bpy.types.Scene and other types to Cats specific PropertyGroups. The more properties we add directly the the Scene and other types, the more we run the risk of being incompatible with other addons due to a naming conflict. Generic sounding properties like 'armature' and 'head' are particularly concerning. The alternative would be to prefix all Cats properties with 'cats_' or similar, but I've got enough clutter in my bpy.types.Scene from other addons that aren't using a PropertyGroup already that I would much rather put them into a PropertyGroup.
  • [ ] Move EnumProperty items functions out of common.py and into their own file.

Formatting

  • [ ] Follow the PEP 8 style guide a little more. I recommend that contributors use an IDE with syntax highlighting if they are unfamiliar with common PEP 8 formatting. A lot of code has been getting let into Cats recently with missing or extraneous spaces and/or newlines. I wouldn't recommend following the max line length of 79 characters though since we have nice and wide monitors now that 79 is rather small, I'm personally sticking with PyCharm's default of 120 characters where possible.
  • [ ] snake_case all function names (unless overriding a method that is not snake_case)
  • [ ] Prefix any functions or module-level variables that are not expected to be used by other modules with _.

Coding conventions and optimisation

  • [ ] Remove all bare except clauses. There should always be at least one exception in the except clause so that it is clear what exceptions are expected that they might happen and so that unexpected exceptions are not mistakenly handled incorrectly. If it's easy to check if the exception will occur before running the code in the try block then do so instead of relying on an except clause.
  • [ ] Remove use of bpy.context in functions that already have access to the current context. This will also allow those functions to use context overrides.
  • [ ] Replace as much existing code as possible, that depends on changing the selection of objects in the current context's view layer prior to running operators, to use context overrides so that the selection of the current context's view layer does not need to be changed. This one is a bit more complicated now as Blender 3.2 has introduced a Context.temp_override function that acts as a context manager and has deprecated the old way of passing a context override as a dictionary into the operator as the first variable, meaning it might eventually be removed.
  • [ ] Where possible, replace iteration of vertices/polygons/uvs/etc. with using the bpy_prop_collection foreach_get and foreach_set fast access methods. This is less important when needing to iterate through collections that won't have anywhere near as many items, such as bones in an armature.
  • [ ] If functions in classes don't use the self parameter (and aren't overriding a method from a superclass), remove it and add @staticmethod to the function.

Personally, I would also like to drop support for 2.79 (or even all versions older than the current oldest LTS release of 2.83). 2.79 had its final update just over 4 years ago and 2.80 had a lot of breaking changes so there's a lot of code in Cats for specifically handling 2.79 that could be removed. Moving to 2.80+ also moves the minimum Python version to 3.6, giving us access to variable annotations (so we can remove the annotations part of @register_wrap and define properties using annotations), formatted string literals (print(f"Found {mesh.name}")) and dictionaries that preserve insertion order by default (yes, it's only an implementation detail in CPython 3.6, but we know that it was made the standard Python 3.7).

As far as I can tell, the only reason we have google_trans_new copied directly into Cats instead of as a submodule, is so that we can disable all the code that uses urllib3 due to it not being included with 2.79's bundled Python.

Additionally, if we were to drop support for 2.79, would we be able to stop including mmd_tools directly and instead include it as a submodule too? The new 2.3.0 version of mmd_tools itself, that is now in the development branch as mmd_tools_local, dropped support for 2.79 so I can see that we've had to modify it for 2.79 support.

Mysteryem avatar Jun 24 '22 15:06 Mysteryem

Dropping 2.79 also means we could change some code that requires Edit mode and iterates through a list of meshes, to open all of the required meshes into Edit mode simultaneously instead of opening each mesh individually, due to 2.80's new feature of multi-editing.

Mysteryem avatar Jun 26 '22 15:06 Mysteryem