User:Sybren/SfA/roast-my-addon

= Scripting for Artists: Roast my Add-on =

Suggested in the Blender Today livestream.

The add-ons sent in for roasting:


 * tonton @tonton97583844 https://github.com/samytichadou/Auto_Reload-Blender_addon
 * ambi @th127: https://github.com/amb/blender-texture-tools
 * tonton @tonton97583844 https://github.com/samytichadou/blender_project_manager
 * ocupe @ocupe2 https://github.com/Ocupe/Projectors

tonton: auto-reload

 * Nice readme, including example video and link to download the add-on as ZIP.
 * Problem: version number in the ZIP directory. This means that Blender will install multiple versions side-by-side when you upgrade. This is not standard, and also not documented.
 * Confusing file layout. 'developer_utils' is something different than 'dev'. Not sure what would go into 'developer_utils', 'dev', 'misc', or 'functions'.

ambi: Blender Texture Tools

 * Has a nice readme that explains what it does. No images though, which is kind of a shame.
 * Has a `.gitmodules` file, indicating the use of Git submodules. Doesn't explain why.
 * `inactive` directory. That's probably better thrown away. You can always go back to your Git history. If you want to remember, set a tag or make a branch. Then delete.
 * `.clang-format`: nice, auto-formatting is a good idea.
 * No separate directory for sources. This means that the add-on will be imported as `blender-texture-tools`. Although it works as add-on, it's not a valid Python name.

`__init__.py`

 * Nice, GPL header.
 * More than 1300 lines of code, big add-on.

What is going on here?
 * `CUDA_ACTIVE` is set three times, but only two outcomes.
 * `import numpy as np` is common in the numpy world, but I've never seen `import cupy as cup`. Even cupy docs say `import cupy as cp`.
 * cupy is cuda-compatible numpy. Strange what is happening here; after this block, is `np` used or `cup`?
 * Improvement, assuming that cupy should be used, falling back to numpy if cupy is not available:


 * I would want to replace `except Exception` with something like `except ModuleNotFoundError`, but I don't know which exceptions are raised when the module is installed but doesn't find a CUDA-supporting video card.
 * `CUDA_ACTIVE` is only used in one place in the code, and this can be replaced with `if np.__name__ == 'cupy'`. That way you don't have to synchronise `CUDA_ACTIVE` with the module loading.

``` class BTT_InstallLibraries(bpy.types.Operator): bl_idname = "image.ied_install_libraries" ```
 * The class doesn't follow the standard naming `CATEGORY_OT_name`. This isn't even used in the Blender code example templates themselves, so not a big deal.
 * The naming would suggest a `bl_idname` like `btt.install_libraries`, but it's different. Not sure what `ied` means.


 * Don't use `from subprocess import ...` -- the names in the subprocess module are very generic, so you'd get calls like `call` or `run`. Hard to figure out what's going on when you just look at the call itself.
 * Use `subprocess.run(check=True)` instead. `call` does NOT check for errors, so your code just keeps running if any CLI command failed.
 * Not sure how cross-platform this is nowadays. If it works for you, good.


 * Declare global variables at the top of the function. That way it's much clearer that that function modifies the global scope.
 * With my changes, assigning `CUDA_ACTIVE` is no longer necessary, and `cup = cupy` should become `np = cupy`.
 * No error handling around the import statement.
 * No information on how to uninstall.


 * `__name__` here indicates the name of the file, not the name of the class. Just be explicit and write the `bl_idname`.


 * Don't compare directly to True or False in an if-statement. Use `if not CUDA_ACTIVE`. Or, in this case, `if np.__name__ != 'cupy'`
 * Flip the condition, and write:


 * Bad naming. What is `x`? Never use single-letter names. The only exceptions are `i` for index in a loop, or `x`, `y`, `z` as coordinates (but then I'd still use `loc_x` or `location_x` to distinguish between locations, translations, orientations, and rotations).
 * `gauss_curve_np` is a copy of `gauss_curve`, but one is using cupy and the other numpy. Given that cupy is numpy-compatible, this should not be necessary.
 * At least write one function that takes `numpy` or `cupy` as argument.
 * Add type declarations, like `gauss_curve(x: int) -> numpy.ndarray:`
 * Overall: there are no comments, no docstrings. Nothing to indicate which function does what. Not necessary when it's clear, you don't have to document "x: the x-coordinate". These functions are not clear, though.
 * Same for other functions, `def aroll0(o, i, d):`, what is the difference with `def aroll1(o, i, d):`? What are these single-letter parameters?
 * Naming again. If you feel the need to add a comment that explains the naming, change the variable names. Instead of `spp` just write `source`. Or `source_image`.
 * No documentation on what is returned. Does this apply the convolution to the source image? Or does it return a new image?


 * `def hist_match(source, template):` is all of a sudden documented. Could this be copied from somewhere? Yes it is: https://stackoverflow.com/questions/45926871/webcam-color-calibration-using-opencv.


 * `def hi_pass_balance(pix, s, zoom):` has `yzoom = zoom if zoom < yzm else yzm`. This is a brain teaser, just write `yzoom = min(zoom, yzm)`. Also naming. WTF is `yzm`?


 * You already have a multi-file add-on. Why is this file so big? It's clearly consisting of some basic math functions you need for the operations, the operations themselves, and Blender operators, Blender UI, and more. This can easily be split into several files.


 * Wait what? If `self.payload` is a function, especially one that takes `self` as well, just override it!
 * Going further: It all looks over-engineered, with operator generators that are subclassed and provide yet more complexity. I have the feeling that a few mix-in classes can replace all of this code, making this easier to understand for anyone except the original writer. What you end up with now is a function call with 9 parameters to `PanelBuilder`.