hlorus / CAD_Sketcher

Constraint-based geometry sketcher for blender
GNU General Public License v3.0
2.67k stars 128 forks source link

[Rewrite] Rewrite variable names for easier contribution #478

Open MagicBOTAlex opened 2 weeks ago

MagicBOTAlex commented 2 weeks ago

Desc

Maybe this is a personal issue, but I found it quite hard to understand, and implement items to the codebase, since many of the variables were quite shortened, with no comments at all. If it's easier for people to participate, then there would likely be more contributors.

It took me multiple hours before understanding this small example code snippet. Now that i understand it, i can easily make changes to it. (I am currently trying to implement another feature.)

In short, it's hard to understand and thus less people would participate.

Example

Original

def main(self, context: Context):
        sse = context.scene.sketcher.entities

        ob_name, face_index = self.get_state_pointer(index=0, implicit=True)
        ob = get_evaluated_obj(context, bpy.data.objects[ob_name])
        mesh = ob.data
        face = mesh.polygons[face_index]

        mat_obj = ob.matrix_world
        quat = get_face_orientation(mesh, face)
        quat.rotate(mat_obj)
        pos = mat_obj @ face.center
        origin = sse.add_point_3d(pos)
        nm = sse.add_normal_3d(quat)

        self.target = sse.add_workplane(origin, nm)
        ignore_hover(self.target)
        context.area.tag_redraw() # Force re-draw of UI (Blender doesn't update after tool usage)
        return True

After

def main(self, context: Context):
        sketcherEntities = context.scene.sketcher.entities

        obj_name, clicked_face_index = self.get_state_pointer(index=0, implicit=True)
        clicked_obj = get_evaluated_obj(context, bpy.data.objects[obj_name])
        clicked_mesh = clicked_obj.data
        clicked_face = clicked_mesh.polygons[clicked_face_index]

        obj_translation = clicked_obj.matrix_world
        quat = get_face_orientation(clicked_mesh, clicked_face) # Quternion
        quat.rotate(obj_translation)

        workplane_position = obj_translation @ clicked_face.center
        sketch_centrum_point= sketcherEntities.add_point_3d(workplane_position)
        workplane_normal = sketcherEntities.add_normal_3d(quat)

        self.target = sse.add_workplane(sketch_centrum_point, workplane_normal )
        ignore_hover(self.target)
        context.area.tag_redraw() # Force re-draw of UI (Blender doesn't update after tool usage)
        return True

Pros

Cons

hlorus commented 1 week ago

IMO in places where the scope is as small as in your example and variables aren't prone to be confused with each other there's not really a benefit in such verbose naming. I agree it's good for bigger scopes but i'd still argue that it's not worth doing a rewrite. Maybe there could be a chapter in the documentation regarding variable naming and best practices. Potentially a glossar for frequently used abbreviations would already go a long way.

MagicBOTAlex commented 1 week ago

ooh, good solution I'd say. And I see your point, just thought I'd suggest it ¯_(ツ)_/¯