3D Print Toolbox: Add hollow out #105194
@ -853,66 +853,66 @@ class MESH_OT_print3d_hollow(Operator):
|
||||
import numpy as np
|
||||
import pyopenvdb as vdb
|
||||
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
|
||||
offset = self.offset
|
||||
resolution = self.resolution
|
||||
join = self.join
|
||||
|
||||
mode_orig = context.mode
|
||||
if not self.offset:
|
||||
return {'FINISHED'}
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
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 Also, it is better to group related options together, so place 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
You did not expand enum property so it currently shows as dropdown list. Add draw method to the operator, right before If you want to keep presets, then throw 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
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
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.
|
||||
# Target object
|
||||
obj = context.active_object
|
||||
m = obj.data # mesh
|
||||
depsgraph = context.evaluated_depsgraph_get()
|
||||
mesh_target = bpy.data.meshes.new_from_object(obj.evaluated_get(depsgraph))
|
||||
mesh_target.transform(obj.matrix_world)
|
||||
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
Code style Here Getting Mesh
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).
|
||||
# Read mesh to numpy arrays
|
||||
nverts = len(m.vertices)
|
||||
ntris = len(m.loop_triangles)
|
||||
verts = np.zeros(3*nverts, dtype=np.float32)
|
||||
tris = np.zeros(3*ntris, dtype=np.int32)
|
||||
m.vertices.foreach_get('co', verts)
|
||||
nverts = len(mesh_target.vertices)
|
||||
ntris = len(mesh_target.loop_triangles)
|
||||
verts = np.zeros(3 * nverts, dtype=np.float32)
|
||||
tris = np.zeros(3 * ntris, dtype=np.int32)
|
||||
mesh_target.vertices.foreach_get('co', verts)
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
Whitespace missing around multiplication operator, same thing below with division. Whitespace missing around multiplication operator, same thing below with division.
|
||||
verts.shape = (-1, 3)
|
||||
m.loop_triangles.foreach_get('vertices', tris)
|
||||
mesh_target.loop_triangles.foreach_get('vertices', tris)
|
||||
tris.shape = (-1, 3)
|
||||
|
||||
# Generate VDB levelset
|
||||
half_width = max(3.0, math.ceil(abs(offset)/resolution) + 2.0) # half_width has to envelop offset
|
||||
half_width = max(3.0, math.ceil(abs(self.offset) / self.resolution) + 2.0) # half_width has to envelop offset
|
||||
trans = vdb.Transform()
|
||||
trans.scale(resolution)
|
||||
levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris,
|
||||
transform=trans, halfWidth=half_width)
|
||||
trans.scale(self.resolution)
|
||||
levelset = vdb.FloatGrid.createLevelSetFromPolygons(verts, triangles=tris, transform=trans, halfWidth=half_width)
|
||||
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
Trailing whitespace and hard to read. Either do a newline for each attribute or keep it single line, it's not that long anyway. Trailing whitespace and hard to read. Either do a newline for each attribute or keep it single line, it's not that long anyway.
|
||||
# Generate offset surface
|
||||
newverts, newquads = levelset.convertToQuads(offset)
|
||||
polys = [x for x in newquads]
|
||||
newverts, newquads = levelset.convertToQuads(self.offset)
|
||||
polys = list(newquads)
|
||||
|
||||
# Instantiate new object in Blender
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
What is happening here? If you need to convert to list just use What is happening here? If you need to convert to list just use `newquads = list(newquads)`.
|
||||
mesh = bpy.data.meshes.new(m.name + ' offset')
|
||||
mesh.from_pydata(newverts, [], polys)
|
||||
newobj = bpy.data.objects.new(obj.name + ' offset', mesh)
|
||||
newobj.matrix_world = obj.matrix_world.copy()
|
||||
bpy.context.collection.objects.link(newobj)
|
||||
mesh_offset = bpy.data.meshes.new(mesh_target.name + ' offset')
|
||||
mesh_offset.from_pydata(newverts, [], polys)
|
||||
obj_offset = bpy.data.objects.new(obj.name + ' offset', mesh_offset)
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
It would be better to name it It would be better to name it `mesh_hollow` to avoid shadowing and make intention clearer.
|
||||
bpy.context.collection.objects.link(obj_offset)
|
||||
|
||||
if not join:
|
||||
if not self.join:
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
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.
|
||||
# For some reason OpenVDB has inverted normals
|
||||
mesh.flip_normals()
|
||||
mesh_offset.flip_normals()
|
||||
# This mesh will not be used anymore
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
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
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:
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
Yeah, let's do I'm not getting what you mean by instantiate in 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: 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
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 Now In this way, we can use the join operator, and Some other points:
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
Your approach is fine, the intent is not as clear with
You cannot use the result of
That is not required, keeping it in place is fine.
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.
That's easy:
It should be called > 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
Or alternatively strip world matrix of translation component. > 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
I see. Thanks for the info! Now I understand a problem I had with some of my other scripts.
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
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.
|
||||
bpy.data.meshes.remove(mesh_target)
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
It's better to avoid negation, if possible, it just makes harder to follow the logic. It's better to avoid negation, if possible, it just makes harder to follow the logic.
```python
if self.create_hollow:
...
else:
...
```
|
||||
else:
|
||||
if offset < 0.0:
|
||||
# Create a copy of the target object with applied modifiers, scale
|
||||
obj_hollow = bpy.data.objects.new(obj.name + " hollow", mesh_target)
|
||||
bpy.context.collection.objects.link(obj_hollow)
|
||||
if self.offset < 0.0:
|
||||
# Offset surface already has normals as they should, see above
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
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 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).
|
||||
pass
|
||||
else:
|
||||
# Offset surface is outside, correct normals, see above
|
||||
mesh.flip_normals()
|
||||
mesh_offset.flip_normals()
|
||||
# Original surface is inside, flip its normals
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
This code block is useless, just do This code block is useless, just do `if self.offset > 0.0`
|
||||
m.flip_normals()
|
||||
mesh_target.flip_normals()
|
||||
|
||||
bpy.ops.object.mode_set(mode='OBJECT')
|
||||
bpy.ops.object.select_all(action='DESELECT')
|
||||
newobj.select_set(True)
|
||||
obj.select_set(True)
|
||||
context.view_layer.objects.active = obj
|
||||
obj_offset.select_set(True)
|
||||
obj_hollow.select_set(True)
|
||||
context.view_layer.objects.active = obj_hollow
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
Again, you don't need to comment on every line, there is a check above 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.
|
||||
bpy.ops.object.join()
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
That's unnecessary, just check if in edit mode in 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.
|
||||
|
||||
if mode_orig == 'EDIT_MESH':
|
||||
bpy.ops.object.mode_set(mode='EDIT')
|
||||
|
||||
return {'FINISHED'}
|
||||
|
||||
@ -921,6 +921,8 @@ class MESH_OT_print3d_hollow(Operator):
|
||||
self.offset = print_3d.hollow_offset
|
||||
self.resolution = print_3d.hollow_resolution
|
||||
self.join = print_3d.hollow_join
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
If you find matrix stuff confusing, then you could write it as: 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. 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
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 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 context.mode == 'EDIT_MESH':
|
||||
bpy.ops.object.mode_set(mode='OBJECT')
|
||||
|
MikhailRachinskiy marked this conversation as resolved
Outdated
Mikhail Rachinskiy
commented
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.
|
||||
|
||||
return self.execute(context)
|
||||
|
||||
|
||||
Ther should be "gate" conditions at the beginning of the execute method checking for invalid values.