blender/blender-addons
19
44
Fork 96
Code Issues 632 Pull Requests 28 Projects 1 Releases Wiki Activity

3D Print Toolbox: Add hollow out #105194

Merged
Mikhail Rachinskiy merged 9 commits from usfreitas/blender-addons:hollow into main 2024-03-18 12:24:30 +01:00
Conversation 6 Commits 9 Files Changed 3 +21 -9
1 changed files with 21 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c3053a2167 - Show all commits

30
object_print3d_utils/operators.py
View File

@ -833,12 +833,12 @@ class MESH_OT_print3d_hollow(Operator):
offset_direction: EnumProperty( offset_direction: EnumProperty(
items=[ items=[
('inside', "Inside", "Offset surface inside of object"), ('INSIDE', "Inside", "Offset surface inside of object"),
('outside', "Outside", "Offset surface outside of object"), ('OUTSIDE', "Outside", "Offset surface outside of object"),
], ],
name="Offset Direction", name="Offset Direction",
description="Where the offset surface is created relative to the object", description="Where the offset surface is created relative to the object",
default='inside', default='INSIDE',
) )
MikhailRachinskiy marked this conversation as resolved
Mikhail Rachinskiy commented 2024-03-17 06:07:02 +01:00
Review
Copy Link

Enum values are supposed to be in UPPERCASE

I noticed inconsistent use of double vs single quotation marks, Blender code guidelines recommends: single quotes when using enum values, double quotes everything else.

Enum values are supposed to be in `UPPERCASE` --- I noticed inconsistent use of double vs single quotation marks, Blender code guidelines recommends: single quotes when using enum values, double quotes everything else.
usfreitas commented 2024-03-17 08:00:31 +01:00
Review
Copy Link

Sorry, my bad. It should be fixed now.

Sorry, my bad. It should be fixed now.
offset: FloatProperty( offset: FloatProperty(
name="Offset", name="Offset",
MikhailRachinskiy marked this conversation as resolved
Mikhail Rachinskiy commented 2024-03-14 07:48:56 +01:00
Review
Copy Link

default=1

default=1
@ -861,6 +861,18 @@ class MESH_OT_print3d_hollow(Operator):
description="Create hollowed out copy of the object", description="Create hollowed out copy of the object",
) )
def draw(self, context):
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 15:04:44 +01:00
Outdated
Review
Copy Link

Code style

Here m = obj.data # mesh is unnecessary abbreviation and redundant comment:
mesh = obj.data or me = obj.data would be more appropriate.

Getting Mesh

obj.data will give you base mesh datablock without modifiers and object deformations. In this case we need modifiers and deformations, so instead of base mesh we should get evaluated mesh and apply object transforms:

depsgraph = bpy.context.evaluated_depsgraph_get()

ob_eval = ob.evaluated_get(depsgraph)
me = ob_eval.to_mesh()
me.transform(ob.matrix_world)

# Stuff happens

ob_eval.to_mesh_clear()

If we do not apply object transforms to mesh and instead just scale object itself, then offset will also be scaled (which is what currently happens).

**Code style** Here `m = obj.data # mesh` is unnecessary abbreviation and redundant comment: `mesh = obj.data` or `me = obj.data` would be more appropriate. **Getting Mesh** `obj.data` will give you base mesh datablock without modifiers and object deformations. In this case we need modifiers and deformations, so instead of base mesh we should get evaluated mesh and apply object transforms: ``` depsgraph = bpy.context.evaluated_depsgraph_get() ob_eval = ob.evaluated_get(depsgraph) me = ob_eval.to_mesh() me.transform(ob.matrix_world) # Stuff happens ob_eval.to_mesh_clear() ``` If we do not apply object transforms to mesh and instead just scale object itself, then offset will also be scaled (which is what currently happens).
layout = self.layout
layout.use_property_split = True
layout.use_property_decorate = False
layout.separator()
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 17:14:02 +01:00
Outdated
Review
Copy Link

Whitespace missing around multiplication operator, same thing below with division.

Whitespace missing around multiplication operator, same thing below with division.
layout.prop(self, "offset_direction", expand=True)
layout.prop(self, "offset")
layout.prop(self, "voxel_size")
layout.prop(self, "make_hollow_duplicate")
def execute(self, context): def execute(self, context):
import numpy as np import numpy as np
import pyopenvdb as vdb import pyopenvdb as vdb
@ -883,9 +895,9 @@ class MESH_OT_print3d_hollow(Operator):
ntris = len(mesh_target.loop_triangles) ntris = len(mesh_target.loop_triangles)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 19:46:19 +01:00
Outdated
Review
Copy Link

It's better to avoid negation, if possible, it just makes harder to follow the logic.

if self.create_hollow:
    ...
else:
    ...
It's better to avoid negation, if possible, it just makes harder to follow the logic. ```python if self.create_hollow: ... else: ... ```
verts = np.zeros(3 * nverts, dtype=np.float32) verts = np.zeros(3 * nverts, dtype=np.float32)
tris = np.zeros(3 * ntris, dtype=np.int32) tris = np.zeros(3 * ntris, dtype=np.int32)
mesh_target.vertices.foreach_get('co', verts) mesh_target.vertices.foreach_get("co", verts)
verts.shape = (-1, 3) verts.shape = (-1, 3)
mesh_target.loop_triangles.foreach_get('vertices', tris) mesh_target.loop_triangles.foreach_get("vertices", tris)
tris.shape = (-1, 3) tris.shape = (-1, 3)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 19:56:57 +01:00
Outdated
Review
Copy Link

These comments are redundant, it's obvious why you flip normals, why you move new object to a location of current object, or why you remove mesh (you could name it mesh_temp to emphasize intent).

These comments are redundant, it's obvious why you flip normals, why you move new object to a location of current object, or why you remove mesh (you could name it `mesh_temp` to emphasize intent).
# Generate VDB levelset # Generate VDB levelset
@ -895,7 +907,7 @@ class MESH_OT_print3d_hollow(Operator):
levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris, transform=trans, halfWidth=half_width) levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris, transform=trans, halfWidth=half_width)
# Generate offset surface # Generate offset surface
if self.offset_direction == 'inside': if self.offset_direction == 'INSIDE':
newverts, newquads = levelset.convertToQuads(-self.offset) newverts, newquads = levelset.convertToQuads(-self.offset)
else: else:
newverts, newquads = levelset.convertToQuads(self.offset) newverts, newquads = levelset.convertToQuads(self.offset)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 20:05:22 +01:00
Outdated
Review
Copy Link

Again, you don't need to comment on every line, there is a check above if create_hollow and you are using bpy.data.objects.new right after it, the code is self-explanatory.

Same with offset check.

Again, you don't need to comment on every line, there is a check above `if create_hollow` and you are using `bpy.data.objects.new` right after it, the code is self-explanatory. Same with offset check.
@ -905,12 +917,12 @@ class MESH_OT_print3d_hollow(Operator):
# Instantiate new object in Blender # Instantiate new object in Blender
bpy.ops.object.mode_set(mode='OBJECT') bpy.ops.object.mode_set(mode='OBJECT')
bpy.ops.object.select_all(action='DESELECT') bpy.ops.object.select_all(action='DESELECT')
mesh_offset = bpy.data.meshes.new(mesh_target.name + ' offset') mesh_offset = bpy.data.meshes.new(mesh_target.name + " offset")
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 20:15:24 +01:00
Outdated
Review
Copy Link

Part of this code should be outside if block, currently selection changes only when hollow duplicate is created. It should change regardless of that, new object is added to the scene - it should be selected and active.

Part of this code should be outside `if` block, currently selection changes only when hollow duplicate is created. It should change regardless of that, new object is added to the scene - it should be selected and active.
mesh_offset.from_pydata(newverts, [], polys) mesh_offset.from_pydata(newverts, [], polys)
# For some reason OpenVDB has inverted normals # For some reason OpenVDB has inverted normals
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 20:10:22 +01:00
Outdated
Review
Copy Link

If you find matrix stuff confusing, then you could write it as:

obj_hollow.location = obj.matrix_world.translation

It actually could be like this, but there is a chance that object could have a parent, so using matrix_world ensures we have visible location.

obj_hollow.location = obj.location
If you find matrix stuff confusing, then you could write it as: ```python obj_hollow.location = obj.matrix_world.translation ``` It actually could be like this, but there is a chance that object could have a parent, so using matrix_world ensures we have visible location. ```python obj_hollow.location = obj.location ```
usfreitas commented 2024-03-12 22:14:12 +01:00
Outdated
Review
Copy Link

I'm actually pretty comfortable around matrices. I had a good training in linear algebra, and I've been using NumPy way longer than Blender.

I was looking for something like obj_hollow.matrix_world[:3, 3] = ... and had not realized that mathutils.Matrix.translation is writable until you suggested it. That code line, as it is now, makes perfect sense for me, and I would not have added the comment above it if I were the only one touching this add-on.

However, I have very little experience in programing with other people. As I was afraid that what I found easy to understand might not be that for the next one who needs to work in this code, I went trigger happy with the comments 😅

I'll remove this and the other redundant comments.

I'm actually pretty comfortable around matrices. I had a good training in linear algebra, and I've been using NumPy way longer than Blender. I was looking for something like `obj_hollow.matrix_world[:3, 3] = ...` and had not realized that `mathutils.Matrix.translation` is writable until you suggested it. That code line, as it is now, makes perfect sense for me, and I would not have added the comment above it if I were the only one touching this add-on. However, I have very little experience in programing with other people. As I was afraid that what I found easy to understand might not be that for the next one who needs to work in this code, I went trigger happy with the comments 😅 I'll remove this and the other redundant comments.
mesh_offset.flip_normals() mesh_offset.flip_normals()
obj_offset = bpy.data.objects.new(obj.name + ' offset', mesh_offset) obj_offset = bpy.data.objects.new(obj.name + " offset", mesh_offset)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 21:10:11 +01:00
Outdated
Review
Copy Link

Double newline only allowed in-between class and function definitions, everything inside function body should be separated by 1 line max.

Double newline only allowed in-between class and function definitions, everything inside function body should be separated by 1 line max.
obj_offset.matrix_world.translation = obj.matrix_world.translation obj_offset.matrix_world.translation = obj.matrix_world.translation
bpy.context.collection.objects.link(obj_offset) bpy.context.collection.objects.link(obj_offset)
obj_offset.select_set(True) obj_offset.select_set(True)
@ -921,7 +933,7 @@ class MESH_OT_print3d_hollow(Operator):
bpy.context.collection.objects.link(obj_hollow) bpy.context.collection.objects.link(obj_hollow)
obj_hollow.matrix_world.translation = obj.matrix_world.translation obj_hollow.matrix_world.translation = obj.matrix_world.translation
obj_hollow.select_set(True) obj_hollow.select_set(True)
if self.offset_direction == 'inside': if self.offset_direction == 'INSIDE':
mesh_offset.flip_normals() mesh_offset.flip_normals()
else: else:
mesh_target.flip_normals() mesh_target.flip_normals()