 Hello and welcome to Scripting for Artists. My name is Sebelin and this episode is called Roast My Add-on. I've asked on social media for your add-ons for me to roast so that we can go through it and see what is bad, what is good, and how things can be improved. We look at a few different add-ons and in the end of this episode, I will extract some general hints and tips for you to use in your own code. On Twitter, Tonton says, Hey, thanks. That's a great idea. I would be glad to have some pros inside about my coding. This first add-on is a project manager for anim workflow and the other add-on is a simple tool to reload images in the blend file. Feel free to pick one if you have some time. Cheers. I will pick the second one first because that looks like something simple and nice to start with. So let's take a look at the code. Here we are on GitHub and the first thing I notice is that it has a description, which is good, handy automatic reload for image textures. Then there is a bunch of files and there's a readme that says it's Blender 2.80 add-on so it's compatible with more modern Blender versions. I'm assuming that it will also run on newer than 2.80. It can refresh all the images of the blend file, it can use a timer to fetch modified image file and reload them if needed. That is pretty handy and already I want to use it. It has a link to download and it has a little demo video. If you want to watch the video I've put the link down below in the description so you can just click it there. I'm looking at all the files that are there and I wouldn't exactly know what to expect where. There's developer utils, there's functions, there is dev, I don't know how dev differs from developer functions and then there is misc, probably short for miscellaneous. So where would some function go? Would it be miscellaneous? Would it be dev? Would it be developer utils? Would it be functions? So I try to always avoid naming things utils or helpers or miscellaneous. Generally that's an indication that it's just a bag of stuff that is going to expand over time and that is not something you want in your source code. Now let's take a look at the init file. 90 lines in the file and 71 source lines of code. So rather small file which can be pretty nice so it gives a nice overview. It starts with a copyright information, GPL, license of course, BL Info is all given, it even shows where you can find the settings for this add-on. It has a wiki URL, it has a tracker URL, import bpi so far so good. Now it imports import lib and inspect from period in import developer utils which means from the current module import the sub module called developer utils and then it tries to reload that. This works but generally the reloading scheme is a little bit different for multi file modules but if this works for you that's fine. Then we have modules is developer utils set up add-on modules path name bpi in locals. Okay so magic is going on there and then classes and is an empty list and then so this is basically I'm guessing a collecting all the Python classes that need to be registered and unregistered from the sub modules. Usually I wouldn't do it in this way I would give every sub module its own register and unregistered function and just call that from the top level function. That way there is no magic it's all pretty clear where things are called from. Yeah I'm not a I'm not a big fan of this kind of magic and then there is from dot functions import reload startup so from the sub module functions it imports the reload startup thing I think it's a function. Okay then for the register some properties are added which are all fine and then for CLS in classes called register class yes so this uses this classes list that you define here and then registers everything all right and then the reload startup is added as a post load handler. The del CLS here is really not necessary because the only thing that does in Python is it removes the CLS name but then on the next iteration CLS gets the next class assigned to it anyway so then it exists again this is not like C++ where it would delete the actual object in memory it just removes the name from the current namespace so you can no longer use it. Because of all the magic I still don't know what is defined where so let's go and look at a different file. Let's take a look at operators and see what kind of operators there are. Let's stop being imported more stuff from the oh if you have to import so many names from that module I would rather just import the module itself like from dot import functions and then call functions dot update viewer or functions dot get my dear that way it's easier to see again where things are coming from this should be the core of the code. Oh and please just no don't no don't do this like indent your code. Python is based on indenting and if you write it like this it's so hard to see what is going on so if length of modified list doesn't equal zero so that means if there's stuff in there Python automatically can convert things to Boolean for you so the list if it's empty is considered false and if it's not empty is considered true so this means that you can say if modified list instead of if Len modified list doesn't equal zero I'm getting ahead of myself let's see WMS context window manager okay fair enough it's not used here it's not used here so that could be few lines lower then modified list and missing list is reload modified images so I'm guessing that load reloads all the modified images like the images that were modified on disk so it's nice to have that in a separate function that's good so then if there is images modified with updated viewers which is also nice it also passes the context that's also nice I'm not sure why there is no context here maybe it's not necessary maybe it uses the global context I don't know there's a style conflict here so using camel case here you're using underscores pick one and stick to it and I would say pick the Python style that's documented in PEP 8 which means use the underscores here for functions then if there's no missing images auto-reload missing images is set to false otherwise auto-reload missing images is set to true well you can just rewrite this right here I have the same following my code editor so I can show you what I mean first I'll re-indent it this is already a little bit nicer now if we compare that what happens to the f above that we can see that if land modify list is not zero then we do something so first we check for whether that list has elements and then we have a very similarly looking if statement that checks for whether it's empty and this will cost you brainpower so I would say if modified lists that's it if modified list update viewers context if there's anything in there update your viewers and this is way simpler and we can do something similar to this as well but this checks whether it's empty so if it's empty it does this if it's not empty it does this so this checks where there is empty which you can just write it this if not missing list then set something to false else set something to true I don't quite like the if not in combination with the else if you're going to handle both the true and the false cases anyway then you might as well get rid of the negation by swapping what is in the f and what's in the else so I move the equals false down I move the equals true up and I remove them not here and now what you have is two consecutive checks that do the same thing if modified list do something if missing list do something else do the opposite the cool thing about improvements like this is that they really help the readability of your code and I actually do this a lot once I've written the code I will go back in and see okay now that is working and I improve it can I clarify what's going on if you want we can take this one step further because basically what this is saying is how to reload missing images when there are missing images so we can write that as an expression this does exactly the same thing if missing list is considered true bull missing list is true and assigns true to auto reload missing images and the same for when the list is empty it is false so this line expresses what you actually want auto reload missing images when there are missing images the rest of the coaches seems to be for debugging and here you can also see the issue with the imports if you import if you do from dot global variables import sign comma reloaded now here you all of a sudden have sign and reloaded and missing and it's hard to see where that comes from let's continue with the add-on the next function is load font and immediately I see something that I really don't like area of a try except block and the except is just a bear except so it will catch anything this means it will also catch a keyboard interrupt when the user presses control C to to break off some hanging operation it will catch any error including the ones that you don't want to catch at all and then this bit of code the race exception it doesn't include anything anymore so when the original exception would have provided some information as to what is going wrong maybe the font cannot be loaded maybe the memory is full maybe the font is corrupted maybe something else went wrong you never know so just remove it otherwise this code is quite simple so that's all fine one last thing that I would really like to see is a bit of documentation about what self is in this case because when you just go through the code top to bottom you have no idea what it is let's quickly go through the next operator this is the actual reloading timer so because this video has been going on for quite a bit already I'll go into a quick rose mode here we have OS path join of OS path join that's not necessary you can just do it like this I wouldn't mix regular Python properties with lender properties except attribute error no just fix your code make sure that there is no attribute error comparison to false nope just do if not else after return no I wouldn't do it you can just do this which means that you can just flip this condition around and then return pass through an event this and that we can do again there is if something is not something else well you can just flip it and do that and now I've already undented the code twice this code we've seen it before no don't do that where the function put the code there call it from two different places the rest of the code pretty much follows the same style so I won't go through that again thank you very much Tonton for lending me your code to roast and let's move on to the next so the next add-on is by a cube and he is very to the point roast my add-on so here we go his code is on github as well so we can scroll through it I see a bunch of files including a read me which is nice it has some examples it shows what it can do Germans like to call a projector a beamer same in the Netherlands actually let's take a look at the init pie because that's where it all starts import logging cool nice separate imports that's also good bill info it seems to be complete no don't do this this configures the entire logging module of python and basically forces every user to have debug logging for everything in a specific format this is really that something that you should leave to users or other developers to control on their system also the log here is not used at all so there's no point in creating it nice separate register and unregistered functions per module that's what I like to see so let's take a look at the operators but there's only one it sets the render engines with cycles let's take a look at the projector itself I kind of expected an operator that's an ad new projector to the scene in the same way that we added a monkey grid to the ad mesh menu again you call logging the basic config which again overrides the configuration that you have just set in a different module so this is really a weird way of going about things as I said before I'm not a fan of modules being called helper util miscellaneous that kind of stuff but let's take a look at these get projectors gets the context and only selected which default to false but when you look at the references you only call it with only select as is true so why not just name the function get selected projectors and have it over with random color alpha equals false as default you only use it twice so why not remove the default and just pass alphas true and alphas false and then you have it explicitly said what I do like is that you say alphas true and not just true this makes it much clearer what is it going on in that particular call so on to the rest of the code you define some stuff which is nice but I'm not here to be nice then projector OT change color randomly first operator that we see why is this not in the operators module there's something weird going on because the poll method only allows this operator to be called when the number of selected projectors equals one but then you select all but then here you take all the projectors and just look over them so if you have more than one selected it would actually work blender being blender this is basically the difference between working on the active object of working on the selection and this code seems to confuse the two that we have a function create project textures you construct an image name from the resolution which is fine then you check whether the image is there and then you use an operator to actually create the image if it doesn't exist yet the problem with this approach is that this operator may create an image with a different name than you give it even image with that name already exists it will just depend on 001 that means that this line may actually access the wrong image now the chances of that happening are pretty much zero because of the line before it but I would never use this anyway here we have the code in my editor so I can show you some changes that would do so instead of putting this in the f I would just say image is this this get function basically means give me the image with that name if it doesn't exist don't throw an error but just return none if not image then create a new image which you can do like this this creates the exact same image except that it gives you a reference to that data block that it just created so even when that name was unavailable and they choose something else or something completely different happened you get the actual image data block that you're interested in and then you can set properties on it in this in this case we have to set the generated type because we can't pass that directly to the new function and this can then just use image dot use fake user is true I think this reads a little bit nicer and also it makes less assumptions about what's going on behind the scenes the next function is called add projector node tree to spot which I think is quite a good name because it exactly says what it's doing it is documented as this function returns a spotlight into a projector and this is the first time that we'll learn that a projector is actually modeled as a spotlight this is actually something that I would have expected to read me and not in the middle of some code so when we scroll down what I notice is that there's a comparison with the blender version I think this is a good idea the only thing is that the index here on the inputs array you can also use a string for that here in inputs instead of writing three you can also write the string scale the rest of the add-on is boring because the codes were written and it's pretty clear what it's doing the only comment that I have is that it could use a few more comments so let's move on to the final add-on that I will be roasting today and be is wondering how long a book are you going to write well and be not as long as your add-on he made the blender texture tools a blender add-on for simple image manipulation and I wish that the source code was as simple as that description so looking at the files I already noticed that there is a git sub module in here git sub modules are a way to combine multiple git repositories into one project and I think for a blender add-on this may actually be overkill. Let's start again at the initpy free software that is very nice and now we have some import magic numpy is a Python library that allows you to do number crunching and it's good at handling large amount of data so I think it's a good choice for this particular add-on because it has to deal with images but then the rest there is apparently QPy is basically a library that allows you to do the same operations as numpy but then in CUDA on your GPU again I think it's a good choice to use here but the way that it's handled is really weird CUDA active is set to false or true depending on whether QPy was loaded or not to me this code looks like it's trying to use QPy if it exists on the system and it can be loaded and otherwise fall back to numpy instead however that's not how this reads is really confusing and in the end I don't know what is going to be used by the rest of the code. Let me give you an example of how I would do this. So first of all I'm going to assume that we want to use QPy unless that's not available and then we want to use numpy but in the end the code is going to be using either one or the other library. So the try and import QPy is fine but let's import it as NP. Importing numpy as NP is really common because that prefixes use a lot in your code and the difference between two and five letters is going to be considerable given the number of implications that you have on a typical line so let's just keep using that NP and use QPy as a dropping replacement. So we're going to try to import QPy and if that fails then we're going to import numpy. Both will end up as the NP name so once this is done we can just say NP.something and it will use whatever was loaded here. This immediately shows the reader what is going on here and what is the intention behind this code. Now I've already looked ahead a little bit and the CUDA active is only used in one place. That place is here in the add-on preferences. It says if CUDA active is false well instead of this we can now just write if NP.name equals numpy. This avoids the whole keeping track of what was loaded and was not because Python already does that for us. So QPy is a library that you can install. It doesn't come bundled with Blender so we made an operator that can download it for us which is pretty nice but the way he does it not so much. From sub process import call. I wouldn't do that because then you have a call name that is just in your namespace there and you get these kind of calls that call call and it's not immediately obvious that it is actually calling a sub process. So I would always say import sub process and then sub process at call. However a sub process at call doesn't check any errors so it could be completely failing and you wouldn't know. So instead of this use sub process.run with check equals true as a parameter then at least it will stop your Python code from running any further when the download or the install fails. In this case you have a double problem because you already set QtaActive to true before importing QPy. So even when all of this failed import QPy will fail again but then you have QtaActive already set to true and then your state is all messed up. Let's browse through the code a bit more. I see a lot of small functions that do something very specific which is kind of nice but the naming is a bit strange. What is IG? What does the neighbor average do? What does it return? There is no documentation at all about any of the types. There is no type and notations that explain what is returned or what is expected. Especially when you're using short variable names here like O, I and D. I don't know what's going on. A roll 0? Or is a roll 0? Why is it different from a roll 1? And why is a roll 0 different from add roll 0? Here if you have a comment that needs to explain what the parameters mean just name the parameters differently. Instead of SSP name it source instead of Intense name it Intensity. So a convolution is an image operation and here there's a problem because it doesn't tell us whether the source image is changed or whether a new image is returned. This is something that permeates through this code. Also this grayscale function does it turn the given image into a grayscale image or does it return a grayscale copy of the image. This should be documented well because it really helps you understand what's going on. So having scrolled down about a thousand lines of code we get to these classes which are subclasses of image operator generator and this is starting to turn into a bit of code smell. It has one function called generate and then it sets certain keys in self-taught props to blender properties and it sets the prefix it sets info and category and then it has self-taught payload which is a lambda function. I won't even go into what a lambda function actually is. I want to know what this image operator generator is. I found it here in a different file and it's a subclass of operator generator and it gets somehow a master name which it passes to in it begin then it says force num point equals false calls that generate function that we already saw then in it end it sets its own name to image ot and then self.name so it doesn't even wait but here you don't have a name at all so I don't even know where this name comes from and then it calls create op which I guess creates the blender operator and then it says self.op dot first name numpy equals self.force numpy which was already set to false here. I found the operator generator inside the get-stop module so here we have the in it begin and it gets the master name which is then assigned to the parent name so apparently the parent is the master I don't know what that means even. It says self.payload instead of having a payload function then it says a bunch of other things to empty but not that name that is only set in in it end and there is a create op function that dynamically creates a new class. I think this is clever code but it's a bit too clever it is so hard to follow and especially when you go into blender and these operators have been registered and then you want to search for where that operator was created you will never find it. That naming convention of having category in capitals and then underscore ot underscore and then the rest of the identifier that is very useful because that is used all over the place if you have bpi.ops.something. something you can apply those rules and then you have the class name so given a name of something in blender it's pretty easy to find it in the source code and with this structure that name is nowhere I think the advantage of this code is that it is pretty easy to add new operators. Personally I would make the code just a little bit larger type a bit more but then have everything there where it's supposed to be easily findable easily understandable how things are created okay we made it to the end I hope you have enjoyed this roasting as much as I have so as a final thought name things well so that you don't have to use your brain later on when you try to read it return early and flip those conditions and see if it makes a difference so that's it for this episode if you have any questions or comments leave them below and I will see you soon