blender/blender-addons
19
43
Fork 96
Code Issues 630 Pull Requests 27 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 +14 -39
2 changed files with 14 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0e9582d3b7 - Show all commits

17
object_print3d_utils/__init__.py
View File

@ -129,23 +129,6 @@ class SceneProperties(PropertyGroup):
min=0.0,
step=1,
)
hollow_voxel_size: FloatProperty(
name="Voxel size",
description="Size of the voxel used for volume evaluation. Lower values preserve finer details",
default=1.0,
min=0.0001,
step=1,
subtype='DISTANCE',
)
make_hollow_duplicate: BoolProperty(
name="Hollow Duplicate",
description="Create hollowed out copy of the object",
)
make_hollow_inside: BoolProperty(
name="Inside",
description="Offset surface inside of the object",
default=True,
)
classes = (

36
object_print3d_utils/operators.py
View File

@ -13,6 +13,7 @@ from bpy.props import (
IntProperty,
FloatProperty,
BoolProperty,
EnumProperty,
)
import bmesh
@ -828,8 +829,17 @@ class MESH_OT_print3d_hollow(Operator):
bl_idname = "mesh.print3d_hollow"
bl_label = "Hollow"
bl_description = "Create offset surface"
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 18:26:51 +01:00
Outdated
Review
Copy Link
  • Operator should be called Hollow, it is short and widely accepted terminology.
  • Description should be shorter as well: Create offset surface it describes exactly what the tool does.
- Operator should be called `Hollow`, it is short and widely accepted terminology. - Description should be shorter as well: `Create offset surface` it describes exactly what the tool does.
bl_options = {'REGISTER', 'UNDO'}
bl_options = {'REGISTER', 'UNDO', 'PRESET'}
offset_direction: EnumProperty(
items=[
('inside', "Inside", "Offset surface inside of object"),
('outside', "Outside", "Offset surface outside of object"),
],
name="Offset Direction",
description="Where the offset surface is created relative to the object",
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(
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
description="Surface offset in relation to original mesh",
@ -841,7 +851,7 @@ class MESH_OT_print3d_hollow(Operator):
voxel_size: FloatProperty(
name="Voxel size",
description="Size of the voxel used for volume evaluation. Lower values preserve finer details",
default=5.0,
default=1.0,
min=0.0001,
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 17:21:13 +01:00
Outdated
Review
Copy Link

Ther should be "gate" conditions at the beginning of the execute method checking for invalid values.

if not self.offset:
    return {'FINISHED'}
Ther should be "gate" conditions at the beginning of the execute method checking for invalid values. ``` if not self.offset: return {'FINISHED'} ```
step=1,
subtype='DISTANCE',
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-03-14 07:44:37 +01:00
Outdated
Review
Copy Link

I think that simple checkbox called inside is not user friendly, it is fine with on/off options, but with something more complex it's better to make all available options visible right away (even when there is only two options).

Expanded enum called Offset Direction with values Inside and Outside fits better here.

Also, it is better to group related options together, so place Offset Direction right before Offset option.

I think that simple checkbox called inside is not user friendly, it is fine with on/off options, but with something more complex it's better to make all available options visible right away (even when there is only two options). Expanded enum called `Offset Direction` with values `Inside` and `Outside` fits better here. Also, it is better to group related options together, so place `Offset Direction` right before `Offset` option.
Mikhail Rachinskiy commented 2024-03-17 06:03:22 +01:00
Outdated
Review
Copy Link

You did not expand enum property so it currently shows as dropdown list.

Add draw method to the operator, right before execute method and manually compose UI:

def draw(self, context):
    layout = self.layout
    layout.use_property_split = True
    layout.use_property_decorate = False

    layout.prop(self, "offset_direction", expand=True)
    layout.prop(self, "offset")
    layout.prop(self, "voxel_size")
    layout.prop(self, "make_hollow_duplicate")

If you want to keep presets, then throw layout.separator() in there, to keep it visually separated:

def draw(self, context):
    ...
    layout.use_property_decorate = False

    layout.separator()

    layout.prop(self, "offset_direction", expand=True)
    ...
You did not expand enum property so it currently shows as dropdown list. Add draw method to the operator, right before `execute` method and manually compose UI: ```python def draw(self, context): layout = self.layout layout.use_property_split = True layout.use_property_decorate = False layout.prop(self, "offset_direction", expand=True) layout.prop(self, "offset") layout.prop(self, "voxel_size") layout.prop(self, "make_hollow_duplicate") ``` If you want to keep presets, then throw `layout.separator()` in there, to keep it visually separated: ```python def draw(self, context): ... layout.use_property_decorate = False layout.separator() layout.prop(self, "offset_direction", expand=True) ... ```
usfreitas commented 2024-03-17 08:03:02 +01:00
Outdated
Review
Copy Link

I did not understand what you meant by "expand". Now I know, and it looks really better. The separator also helps.

I did not understand what you meant by "expand". Now I know, and it looks _really_ better. The separator also helps.
@ -850,11 +860,6 @@ class MESH_OT_print3d_hollow(Operator):
name="Hollow Duplicate",
description="Create hollowed out copy of the object",
)
make_hollow_inside: BoolProperty(
name="Inside",
description="Offset surface inside of the object",
default=True,
)
def execute(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).
import numpy as np
@ -890,7 +895,7 @@ class MESH_OT_print3d_hollow(Operator):
levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris, transform=trans, halfWidth=half_width)
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: ... ```
# Generate offset surface
if self.make_hollow_inside:
if self.offset_direction == 'inside':
newverts, newquads = levelset.convertToQuads(-self.offset)
else:
newverts, newquads = levelset.convertToQuads(self.offset)
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).
@ -916,7 +921,7 @@ class MESH_OT_print3d_hollow(Operator):
bpy.context.collection.objects.link(obj_hollow)
obj_hollow.matrix_world.translation = obj.matrix_world.translation
obj_hollow.select_set(True)
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.
if self.make_hollow_inside:
if self.offset_direction == 'inside':
mesh_offset.flip_normals()
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.
else:
mesh_target.flip_normals()
@ -925,23 +930,10 @@ class MESH_OT_print3d_hollow(Operator):
else:
bpy.data.meshes.remove(mesh_target)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-03-14 07:31:53 +01:00
Outdated
Review
Copy Link

The scene settings have to go, it's hidden behavior and is really convoluted without clear benefit, and for reusing settings it is better to use presets.
Just add PRESET value to bl_options.

bl_options = {'REGISTER', 'UNDO', 'PRESET'}
The scene settings have to go, it's hidden behavior and is really convoluted without clear benefit, and for reusing settings it is better to use presets. Just add `PRESET` value to `bl_options`. ```python bl_options = {'REGISTER', 'UNDO', 'PRESET'} ```
usfreitas commented 2024-03-16 19:22:00 +01:00
Outdated
Review
Copy Link

I'm sorry about you being sick. I hope when you will be already fully recovered when you read this.

This time I implemented all suggestions. At least I hope. Last time a wrong default flew by... 😅

I'm not very happy about the preset. I think it cluttered a bit an interface that was otherwise very neat. But I think it is better to let that in and give the user the option to customize the operator.

Thanks again! This is fun :D

I'm sorry about you being sick. I hope when you will be already fully recovered when you read this. This time I implemented all suggestions. At least I hope. Last time a wrong default flew by... 😅 I'm not very happy about the preset. I think it cluttered a bit an interface that was otherwise very neat. But I think it is better to let that in and give the user the option to customize the operator. Thanks again! This is fun :D
Mikhail Rachinskiy commented 2024-03-17 05:33:46 +01:00
Outdated
Review
Copy Link

Adding presets was just a suggestion, the operator settings are quite minimal so presets not necessary, you can remove it if you want.

Adding presets was just a suggestion, the operator settings are quite minimal so presets not necessary, you can remove it if you want.
usfreitas commented 2024-03-17 08:08:17 +01:00
Outdated
Review
Copy Link

With the separator it looks definitely better.

I think the preset is useful because of my points about the default voxel size. If a user has a setup where this default isn't appropriate, having the possibility to customize it is a plus. I wish I could do that with the remesh modifier.

With the separator it looks definitely better. I think the preset is useful because of my points about the default voxel size. If a user has a setup where this default isn't appropriate, having the possibility to customize it is a plus. I wish I could do that with the remesh modifier.
print_3d = context.scene.print_3d
print_3d.hollow_offset = self.offset
print_3d.hollow_voxel_size = self.voxel_size
print_3d.make_hollow_duplicate = self.make_hollow_duplicate
print_3d.make_hollow_inside = self.make_hollow_inside
return {'FINISHED'}
def invoke(self, context, event):
print_3d = context.scene.print_3d
self.offset = print_3d.hollow_offset
self.voxel_size = print_3d.hollow_voxel_size
self.make_hollow_duplicate = print_3d.make_hollow_duplicate
self.make_hollow_inside = print_3d.make_hollow_inside
if context.mode == 'EDIT_MESH':
bpy.ops.object.mode_set(mode='OBJECT')
wm = context.window_manager
return wm.invoke_props_dialog(self)