Roast My Code (Color Switcher)

[don1138]

Well, rather than a roasting, I’m actually asking for peer review

I’ve made some additions to my add-on, but I wonder if I’m going about it the long way. I’m posting here to see if anyone has any suggestions for improving or simplifying it.

The goal is to let users select one, several, or all nodes in a material, and change the Base Color of all selected nodes that have a Base Color input.

Currently, I give each type of shader node that has a Base Color input its own if statement.

To test if I should display an error message, I have a counter that ticks up for each node changed.

Here’s the relevant chunk:

# COLOR SWITCHER

def set_base_color(hex):
  material = bpy.context.object.active_material

  if material:
    AN = material.node_tree.nodes
    g = 0

    for n in AN:
      if n.select:
        if n and n.bl_idname == "ShaderNodeBsdfAnisotropic":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfDiffuse":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeEmission":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfGlass":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfGlossy":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfHair":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfPrincipled":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfHairPrincipled":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeVolumePrincipled":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfRefraction":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeSubsurfaceScattering":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfToon":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfTranslucent":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfTransparent":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBsdfVelvet":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeBackground":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeVolumeAbsorption":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
        elif n and n.bl_idname == "ShaderNodeVolumeScatter":
          n.inputs[0].default_value = hex_to_rgb(hex)
          material.diffuse_color = hex_to_rgb(hex)
          g += 1
      if g == 0:
        ShowMessageBox(no_active, "Unable To Comply")
        return {'FINISHED'}
  else:
    ShowMessageBox(no_material, "Unable To Comply")
    return {'FINISHED'}

First off…

It’s obvious I should take the repeating code and make it a separate class like so:

def apply_color(na,ma,ga)
  na.inputs[0].default_value = hex_to_rgb(hex)
  ma.diffuse_color = hex_to_rgb(hex)
  ga += 1

and then apply it to each case:

if n and n.bl_idname == "ShaderNodeBsdfAnisotropic":
  apply_color(n,material,g)

But beyond that…

Four questions occur to me:

1. Do I actually need to include bpy in material = bpy.context.object.active_material?

2. Am I using return {'FINISHED'} too many times?

3. Is there a better way of testing for no nodes changed than the counter?

4. And most importantly, is there a more efficient way of looping through the node types than by using multiple if statements? Maybe an array?

As always, feedback is greatly appreciated.

[joseph]

@don1138

Do I actually need to include bpy in material = bpy.context.object.active_material?

Yes, unless you do something like “C = bpy.context / c.object…”

@don1138

Am I using return {'FINISHED'} too many times?

No, you have to use it (although you could probably consolidate a few uses, but it’s not going to hurt anything)

[iceythe (Kaio)]

for n in AN:
    if n.select:
        if n and n.bl_idname == "ShaderNodeBsdfAnisotropic":

You don’t need “if n”. Having it there is confusing, because it makes me wonder if n’s __bool__ method is overridden, or if n ever becomes None which doesn’t make sense because you’re accessing n.select in the previous line, etc etc.

If n was ever evaluated to False, something else is very, very wrong.

You can deduplicate your code by putting the strings of node types in a set,

node_bl_idnames = {
    "ShaderNodeBsdfAnisotropic",
    "ShaderNodeBsdfDiffuse",
    "ShaderNodeEmission", }  # ..and all the others.

.. then do a check that covers all of the nodes.

for n in AN:
    if n.select and n.bl_idname in node_bl_idnames:
        n.inputs[0].default_value = hex_to_rgb(hex)
        material.diffuse_color = hex_to_rgb(hex)

        # (Nothing more to do now, actually. Drink_tea() maybe)

@don1138

Am I using return {'FINISHED'} too many times?

Multiple returns should return different values, otherwise it’s confusing to the reader.
If I were pedantic, I’d find it strange that a free function is returning a value that has no meaning outside of an operator method. It would make more sense to return a True/False value, which you check in your operator, then let it return a set with a value.

Note that returning {'FINISHED'} indicates a successful operation, causes the view layer to be updated and pushes an undo step for undo-enabled operators, while returning {'CANCELLED'} does nothing. If your operator uses undo, this has an impact.

@don1138

Is there a better way of testing for no nodes changed than the counter?

A common approach is to use a changed variable.

changed = False

for a in b:
    if x or y:
        if z:
            changed = True

if not changed:
    ...
else:
    ...

Using a counter technically works, but it doesn’t show your intent. The pattern above is fairly common.

[don1138]

Mille grazie! I’ve incorporated these changes into my code.