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: 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.
import numpy as np

CUDA_ACTIVE = False
try:
    import cupy as cup

    CUDA_ACTIVE = True
except Exception:
    CUDA_ACTIVE = False
    cup = np

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:
try:
    import cupy as np
except Exception:
    import numpy as np
  • 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.
    def execute(self, context):
        from subprocess import call

        pp = bpy.app.binary_path_python

        call([pp, "-m", "ensurepip", "--user"])
        call([pp, "-m", "pip", "install", "--user", "cupy-cuda100"])
  • 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.
        global CUDA_ACTIVE
        CUDA_ACTIVE = True

        import cupy

        global cup
        cup = cupy
  • 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.
class BTT_AddonPreferences(bpy.types.AddonPreferences):
    bl_idname = __name__
  • __name__ here indicates the name of the file, not the name of the class. Just be explicit and write the bl_idname.
if CUDA_ACTIVE is False:
  • 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:
        if CUDA_ACTIVE:
            row = self.layout.row()
            row.label(text="All optional libraries installed")
            return

        info_text = (
            "The button below should automatically install required CUDA libs.\n"
            "You need to run the reload scripts command in Blender to activate the\n"
            " functionality after the installation finishes, or restart Blender."
        )
        col = self.layout.box().column(align=True)
        for l in info_text.split("\n"):
            row = col.row()
            row.label(text=l)
        # col.separator()
        row = self.layout.row()
        row.operator(BTT_InstallLibraries.bl_idname, text="Install CUDA acceleration library")

def gauss_curve(x):
  • 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?
def convolution(ssp, intens, sfil):
    # source, intensity, convolution matrix
  • 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 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.
class Grayscale_IOP(image_ops.ImageOperatorGenerator):
    def generate(self):
        self.prefix = "grayscale"
        self.info = "Grayscale from RGB"
        self.category = "Basic"
        self.payload = lambda self, image, context: grayscale(image)
  • 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.

tonton: Blender Project Manager

ocupe: Projectors

Retrieved from "https://wiki.blender.org/w/index.php?title=User:Sybren/SfA/roast-my-addon&oldid=16389"