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 +83 -72
3 changed files with 83 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f9f897a6d7 - Show all commits

37
object_print3d_utils/__init__.py
View File

@ -122,22 +122,29 @@ class SceneProperties(PropertyGroup):
max=math.radians(90.0), max=math.radians(90.0),
) )
hollow_offset: FloatProperty( hollow_offset: FloatProperty(
name = "Offset", name="Offset",
description = "Surface offset in relation to original mesh. Negative -> inwards", description="Surface offset in relation to original mesh",
default = -5.0, default=1.0,
subtype = 'DISTANCE', subtype='DISTANCE',
min=0.0,
step=1,
) )
hollow_resolution: FloatProperty( hollow_voxel_size: FloatProperty(
name = "Resolution", name="Voxel size",
description = "Resolution of the voxel grid used for hollowing", description="Size of the voxel used for volume evaluation. Lower values preserve finer details",
default = 5.0, default=1.0,
min = 0.01, min=0.0001,
subtype = 'DISTANCE', step=1,
subtype='DISTANCE',
) )
hollow_create: BoolProperty( make_hollow_duplicate: BoolProperty(
name = "Create Hollow Copy", name="Hollow Duplicate",
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 18:14:30 +01:00
Outdated
Review
Copy Link

Duplicating operator settings in the scene is redundant, it is much better to invoke popup dialog with operator settings:

def invoke(self, context, event):
    wm = context.window_manager
    return wm.invoke_props_dialog(self)

If you want to let user choose defaults, then add-on preferences should be used instead.

Duplicating operator settings in the scene is redundant, it is much better to invoke popup dialog with operator settings: ```python def invoke(self, context, event): wm = context.window_manager return wm.invoke_props_dialog(self) ``` If you want to let user choose defaults, then add-on preferences should be used instead.
usfreitas commented 2024-03-12 20:58:48 +01:00
Outdated
Review
Copy Link

This is cool! I did not know about it. I much prefer the dialog, as the operator UI can then be just a button.

Duplication of the operator settings is a pattern that occur often in the 3D-Print Toolbox. I actually learned it by reading the add-on code. The advantage I see with it is that the last used values get saved along with the Blender file when the user saves it. This gives a set of defaults adapted to the objects. If I understand correctly, add-on preferences are global, so it is not possible to have a per file set of add-on preferences. I also like the fact the user does not need to do anything to save the defaults in the duplicated settings case.

My option would be to use the popup dialog and keep the duplicated settings in the scene. If the duplicated setting are a no go, I would just use hard coded defaults. I don't think add-on preferences are worth it here.

This is cool! I did not know about it. I much prefer the dialog, as the operator UI can then be just a button. Duplication of the operator settings is a pattern that occur often in the 3D-Print Toolbox. I actually learned it by reading the add-on code. The advantage I see with it is that the last used values get saved along with the Blender file when the user saves it. This gives a set of defaults adapted to the objects. If I understand correctly, add-on preferences are global, so it is not possible to have a per file set of add-on preferences. I also like the fact the user does not need to do anything to save the defaults in the duplicated settings case. My option would be to use the popup dialog and keep the duplicated settings in the scene. If the duplicated setting are a no go, I would just use hard coded defaults. I don't think add-on preferences are worth it here.
description = "Create a hollow copy of the target object, with applied modifiers and scale", description="Create hollowed out copy of the object",
default = False )
make_hollow_inside: BoolProperty(
name="Inside",
description="Offset surface inside of the object",
default=True,
) )
@ -146,7 +153,7 @@ classes = (
ui.VIEW3D_PT_print3d_analyze, ui.VIEW3D_PT_print3d_analyze,
ui.VIEW3D_PT_print3d_cleanup, ui.VIEW3D_PT_print3d_cleanup,
ui.VIEW3D_PT_print3d_transform, ui.VIEW3D_PT_print3d_edit,
ui.VIEW3D_PT_print3d_export, ui.VIEW3D_PT_print3d_export,
operators.MESH_OT_print3d_info_volume, operators.MESH_OT_print3d_info_volume,

107
object_print3d_utils/operators.py
View File

@ -826,27 +826,34 @@ class MESH_OT_print3d_export(Operator):
class MESH_OT_print3d_hollow(Operator): class MESH_OT_print3d_hollow(Operator):
bl_idname = "mesh.print3d_hollow" bl_idname = "mesh.print3d_hollow"
bl_label = "Create offset surface" bl_label = "Hollow"
bl_description = "Create offset surface for hollowing objects" bl_description = "Create offset surface"
bl_options = {'REGISTER', 'UNDO'} bl_options = {'REGISTER', 'UNDO'}
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.
offset: FloatProperty( offset: FloatProperty(
name = "Offset", name="Offset",
description = "Surface offset in relation to original mesh. Negative -> inwards", description="Surface offset in relation to original mesh",
default = -5.0, default=1.0,
subtype = 'DISTANCE', subtype='DISTANCE',
min=0.0,
step=1,
) )
resolution: FloatProperty( voxel_size: FloatProperty(
name = "Resolution", name="Voxel size",
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.
description = "Resolution of the voxel grid used for hollowing", description="Size of the voxel used for volume evaluation. Lower values preserve finer details",
default = 5.0, default=5.0,
MikhailRachinskiy marked this conversation as resolved
Mikhail Rachinskiy commented 2024-03-14 07:48:56 +01:00
Review
Copy Link

default=1

default=1
min = 0.01, min=0.0001,
subtype = 'DISTANCE', step=1,
subtype='DISTANCE',
) )
create_hollow: BoolProperty( make_hollow_duplicate: BoolProperty(
name = "Create Hollow Copy", name="Hollow Duplicate",
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 19:41:44 +01:00
Outdated
Review
Copy Link

Code style

  • There should be no whitespace around = operator for arguments in function/object call.

Offset

  • I don't think we should use negative as default here, similar to Solidify modifier the number describes wall thickness and negative numbers just don't seem right. So, let's just flip the sign here.
  • Don't mention offset direction in description, after sign flip it will work intuitively.
  • Default offset of 5 is too much, let's do 1, it's closer to minimum thickness for 3D printed stuff.
  • Let's define float property step attribute step=1, it just makes slider drag a bit more pleasant.

Resolution

  • The name is ambiguous, let's use Voxel Size it is already used across Blender and is much more descriptive.
  • Description: just take the description of Voxel Size property from remesh modifier but remove the mention of object space.
  • Default value is too big, let's get closer to default of remesh modifier, somewhere between 0.1 and 0.5. I usually do it with 0.3 when I don't need the detail.
  • min is too big, both sculpt remesh and modifier have it at 0.0001
  • step=1

Hollow Copy

  • Boolean property name (as in variable name in code) should use certain name prefixes as a convention: use_, show_, make_, etc., whichever is more applicable in given context make_hollow_duplicate
  • Property UI name should be just Hollow Copy or Hollow Duplicate, I think duplicate is better, because there is already tool called duplicate, but copy is also fine.
  • Description: Create hollowed out copy of the object, I don't think we need to clarify scale and modifiers part, it's kind of obvious given the nature of the tool.
  • default = False default of boolean property is false, you don't need to specify it.
## Code style - There should be no whitespace around `=` operator for arguments in function/object call. ## Offset - I don't think we should use negative as default here, similar to Solidify modifier the number describes wall thickness and negative numbers just don't seem right. So, let's just flip the sign here. - Don't mention offset direction in description, after sign flip it will work intuitively. - Default offset of `5` is too much, let's do `1`, it's closer to minimum thickness for 3D printed stuff. - Let's define float property step attribute `step=1`, it just makes slider drag a bit more pleasant. ## Resolution - The name is ambiguous, let's use `Voxel Size` it is already used across Blender and is much more descriptive. - Description: just take the description of Voxel Size property from remesh modifier but remove the mention of object space. - Default value is too big, let's get closer to default of remesh modifier, somewhere between `0.1` and `0.5`. I usually do it with `0.3` when I don't need the detail. - `min` is too big, both sculpt remesh and modifier have it at `0.0001` - `step=1` ## Hollow Copy - Boolean property name (as in variable name in code) should use certain name prefixes as a convention: `use_`, `show_`, `make_`, etc., whichever is more applicable in given context `make_hollow_duplicate` - Property UI name should be just `Hollow Copy` or `Hollow Duplicate`, I think duplicate is better, because there is already tool called duplicate, but copy is also fine. - Description: `Create hollowed out copy of the object`, I don't think we need to clarify scale and modifiers part, it's kind of obvious given the nature of the tool. - `default = False` default of boolean property is false, you don't need to specify it.
usfreitas commented 2024-02-28 22:40:02 +01:00
Outdated
Review
Copy Link

Thanks again for your feedback. As for the last time, I agree with most points. I have, however, observations.

Offset

I was rather too liberal with comments in the code, but I think I still managed not to explain an important point.
There are two distinct use cases for this tool. One is the standard hollowing out, where the offset surface is on the inside of the the target object. The other use case is when the offset surface is on the outside of the target. Its application is to generate a thin walled mesh that, after printing, can be used as a mold for the target object to cast other materials in the target form. If the mold is one-shot, that is, it is discarded or destroyed after casing a single part, this technique is sometimes called eggshell molding.

While this other use case is definitely not so common as the first one, I still would like to keep it in the same operator. The application domain is the same (3d printing), and the two use cases share so much code that I think it makes sense to keep them together.

This second use case, let's call it mold making, is the reason why I used a signed offset as a parameter and mentioned the direction in the description. The "Negative -> inwards" has a counter part that means "positive -> outwards", which produces this other effect.

If we keep this mold making in the operator, there need to be some way to differentiate an inwards from an outwards offset surface. I used the sign convention of negative offset to inwards simply because that is the convention of OpenVDB when generating offset surfaces. Another possibility is to use the opposite sign convention, or to use two parameters like the Solidify modifier. Solidify uses one unsigned parameter, Thickness, for the amount of offset, and a signed parameter, Offset, to indicate inwards or outwards, with the convention of negative meaning inwards.

I'm open to the way to indicate that to the user, but I really would like to keep both functionalities in the operator, if possible.

My other point is about the default values for the voxel size. However, it is late here so I comment about that later. I'm also currently traveling on vacation, so don't be alarmed if I don't update as often as before.

Thanks again for your feedback. As for the last time, I agree with most points. I have, however, observations. ### Offset I was rather too liberal with comments in the code, but I think I still managed not to explain an important point. There are _two_ distinct use cases for this tool. One is the standard hollowing out, where the offset surface is on the inside of the the target object. The other use case is when the offset surface is on the _outside_ of the target. Its application is to generate a thin walled mesh that, after printing, can be used as a mold for the target object to cast other materials in the target form. If the mold is one-shot, that is, it is discarded or destroyed after casing a single part, this technique is sometimes called _eggshell molding_. While this other use case is definitely not so common as the first one, I still would like to keep it in the same operator. The application domain is the same (3d printing), and the two use cases share so much code that I think it makes sense to keep them together. This second use case, let's call it _mold making_, is the reason why I used a signed offset as a parameter and mentioned the direction in the description. The "Negative -> inwards" has a counter part that means "positive -> outwards", which produces this other effect. If we keep this mold making in the operator, there need to be some way to differentiate an inwards from an outwards offset surface. I used the sign convention of negative offset to inwards simply because that is the convention of OpenVDB when generating offset surfaces. Another possibility is to use the opposite sign convention, or to use two parameters like the Solidify modifier. Solidify uses one unsigned parameter, _Thickness_, for the amount of offset, and a signed parameter, _Offset_, to indicate inwards or outwards, with the convention of negative meaning inwards. I'm open to the way to indicate that to the user, but I really would like to keep both functionalities in the operator, if possible. My other point is about the default values for the voxel size. However, it is late here so I comment about that later. I'm also currently traveling on vacation, so don't be alarmed if I don't update as often as before.
Mikhail Rachinskiy commented 2024-02-29 00:22:13 +01:00
Outdated
Review
Copy Link

I'm not suggesting removing the bi-directional offset functionality, just flip the sign, so positive would mean inward and negative outward, similar to solidify modifier.

The only software that I know of that does negative inward is MeshLab, the rest of them have a separate direction switch toggle.

Have a nice time traveling!

I'm not suggesting removing the bi-directional offset functionality, just flip the sign, so positive would mean inward and negative outward, similar to solidify modifier. The only software that I know of that does negative inward is MeshLab, the rest of them have a separate direction switch toggle. Have a nice time traveling!
usfreitas commented 2024-03-12 19:48:32 +01:00
Outdated
Review
Copy Link

I'm back.

Regarding offset sign, I would rather go with an unsigned thickness parameter and a direction switch toggle, instead of just flipping the sign. I think it would be easier for users to understand.

Regarding the voxel size (this name is really better), I think we should be ware of setting the default too low. I see two reasons for that.

The first is that, when using Blender for 3D printing, the size of the objects in units tend to be larger compared to the default 0.1 of the remesh modifier. At least in my case, where I have my units in millimeters, and my objects easily reach the hundreds. I usually freeze or crash Blender if I'm not careful when adding a remesh modifier, for instance. In our case, if the voxel size is too small compared with the object, the level set creation in OpenVDB can use an excessive amount of memory. Another potential problem is when the voxel size is small compared with the offset. Since the half width of the level set needs to envelop the offset, this can lead to a very large number of active voxels in the level set, again needing a large amount of memory.

The other reason that I see is that a highly detailed offset surface is neither needed nor generally desired for hollowing out objects for printing, IMO. I personally prefer the offset surface to be low poly compared to the target object.

I ran some tests with the Stanford bunny in millimeters. The model is about 150mm long. I've generated an outside surface with a 2mm offset. Using a voxel size of 0.5mm it took a couple of seconds to execute and generated a mesh with more than ten times the number of triangles of the original model. With a voxel size of 0.3, it took 8 seconds to run and produced about 30x more tris than the model. A voxel size of 0.1 crashed my Blender. With 2mm voxel size, it runs instantly and the result has about 25% less tris than the model. For mold making, I think this latter result is still oversampled and could be made with even less tris.

With this in mind, I would suggest using a default of around 1.0 or larger, as a smaller offset could cause the problems above.

Another way of dealing with this question would be to have dynamic defaults. If we could somehow compute a voxel size adapted to the target object and offset value and set the default to this value, this would be a better solution than a hardcoded default. But I have no idea how to do this, or if dynamic defaults are even possible.

I'm back. Regarding offset sign, I would rather go with an unsigned thickness parameter and a direction switch toggle, instead of just flipping the sign. I think it would be easier for users to understand. Regarding the voxel size (this name is really better), I think we should be ware of setting the default too low. I see two reasons for that. The first is that, when using Blender for 3D printing, the size of the objects in units tend to be larger compared to the default 0.1 of the remesh modifier. At least in my case, where I have my units in millimeters, and my objects easily reach the hundreds. I usually freeze or crash Blender if I'm not careful when adding a remesh modifier, for instance. In our case, if the voxel size is too small compared with the object, the level set creation in OpenVDB can use an excessive amount of memory. Another potential problem is when the voxel size is small compared with the offset. Since the half width of the level set needs to envelop the offset, this can lead to a very large number of active voxels in the level set, again needing a large amount of memory. The other reason that I see is that a highly detailed offset surface is neither needed nor generally desired for hollowing out objects for printing, IMO. I personally prefer the offset surface to be low poly compared to the target object. I ran some tests with the Stanford bunny in millimeters. The model is about 150mm long. I've generated an outside surface with a 2mm offset. Using a voxel size of 0.5mm it took a couple of seconds to execute and generated a mesh with more than ten times the number of triangles of the original model. With a voxel size of 0.3, it took 8 seconds to run and produced about 30x more tris than the model. A voxel size of 0.1 crashed my Blender. With 2mm voxel size, it runs instantly and the result has about 25% less tris than the model. For mold making, I think this latter result is still oversampled and could be made with even less tris. With this in mind, I would suggest using a default of around 1.0 or larger, as a smaller offset could cause the problems above. Another way of dealing with this question would be to have dynamic defaults. If we could somehow compute a voxel size adapted to the target object and offset value and set the default to this value, this would be a better solution than a hardcoded default. But I have no idea how to do this, or if dynamic defaults are even possible.
usfreitas commented 2024-03-13 17:58:46 +01:00
Outdated
Review
Copy Link

I've committed some code. I've tried to make all the suggested changes, with some exceptions:

  • Default voxel size is 1.0
  • Offset now is only positive. Direction is indicated by a new boolean property.
  • Operator properties are still duplicated in the scene. The scene properties are updated when the operator concludes.

I've also changed the sequence of operations a bit, and now it can happen that the offset surface have its normals flipped twice. However, I think it is worth it due to the simpler logic and better code legibility.

I would like to thank you again for the time you are committing to this code review. I've learned a lot, and using the tool feels much better. I think code quality also improved significantly.

I've committed some code. I've tried to make all the suggested changes, with some exceptions: - Default voxel size is 1.0 - Offset now is only positive. Direction is indicated by a new boolean property. - Operator properties are still duplicated in the scene. The scene properties are updated when the operator concludes. I've also changed the sequence of operations a bit, and now it can happen that the offset surface have its normals flipped twice. However, I think it is worth it due to the simpler logic and better code legibility. I would like to thank you again for the time you are committing to this code review. I've learned a lot, and using the tool feels much better. I think code quality also improved significantly.
description = "Create a hollow copy of the target object, with applied modifiers and scale", description="Create hollowed out copy of the object",
default = False )
make_hollow_inside: BoolProperty(
name="Inside",
description="Offset surface inside of the object",
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'} ```
default=True,
) )
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.
MikhailRachinskiy marked this conversation as resolved
Mikhail Rachinskiy commented 2024-02-24 16:43:11 +01:00
Review
Copy Link

It's better to not create unnecessary local variables, especially when they are rarely used.

It's better to not create unnecessary local variables, especially when they are rarely used.
def execute(self, context): def execute(self, context):
@ -877,62 +884,64 @@ class MESH_OT_print3d_hollow(Operator):
tris.shape = (-1, 3) tris.shape = (-1, 3)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 17:44:33 +01:00
Outdated
Review
Copy Link

What is happening here? If you need to convert to list just use newquads = list(newquads).

What is happening here? If you need to convert to list just use `newquads = list(newquads)`.
# Generate VDB levelset # Generate VDB levelset
half_width = max(3.0, math.ceil(abs(self.offset) / self.resolution) + 2.0) # half_width has to envelop offset half_width = max(3.0, math.ceil(abs(self.offset) / self.voxel_size) + 2.0) # half_width has to envelop offset
trans = vdb.Transform() trans = vdb.Transform()
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 17:01:57 +01:00
Outdated
Review
Copy Link

It would be better to name it mesh_hollow to avoid shadowing and make intention clearer.

It would be better to name it `mesh_hollow` to avoid shadowing and make intention clearer.
trans.scale(self.resolution) trans.scale(self.voxel_size)
levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris, transform=trans, halfWidth=half_width) levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris, transform=trans, halfWidth=half_width)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 17:04:47 +01:00
Outdated
Review
Copy Link

We do not need to copy object transforms, it already happens in evaluated mesh.

We do not need to copy object transforms, it already happens in evaluated mesh.
# Generate offset surface # Generate offset surface
newverts, newquads = levelset.convertToQuads(self.offset) if self.make_hollow_inside:
newverts, newquads = levelset.convertToQuads(-self.offset)
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 16:40:31 +01:00
Outdated
Review
Copy Link

I really think that join feature is not good in this case, since we use evaluated mesh and original object could have a lot of modifiers, like Subd which will freeze/crash Blender if voxel density is too high.

Let's leave it to the user on what they want to do with the result.

I really think that join feature is not good in this case, since we use evaluated mesh and original object could have a lot of modifiers, like Subd which will freeze/crash Blender if voxel density is too high. Let's leave it to the user on what they want to do with the result.
usfreitas commented 2024-02-24 21:03:39 +01:00
Outdated
Review
Copy Link

Thank you for your feed back!

I very much agree with all the other points. However, on the case of join I have a counter proposal.

If we operate on the evaluated and scaled mesh, joining the generated offset surface with the target object really does not make any sense. But in this case, the offset surface on its own is not very useful for the user either. In order to generate an actually hollow object, they would have to apply modifiers and scale to (possibly a copy of) the target, flip the normals of the target or the generated surface, depending on the sign of the offset, and finally join the two. That is a somewhat complex series of operations that I think could be difficult for many users.

What if we offer the user the option to perform this operations for them? My idea is roughly this:

  1. instead of using a temporarily mesh from obj.to_mesh(), instantiate the mesh with modifiers in bpy.data.meshes
  2. use this mesh (scaled, etc.) as the basis for the offset surface
  3. if the user just wants the offset surface, remove this mesh from bpy.data.meshes, otherwise create a new object with it, adjust normals as required, and join it with the offset surface

Instead of calling this option as "Join", it could be something like "Create hollow copy" or "Offset surface only" for its inverse.

What do you think?

Thank you for your feed back! I very much agree with all the other points. However, on the case of join I have a counter proposal. If we operate on the evaluated and scaled mesh, joining the generated offset surface with the target object really does not make any sense. But in this case, the offset surface on its own is not very useful for the user either. In order to generate an actually hollow object, they would have to apply modifiers and scale to (possibly a copy of) the target, flip the normals of the target or the generated surface, depending on the sign of the offset, and finally join the two. That is a somewhat complex series of operations that I think could be difficult for many users. What if we offer the user the option to perform this operations for them? My idea is roughly this: 1. instead of using a temporarily mesh from `obj.to_mesh()`, instantiate the mesh with modifiers in `bpy.data.meshes` 1. use this mesh (scaled, etc.) as the basis for the offset surface 1. if the user just wants the offset surface, remove this mesh from `bpy.data.meshes`, otherwise create a new object with it, adjust normals as required, and join it with the offset surface Instead of calling this option as "Join", it could be something like "Create hollow copy" or "Offset surface only" for its inverse. What do you think?
Mikhail Rachinskiy commented 2024-02-25 11:53:58 +01:00
Outdated
Review
Copy Link

Yeah, let's do Create Hollow Copy instead.

I'm not getting what you mean by instantiate in bpy.data.meshes, we still have to use obj.to_mesh() to get evaluated mesh.
I'm not aware of native mesh.join(other_mesh) method, it seems object join operator is the only one available, so we have to use bmesh for that.

depsgraph = bpy.context.evaluated_depsgraph_get()

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

# Hollow stuff happens

bm = bmesh.new()
bm.from_mesh(me)
bm.from_mesh(mesh_hollow)

ob_eval.to_mesh_clear()

mesh_hollow_copy = bpy.data.meshes.new(name)
bm.to_mesh(mesh_hollow_copy)
bm.free()

Regular hollow result would not require bmesh, we use it only to join 2 meshes together.

It also would be nice to have hollow copy placed beside original object, so something like this:

ob_hollow_copy.location.x = max(ob_orig.dimensions)
Yeah, let's do `Create Hollow Copy` instead. I'm not getting what you mean by instantiate in `bpy.data.meshes`, we still have to use `obj.to_mesh()` to get evaluated mesh. I'm not aware of native `mesh.join(other_mesh)` method, it seems object join operator is the only one available, so we have to use `bmesh` for that. ```python depsgraph = bpy.context.evaluated_depsgraph_get() ob_eval = ob.evaluated_get(depsgraph) me = ob_eval.to_mesh() me.transform(ob.matrix_world) # Hollow stuff happens bm = bmesh.new() bm.from_mesh(me) bm.from_mesh(mesh_hollow) ob_eval.to_mesh_clear() mesh_hollow_copy = bpy.data.meshes.new(name) bm.to_mesh(mesh_hollow_copy) bm.free() ``` Regular hollow result would not require bmesh, we use it only to join 2 meshes together. It also would be nice to have hollow copy placed beside original object, so something like this: ```python ob_hollow_copy.location.x = max(ob_orig.dimensions) ```
usfreitas commented 2024-02-25 15:59:33 +01:00
Outdated
Review
Copy Link

I think it is better to explain with code. I implemented my idea in the commit I just pushed. It is functional, but I've left the old join parameter. I can change its name later. Here is what I did to get the evaluated mesh:

obj = context.active_object
depsgraph = context.evaluated_depsgraph_get()
mesh_target = bpy.data.meshes.new_from_object(obj.evaluated_get(depsgraph))
mesh_target.transform(obj.matrix_world)

Now mesh_target is in bpy.data.meshes. It is then used to create the offset surface. In case the user wants also the hollow object, it can be included in its own object, otherwise it is removed from bpy.data.meshes. From what I understood from obj.to_mesh(), the mesh it produces can not be used in other objects.

In this way, we can use the join operator, and bmesh and the conversions to/from mesh aren't needed. What do you think about this approach?

Some other points:

  • I've tried to address your other points in this commit, I'm not sure I succeeded.
  • I'm not very convinced about always moving away the new hollow copy. Maybe add an option for that?
  • This line mesh_target.transform(obj.matrix_world) will not only apply the scale, but will also set the mesh origin to the global origin. This is most noticeable if the target was translated somewhere before hollowing out. I don't like this effect. I would rather that the origin of the offset surface and the new hollow object stay on the same position of the target object. I'll try to find a way to do that.
I think it is better to explain with code. I implemented my idea in the commit I just pushed. It is functional, but I've left the old `join` parameter. I can change its name later. Here is what I did to get the evaluated mesh: ``` obj = context.active_object depsgraph = context.evaluated_depsgraph_get() mesh_target = bpy.data.meshes.new_from_object(obj.evaluated_get(depsgraph)) mesh_target.transform(obj.matrix_world) ``` Now `mesh_target` is in `bpy.data.meshes`. It is then used to create the offset surface. In case the user wants also the hollow object, it can be included in its own object, otherwise it is removed from `bpy.data.meshes`. From what I understood from `obj.to_mesh()`, the mesh it produces can not be used in other objects. In this way, we can use the join operator, and `bmesh` and the conversions to/from mesh aren't needed. What do you think about this approach? Some other points: - I've tried to address your other points in this commit, I'm not sure I succeeded. - I'm not very convinced about always moving away the new hollow copy. Maybe add an option for that? - This line `mesh_target.transform(obj.matrix_world)` will not only apply the scale, but will also set the mesh origin to the global origin. This is most noticeable if the target was translated somewhere before hollowing out. I don't like this effect. I would rather that the origin of the offset surface and the new hollow object stay on the same position of the target object. I'll try to find a way to do that.
Mikhail Rachinskiy commented 2024-02-25 17:45:54 +01:00
Outdated
Review
Copy Link

What do you think about this approach?

Your approach is fine, the intent is not as clear with new_from_object, but that is semantics, functionality is exactly the same.

mesh it produces can not be used in other objects

You cannot use the result of to_mesh directly, but you can create a copy of it in bpy.data.meshes like so me.copy()

not very convinced about always moving away the new hollow copy

That is not required, keeping it in place is fine.

will not only apply the scale, but will also set the mesh origin to the global origin

You got it a bit wrong, it does not affect origin, it just transforms mesh with object transforms, all new objects have their location in the center of the scene.

I would rather that the origin of the offset surface and the new hollow object stay on the same position of the target object. I'll try to find a way to do that.

That's easy:

from mathutils import Matrix

mat = Matrix.Translation(ob_orig.matrix_world.translation)
ob_copy.matrix_world = mat
mat.invert()
ob_copy.data.transform(mat)

I've left the old join parameter

It should be called Hollow Copy or Hollow Duplicate, the original object is not changed, we're not joining it with anything.

> What do you think about this approach? Your approach is fine, the intent is not as clear with `new_from_object`, but that is semantics, functionality is exactly the same. > mesh it produces can not be used in other objects You cannot use the result of `to_mesh` directly, but you can create a copy of it in `bpy.data.meshes` like so `me.copy()` > not very convinced about always moving away the new hollow copy That is not required, keeping it in place is fine. > will not only apply the scale, but will also set the mesh origin to the global origin You got it a bit wrong, it does not affect origin, it just transforms mesh with object transforms, all new objects have their location in the center of the scene. > I would rather that the origin of the offset surface and the new hollow object stay on the same position of the target object. I'll try to find a way to do that. That's easy: ```python from mathutils import Matrix mat = Matrix.Translation(ob_orig.matrix_world.translation) ob_copy.matrix_world = mat mat.invert() ob_copy.data.transform(mat) ``` > I've left the old join parameter It should be called `Hollow Copy` or `Hollow Duplicate`, the original object is not changed, we're not joining it with anything.
Mikhail Rachinskiy commented 2024-02-25 17:58:02 +01:00
Outdated
Review
Copy Link

hollow object stay on the same position of the target object

Or alternatively strip world matrix of translation component.

mat = ob.matrix_world.copy()
mat.translation = 0, 0, 0

me.transform(mat)

ob_hollow.matrix_world.translation = ob_orig.matrix_world.translation
> hollow object stay on the same position of the target object Or alternatively strip world matrix of translation component. ```python mat = ob.matrix_world.copy() mat.translation = 0, 0, 0 me.transform(mat) ob_hollow.matrix_world.translation = ob_orig.matrix_world.translation ```
usfreitas commented 2024-02-26 22:18:06 +01:00
Outdated
Review
Copy Link

You got it a bit wrong, it does not affect origin, it just transforms mesh with object transforms, all new objects have their location in the center of the scene

I see. Thanks for the info! Now I understand a problem I had with some of my other scripts.

Or alternatively strip world matrix of translation component.

That worked like a charm! Thanks again!

I've updated the branch with this idea. I've also changed the property name, but now I see I did not use your name suggestion, which I think is better. I'll change it later ( again 😞)

Do you have any more feedback/suggestions/comments about this part?

>You got it a bit wrong, it does not affect origin, it just transforms mesh with object transforms, all new objects have their location in the center of the scene I see. Thanks for the info! Now I understand a problem I had with some of my other scripts. > Or alternatively strip world matrix of translation component. That worked like a charm! Thanks again! I've updated the branch with this idea. I've also changed the property name, but now I see I did not use your name suggestion, which I think is better. I'll change it later ( again 😞) Do you have any more feedback/suggestions/comments about this part?
Mikhail Rachinskiy commented 2024-02-27 17:14:01 +01:00
Outdated
Review
Copy Link

Do you have any more feedback/suggestions/comments about this part?

Tool execution is fine, now we need to tackle UI. I'll do it in a separate thread.

> Do you have any more feedback/suggestions/comments about this part? Tool execution is fine, now we need to tackle UI. I'll do it in a separate thread.
else:
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: ... ```
newverts, newquads = levelset.convertToQuads(self.offset)
polys = list(newquads) polys = list(newquads)
# Instantiate new object in Blender # Instantiate new object in Blender
bpy.ops.object.mode_set(mode='OBJECT')
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).
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')
mesh_offset.from_pydata(newverts, [], polys) mesh_offset.from_pydata(newverts, [], polys)
obj_offset = bpy.data.objects.new(obj.name + ' offset', mesh_offset)
bpy.context.collection.objects.link(obj_offset)
if not self.create_hollow: # For some reason OpenVDB has inverted normals
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-27 19:59:29 +01:00
Outdated
Review
Copy Link

This code block is useless, just do if self.offset > 0.0

This code block is useless, just do `if self.offset > 0.0`
# For some reason OpenVDB has inverted normals mesh_offset.flip_normals()
mesh_offset.flip_normals() obj_offset = bpy.data.objects.new(obj.name + ' offset', mesh_offset)
# Translate surface object to target object's position 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)
# This mesh will not be used anymore obj_offset.select_set(True)
bpy.data.meshes.remove(mesh_target) context.view_layer.objects.active = obj_offset
else:
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.
# Create a copy of the target object with applied modifiers and scale if self.make_hollow_duplicate:
MikhailRachinskiy marked this conversation as resolved Outdated
Mikhail Rachinskiy commented 2024-02-24 17:11:30 +01:00
Outdated
Review
Copy Link

That's unnecessary, just check if in edit mode in invoke method and if true switch to object mode, switching back is not needed here.

That's unnecessary, just check if in edit mode in `invoke` method and if true switch to object mode, switching back is not needed here.
obj_hollow = bpy.data.objects.new(obj.name + " hollow", mesh_target) obj_hollow = bpy.data.objects.new(obj.name + " hollow", mesh_target)
bpy.context.collection.objects.link(obj_hollow) bpy.context.collection.objects.link(obj_hollow)
if self.offset < 0.0: obj_hollow.matrix_world.translation = obj.matrix_world.translation
# Offset surface already has normals as they should, see above
pass
else:
# Offset surface is outside, correct normals, see above
mesh_offset.flip_normals()
# Original surface is inside, flip its normals
mesh_target.flip_normals()
bpy.ops.object.mode_set(mode='OBJECT')
bpy.ops.object.select_all(action='DESELECT')
obj_offset.select_set(True)
obj_hollow.select_set(True) obj_hollow.select_set(True)
if self.make_hollow_inside:
mesh_offset.flip_normals()
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.
else:
mesh_target.flip_normals()
context.view_layer.objects.active = obj_hollow context.view_layer.objects.active = obj_hollow
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.
bpy.ops.object.join() bpy.ops.object.join()
else:
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.
bpy.data.meshes.remove(mesh_target)
# Translate hollow object to target object's position print_3d = context.scene.print_3d
obj_hollow.matrix_world.translation = obj.matrix_world.translation 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
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.
return {'FINISHED'} return {'FINISHED'}
def invoke(self, context, event): def invoke(self, context, event):
print_3d = context.scene.print_3d print_3d = context.scene.print_3d
self.offset = print_3d.hollow_offset self.offset = print_3d.hollow_offset
self.resolution = print_3d.hollow_resolution self.voxel_size = print_3d.hollow_voxel_size
self.create_hollow = print_3d.hollow_create self.make_hollow_duplicate = print_3d.make_hollow_duplicate
self.make_hollow_inside = print_3d.make_hollow_inside
if context.mode == 'EDIT_MESH': if context.mode == 'EDIT_MESH':
bpy.ops.object.mode_set(mode='OBJECT') bpy.ops.object.mode_set(mode='OBJECT')
wm = context.window_manager
return self.execute(context) return wm.invoke_props_dialog(self)

11
object_print3d_utils/ui.py
View File

@ -102,8 +102,8 @@ class VIEW3D_PT_print3d_cleanup(View3DPrintPanel, Panel):
# layout.operator("mesh.print3d_clean_thin", text="Wall Thickness") # layout.operator("mesh.print3d_clean_thin", text="Wall Thickness")
class VIEW3D_PT_print3d_transform(View3DPrintPanel, Panel): class VIEW3D_PT_print3d_edit(View3DPrintPanel, Panel):
bl_label = "Transform" bl_label = "Edit"
bl_options = {"DEFAULT_CLOSED"} bl_options = {"DEFAULT_CLOSED"}
def draw(self, context): def draw(self, context):
@ -111,6 +111,7 @@ class VIEW3D_PT_print3d_transform(View3DPrintPanel, Panel):
print_3d = context.scene.print_3d print_3d = context.scene.print_3d
layout.operator("mesh.print3d_hollow")
layout.label(text="Scale To") layout.label(text="Scale To")
row = layout.row(align=True) row = layout.row(align=True)
row.operator("mesh.print3d_scale_to_volume", text="Volume") row.operator("mesh.print3d_scale_to_volume", text="Volume")
@ -118,12 +119,6 @@ class VIEW3D_PT_print3d_transform(View3DPrintPanel, Panel):
row = layout.row(align=True) row = layout.row(align=True)
row.operator("mesh.print3d_align_to_xy", text="Align XY") row.operator("mesh.print3d_align_to_xy", text="Align XY")
row.prop(print_3d, "use_alignxy_face_area") row.prop(print_3d, "use_alignxy_face_area")
layout.label(text="Hollow")
col = layout.column(align=True)
col.prop(print_3d, "hollow_create")
col.prop(print_3d, "hollow_offset")
col.prop(print_3d, "hollow_resolution")
col.operator("mesh.print3d_hollow", text="Create Offset Surface")
class VIEW3D_PT_print3d_export(View3DPrintPanel, Panel): class VIEW3D_PT_print3d_export(View3DPrintPanel, Panel):