Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rewrite] Rewrite variable names for easier contribution #478

Open
MagicBOTAlex opened this issue Aug 28, 2024 · 2 comments
Open

[Rewrite] Rewrite variable names for easier contribution #478

MagicBOTAlex opened this issue Aug 28, 2024 · 2 comments

Comments

@MagicBOTAlex
Copy link
Contributor

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

  • Easier to understand without comments
  • Easier to read (atleast for me)
  • If it's easier for others, then there would probably be more contributors

Cons

  • Is more verbose
  • Takes time to rewrite
  • Takes more time to implement new features
@hlorus
Copy link
Owner

hlorus commented Sep 2, 2024

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants