With the single-button import approved, let’s see what code we can remove from the rcg file.
There should be at least one class, perhaps two, that we can remove.
The first suspect for removal, of course, is our class that did the specialized imports. That’s this one:
class RCG_OT_importObject(Operator):
""" Add an object from a premade blender file """
bl_idname = "rcg.importobject"
bl_label = "Size"
bl_options = {"REGISTER", "UNDO"}
rcg_file: bpy.props.StringProperty(name="Default Value")
@classmethod
def poll(cls, context):
return context.mode == "OBJECT"
def execute(self, context):
if self.rcg_file != "":
name = self.rcg_file
else:
name = "track"
self.report({"WARNING"}, "defaulting to " + name)
self.report({"INFO"}, "adding " + name)
filepath, directory, filename = make_elements(name)
bpy.ops.wm.append(filepath=filepath,
directory=directory,
filename=filename)
item = bpy.data.objects[name]
item.select_set(state=True, view_layer=bpy.context.view_layer)
bpy.context.view_layer.objects.active = item
return {'FINISHED'}
Checking for references to Blender classes is tricky, in that PyCharm is not aware that the bl_idname is used to reference things. So I’ll do a string search for ‘rcg.importobject’ first. The only references seem to be the menu references, all commented out:
...
# col.label(text="Add a track object", icon='ANIM')
# row = col.row()
# row.operator("rcg.importobject", text="Normal").rcg_file = "track"
# row.operator("rcg.importobject", text="Inverted").rcg_file = "invtrack"
# row.operator("rcg.importobject", text="Railway").rcg_file = "ngtrack"
col.label(text="Import track object", icon='ANIM')
col.operator("rcg.importfromfile", text="Select file")
# col.label(text="Add a track ruler", icon='ARROW_LEFTRIGHT')
# row = col.row()
# row.operator("rcg.importobject", text="Ruler").rcg_file = "trackruler"
...
Time to remove those: done. Looking for references to the class itself, we expect and find only the one in the list of classes to register:
classes = [ RCG_OT_addarray,
RCG_OT_addbezcurve,
RCG_OT_addcolumn,
RCG_OT_addnurbscurve,
RCG_OT_apply,
RCG_OT_createbeziercurve,
RCG_OT_createnurbscurve,
RCG_OT_Export,
RCG_OT_importObject, # <------
RCG_OT_importfromfile,
RCG_OT_inputempties,
RCG_OT_inputnurbspath,
RCG_PT_sidebar,
RCGSettings,
SelectFileEmpties,
SelectFileNurbs, ]
Remove that, and the class. Done. The tests all pass, no surprise there, since we aren’t really testing any of the Blender classes: they can’t run outside Blender. Before committing, I’d best try loading rcg and testing in Blender. Tedious but better now than later. I just add a bit of track, which seems to work. It seems very small compared to the standard cube. I hope that’s right.
Commit: remove unused import object class and references.
Looking around, I notice the PropertyGroup settings, which I don’t remember creating, but there it is, with nothing in it but settings for the support columns. Its name is my_settings, which I think is risky because I probably lifted it from an example, so, in principle, some other script or add-on might use the same name, if built by someone as inexperienced as I am. Let’s rename that to rcg_settings.
Easily done. Commit: rename my_settings to rcg_settings.
Of all the classes remaining, the addcolumn one is the messiest, and now that it works, it could use a bit of refactoring. Here’s a look at it as it stands, so we can see how messy it is:
class RCG_OT_addcolumn(Operator):
""" Add support columns"""
bl_idname = "rcg.addcolumn"
bl_label = "Add Supports"
bl_options = {"REGISTER", "UNDO"}
@classmethod
def poll(cls, context):
return context.mode == "OBJECT"
def say_info(self, msg):
self.report({"INFO"}, msg)
def execute(self, context):
offset_desired = context.scene.rcg_settings.offset_distance
column_spacing = context.scene.rcg_settings.column_spacing
column_diameter = context.scene.rcg_settings.column_diameter
# self.say_info(f"Offset {offset_desired}")
activate_object_by_name('ruler')
obj = bpy.context.object
if obj is None or obj.type != "MESH" or 'ruler' not in obj.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
root_collection = self.set_rcg_collection_active() # set to our collection
fins = obj.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
every_nth_pair = pos_up_pairs[::column_spacing]
for pair in every_nth_pair:
self.place_column(pair, column_diameter, offset_desired)
bpy.context.view_layer.active_layer_collection = root_collection # reset collection
return {'FINISHED'}
def place_column(self, pos_up_pair, diameter, offset_desired):
position_vert = pos_up_pair[0]
pos_vec = position_vert.co
up_vec = pos_up_pair[1].co
raw_offset = - (up_vec - pos_vec)
mul = offset_desired / 0.5
pos_vec = pos_vec + mul * raw_offset
z_size = pos_vec.z
bpy.ops.mesh.primitive_cylinder_add(
location=(pos_vec.x, pos_vec.y, pos_vec.z - z_size / 2),
vertices=6,
radius=diameter / 2.0,
depth=z_size,
end_fill_type='NOTHING',
enter_editmode=False)
ob = bpy.context.object
ob.name = 'Support'
bpy.ops.object.shade_smooth()
def set_rcg_collection_active(self):
root_collection = bpy.context.view_layer.layer_collection.children[0]
columns = bpy.data.collections.new("RCG Supports")
scene = bpy.context.scene
scene.collection.children.link(columns)
bpy.context.view_layer.active_layer_collection \
= bpy.context.view_layer.layer_collection.children["RCG Supports"]
return root_collection
There are things unclear here. The set_rcg_collection_active method, for example. We want to put all our supports into their own collection, RCG_Supports. So while we are adding, we want that collection to be active. After we’re done adding, we want the root collection to be active. So the set_rcg_collection_active method creates our collection and sets it active, returning the root collection, which at the end of placing all the columns, we set back to active.
I think we could probably just set the active collection to bpy.context.view_layer.layer_collection.children[0] at the end, but I felt more comfortable saving it and resetting it. I’m not really sure whether collection[0] is always the root, or is the active collection.
As I read this, I see that we use the term columns but the “official” name is now supports. We might do well to do some renaming.
def set_rcg_collection_active(self):
root_collection = bpy.context.view_layer.layer_collection.children[0]
support_collection = bpy.data.collections.new("RCG Supports")
bpy.context.scene.collection.children.link(support_collection)
bpy.context.view_layer.active_layer_collection \
= bpy.context.view_layer.layer_collection.children["RCG Supports"]
return root_collection
Slight improvement, I think. I also inlined scene because I felt the extra line detracted from readability rather than helping. Commit: tidying.
I’m not really sure if that’s the root collection or not. WHatever it is, we set it active and things seem OK so … we’ll live in partial ignorance for a while longer.
This method could use improvement:
def place_column(self, pos_up_pair, diameter, offset_desired):
position_vert = pos_up_pair[0]
pos_vec = position_vert.co
up_vec = pos_up_pair[1].co
raw_offset = - (up_vec - pos_vec)
mul = offset_desired / 0.5
pos_vec = pos_vec + mul * raw_offset
z_size = pos_vec.z
bpy.ops.mesh.primitive_cylinder_add(
location=(pos_vec.x, pos_vec.y, pos_vec.z - z_size / 2),
vertices=6,
radius=diameter / 2.0,
depth=z_size,
end_fill_type='NOTHING',
enter_editmode=False)
ob = bpy.context.object
ob.name = 'Support'
bpy.ops.object.shade_smooth()
Using PyCharms refactoring tools is safe, so let’s first extract a method from that first bit. Down to where we set pos_vec, I don’t think we use any of the variables we create further on down. Using PyCharm’s Extract Method:
def place_column(self, pos_up_pair, diameter, offset_desired):
pos_vec = self.get_position_vector(offset_desired, pos_up_pair)
z_size = pos_vec.z
bpy.ops.mesh.primitive_cylinder_add(
location=(pos_vec.x, pos_vec.y, pos_vec.z - z_size / 2),
vertices=6,
radius=diameter / 2.0,
depth=z_size,
end_fill_type='NOTHING',
enter_editmode=False)
ob = bpy.context.object
ob.name = 'Support'
bpy.ops.object.shade_smooth()
def get_position_vector(self, offset_desired, pos_up_pair):
position_vert = pos_up_pair[0]
pos_vec = position_vert.co
up_vec = pos_up_pair[1].co
raw_offset = - (up_vec - pos_vec)
mul = offset_desired / 0.5
pos_vec = pos_vec + mul * raw_offset
return pos_vec
Now the first method does a bit better at saying what it does, and the second one says how it does it. We could perhaps do better, and perhaps we will. The rest of the first method adds the support, so let’s name that too. Again I use PyCharm ExtractMethod.
def place_column(self, pos_up_pair, diameter, offset_desired):
pos_vec = self.get_position_vector(offset_desired, pos_up_pair)
self.add_support(pos_vec, diameter)
def add_support(self, pos_vec, diameter):
z_size = pos_vec.z
bpy.ops.mesh.primitive_cylinder_add(
location=(pos_vec.x, pos_vec.y, pos_vec.z - z_size / 2),
vertices=6,
radius=diameter / 2.0,
depth=z_size,
end_fill_type='NOTHING',
enter_editmode=False)
ob = bpy.context.object
ob.name = 'Support'
bpy.ops.object.shade_smooth()
Now our first method says what it does and the extracted ones say how it happens. One more thing, let’s rename the base method place_support. PyCharm rename, Shift+F6 on my computer.
def place_support(self, pos_up_pair, diameter, offset_desired):
pos_vec = self.get_position_vector(offset_desired, pos_up_pair)
self.add_support(pos_vec, diameter)
The refactoring operation reminded me of the Notes and Journal files in the repo. I think those can go now, as these articles take their place. But before I remove them, I guess I should check to see if they should be moved over to the github.io pages. We’ll hold off on that.
We can commit this, so let’s do. Commit: refactoring support addition code.
You’ll not that when I use the machine refactoring commands, I am not worried about breaking things. They’re very good about warning you if they’re going to break something. Are they perfect? No. Should I test in Blender pretty soon? Yes. Quite yet? No, not quite yet.
Let’s see about renaming the class itself, and about the execute method.
class RCG_OT_addSupport(Operator):
PyCharm renamed the reference in the classes list. I moved it into its new alphabetic position.
Now execute:
def execute(self, context):
offset_desired = context.scene.rcg_settings.offset_distance
column_spacing = context.scene.rcg_settings.column_spacing
column_diameter = context.scene.rcg_settings.column_diameter
# self.say_info(f"Offset {offset_desired}")
activate_object_by_name('ruler')
obj = bpy.context.object
if obj is None or obj.type != "MESH" or 'ruler' not in obj.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
root_collection = self.set_rcg_collection_active() # set to our collection
fins = obj.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
every_nth_pair = pos_up_pairs[::column_spacing]
for pair in every_nth_pair:
self.place_support(pair, column_diameter, offset_desired)
bpy.context.view_layer.active_layer_collection = root_collection # reset collection
return {'FINISHED'}
I wonder if the checking of the object should be done in poll. I’m not sure and would have to do some research to find out. I’m here for easy stuff right now. I think we should move that checking up to the top, above the setting of the variables.
I decide to rearrange the code a bit and put in some spacing. Spacing is always a hint that there are some methods waiting to be found. Think of paragraph spacing: if we want it set off in a paragraph it should be a separate idea.
def execute(self, context):
activate_object_by_name('ruler')
obj = bpy.context.object
if obj is None or obj.type != "MESH" or 'ruler' not in obj.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
column_spacing = context.scene.rcg_settings.column_spacing
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
fins = obj.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
every_nth_pair = pos_up_pairs[::column_spacing]
offset_desired = context.scene.rcg_settings.offset_distance
column_diameter = context.scene.rcg_settings.column_diameter
root_collection = self.set_rcg_collection_active() # set to our collection
for pair in every_nth_pair:
self.place_support(pair, column_diameter, offset_desired)
bpy.context.view_layer.active_layer_collection = root_collection # reset collection
return {'FINISHED'}
I noticed that the three parameter values are used in two different places, and I thought I’d see if putting them closer to use made for opportunities. I think that all that second paragraph is doing is providing the every_nth_pair collection, so:
I do it but it isn’t right so I undo it. The transform application should be separate from finding the support positions, so I move it up and do again.
def execute(self, context):
activate_object_by_name('ruler')
obj = bpy.context.object
if obj is None or obj.type != "MESH" or 'ruler' not in obj.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, obj)
offset_desired = context.scene.rcg_settings.offset_distance
column_diameter = context.scene.rcg_settings.column_diameter
root_collection = self.set_rcg_collection_active() # set to our collection
for pair in every_nth_pair:
self.place_support(pair, column_diameter, offset_desired)
bpy.context.view_layer.active_layer_collection = root_collection # reset collection
return {'FINISHED'}
def get_support_positions(self, context, obj):
column_spacing = context.scene.rcg_settings.column_spacing
fins = obj.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
every_nth_pair = pos_up_pairs[::column_spacing]
return every_nth_pair
At this point I notice that obj is a terrible name. It’s the ruler. Rename in both execute and get_support_positions. I could have undone the extract, done the rename, redo the extract but d the rename twice instead.
Now the last part of execute can be a method. It’s placing all the supports.
def execute(self, context):
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
def place_supports(self, every_nth_pair, context):
offset_desired = context.scene.rcg_settings.offset_distance
column_diameter = context.scene.rcg_settings.column_diameter
root_collection = self.set_rcg_collection_active() # set to our collection
for pair in every_nth_pair:
self.place_support(pair, column_diameter, offset_desired)
bpy.context.view_layer.active_layer_collection = root_collection # reset collection
def get_support_positions(self, context, ruler):
column_spacing = context.scene.rcg_settings.column_spacing
fins = ruler.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
every_nth_pair = pos_up_pairs[::column_spacing]
return every_nth_pair
def place_support(self, pos_up_pair, diameter, offset_desired):
pos_vec = self.get_position_vector(offset_desired, pos_up_pair)
self.add_support(pos_vec, diameter)
def add_support(self, pos_vec, diameter):
z_size = pos_vec.z
bpy.ops.mesh.primitive_cylinder_add(
location=(pos_vec.x, pos_vec.y, pos_vec.z - z_size / 2),
vertices=6,
radius=diameter / 2.0,
depth=z_size,
end_fill_type='NOTHING',
enter_editmode=False)
ob = bpy.context.object
ob.name = 'Support'
bpy.ops.object.shade_smooth()
Are we done yet? No, but I do want to test, because we’ve done a lot. I cross my fingers, pop into Blender, add some supports. They work. Woot. Commit: refactoring addSupport
Now we need to ask ourselves whether this is better. We have taken one long method and turned it into four methods, and they are still kind of long, 7, 8, 3, and 12 lines. However, each one says what it does and only does one thing. And the top level method, execute, says pretty clearly what it’s about.
I do think it could be better. It’s conceivable that we should make a one-line method for applying transforms, because “the rule” is that a composed method should only call other methods in the class, but the rule is a bit too strict for everyday use. More of a guideline than an actual rule …
The first bit is trying to be a guard clause, checking whether we can proceed. Since it is bigger than the code it guards, we could use some improvement. Instead of having it do the cancel, let’s have it set a flag and then we’ll do the cancel with an if:
def execute(self, context):
have_valid_ruler = True
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
have_valid_ruler = False
if not have_valid_ruler:
return {'CANCELLED'}
else:
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
Hate the not. Invert and rename the flag:
def execute(self, context):
invalid_ruler = False
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
invalid_ruler = True
if invalid_ruler:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
else:
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
Don’t like that either. Back to the other and invert the if.
def execute(self, context):
ruler_is_present = True
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
ruler_is_present = False
if ruler_is_present:
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
else:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
That’s pretty good. Now extract the first paragraph. No, don’t like that either, because we create the ruler. Revert and do again.
def execute(self, context):
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
I’m starting to like this better than what I’ve thought of so far. Let’s try this:
def execute(self, context):
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
ruler = None
if ruler:
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
else:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
Now extract:
def execute(self, context):
ruler = self.get_valid_ruler()
if ruler:
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
else:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
def get_valid_ruler(self):
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
ruler = None
return ruler
No. That’s longer and not better. Revert.
def execute(self, context):
activate_object_by_name('ruler')
ruler = bpy.context.object
if ruler is None or ruler.type != "MESH" or 'ruler' not in ruler.name:
self.report({'ERROR'}, 'You seem to have no ruler.')
return {'CANCELLED'}
bpy.ops.object.transform_apply(location=True, rotation=True, scale=True)
every_nth_pair = self.get_support_positions(context, ruler)
self.place_supports(every_nth_pair, context)
return {'FINISHED'}
I do not love that, but I love everything I’ve tried even less. Let’s move one. Quick check of the other methods and then wrap up.
def get_support_positions(self, context, ruler):
column_spacing = context.scene.rcg_settings.column_spacing
fins = ruler.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
every_nth_pair = pos_up_pairs[::column_spacing]
return every_nth_pair
We are down to tiny matters now. Move column_spacing down closer to use, so that we don’t have to keep it in our head:
def get_support_positions(self, context, ruler):
fins = ruler.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
verts = vertices.values()
pos_up_pairs = make_pairs(verts) # tested in test_file_writing.py
column_spacing = context.scene.rcg_settings.column_spacing
every_nth_pair = pos_up_pairs[::column_spacing]
return every_nth_pair
The untangling of the vertices stuff is odd. What is data.vertices anyway? It’s a blender collection type, so I just wanted to deal with the values. I think that we could have worked with the collection that cam out of there just as well. Shall we try it? We shall, and it works. So:
def get_support_positions(self, context, ruler):
fins = ruler.evaluated_get(bpy.context.view_layer.depsgraph)
vertices = fins.data.vertices
pos_up_pairs = make_pairs(vertices) # tested in test_file_writing.py
column_spacing = context.scene.rcg_settings.column_spacing
every_nth_pair = pos_up_pairs[::column_spacing]
return every_nth_pair
All that said, such a tiny change, removing that call to values(). But the code isn’t quite so ocnfusing now, without the call to values(). We’ll commit one more time and wrap up.
OK, the only semi-useful thing we did here was to remove the unused code for the old imports. We still have it in git, so if we ever needed it, we could get it back. It does have some historical value, as it shows how to define and set a parameter, but we have other code that does that, including the newer import from file.
Then we did some cleanup on the add supports class. Some of that was, I think, worth doing, and some … not so much. And tastes will vary: I prefer shorter one-purpose methods. Other programmers prefer to see everything laid out in a big method. We’ll agree to disagree, I guess, but this is more like what I like.
But it’s not great even so. We may pass this way again, or we may not. For now, good enough.