Details

Reviewers

Maniphest Tasks

Commits

rB95981c987648: Geometry Nodes: Extrude Mesh Node

Summary

This patch introduces an extrude node with three modes. The vertex mode is quite simple,
and just attaches new edges to the selected vertices. The edge mode attaches new faces
to the selected edges. The faces mode extrudes patches of selected faces, or each selected
face individually, depending on the "Individual" boolean input.

Attribute Propagation
Attributes are transferred to the new elements with specific rules.
Attributes will never change domains for interpolations. Generally boolean attributes are
propagated with "or", meaning any connected "true" value that is mixed in for other types
will cause the new value to be "true" as well. The "id" attribute does not have any special
handling currently.

Vertex Mode
- Vertex: Copied values of selected vertices.
- Edge: Averaged values of selected edges. For booleans, the edge is selected if any of the connected edges are selected.
Edge Mode
- Vertex: Copied values of extruded vertices.
- Connecting edges (vertical): Average values of connected extruded edges. For booleans, the edge is selected if any of the connected extruded edges are selected.
- Duplicate edges: Copied values of selected edges.
- Face: Averaged values of all faces connected to the selected edge. For booleans, the face is selected if any of the connected faces are selected.
- Corner: Averaged values of corresponding corners in all faces connected to the selected edge. For booleans, the corner is selected if one of those corners are selected.
Face Mode
- Vertex: Copied values of extruded vertices.
- Connecting edges (vertical): Average values of connected selected edges, not including the edges "on top" of extruded regions. For booleans, the edge is selected when any of those connected extruded edges were selected.
- Duplicate edges: Copied values of extruded edges.
- Face: Copied values of the corresponding selected faces.
- Corner: Copied values of the corresponding corner in the selected faces.
Individual Face Mode
- Vertex: Copied values of extruded vertices.
- Connecting edges (vertical): Average values of the two neighboring edges on each extruded polygon. For booleans, the edge is selected when at least one of the neighbors on the extruded face were selected.
- Duplicate edges: Copied values of extruded edges.
- Face: Copied values of the corresponding selected faces.
- Corner: Copied values of the corresponding corner in the selected faces.

Differences from edit mode
In face mode (non-individual), the behavior can be different than the extrude tools
in edit mode-- this node doesn't handle keeping the back-faces around in some cases.
The planned "Solidify" will handle that use case instead, and keeping this node simpler
is preferable at this point, especially because that sort of "smart" behavior is not that
predictable and makes less sense in a procedural context.

In the future, an "Even Offset" option could be added to this node hopefully fairly
simply. For now it is left out in order to keep the patch simpler.

Implementation
For the implementation, the Mesh data structure is used directly rather than converting
to BMesh and back like D12224. This optimizes for large extrusion operations rather than
many sequential extrusions. While this can be slightly more verbose, it has a few important
benefits: First, there is no inefficient conversion to and from BMesh. The code only has
to fill arrays and it can do that all at once, rather than working as a combination of
many smaller operations. This makes each component of the algorithm much easier to
optimize. It also makes the attribute interpolation more explicit, and likely
faster. Only limited topology maps must be created in most cases.

While there are some necessary loops and allocations for the entire mesh, I tried to keep
everything I could on the order of the size of the selection rather than the size of the mesh.
In that respect, the individual faces mode is probably the best, since there is no topology
information necessary from the input mesh, and the amount of work just depends on
the size of the selection.

These are the test files I used during development. When this is committed I will turn them
into a bunch of regression tests. For reviewers, I suggest looking through the file to
make sure the behavior in the various cases is expected.

Extrude Duplicate Inner Edge Test.blend777 KBDownload

Mesh Extrude Test.blend2 MBDownload

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add implicit normal input (I added this directly in MOD_nodes_evaluator.cc because it will get much simpler when D12770 is committed, and adding it elsewhere didn't feel semantically correct).
Add the strength input to scale the offset input
Merge branch 'master' into temp-geometry-nodes-extrude-mesh
Fix offset of selected vertices inside extrude regions in face mode
Fix shade smooth interpolation in individual mode

I think the way the offset and strength inputs interact is a little odd-- I think personally I would keep it how it was before. But I do see how an implicit normal input will save people time, and how using the length of the offset input is necessary to allow calculating it directly.

I do personally like this behaviour with implicit normal offset a lot better!

I just noticed the inconsistency with the mesh editing behaviour when the selection of the extrusion is equal to a full mesh island (?). I'm actually not quite sure what the rules for this are, it does seem a bit arbitrary.
I do think though, that ideally the node behaviour would be consistent with what the operator does.

It seems to me that in the case that if the selection of the extrusion is equal to a full mesh island, the original mesh is kept around with inverted normals.

In D13709#364251, @Simon Thommes (simonthommes) wrote:

I just noticed the inconsistency with the mesh editing behaviour when the selection of the extrusion is equal to a full mesh island (?). I'm actually not quite sure what the rules for this are, it does seem a bit arbitrary.
I do think though, that ideally the node behaviour would be consistent with what the operator does.
It seems to me that in the case that if the selection of the extrusion is equal to a full mesh island, the original mesh is kept around with inverted normals.

I actually found some information on that in the manual: docs.blender.org/manual/en/2.82/modeling/meshes/editing/duplicating/extrude.html#details
Here's the important bit:

If the edges in the edge loop belong to only one face in the complete mesh, then all of the selected faces are duplicated and linked to the newly created faces. For example, rectangles will result in parallelepipeds during this stage.

In other cases, the selected faces are linked to the newly created faces but not duplicated. This prevents undesired faces from being retained “inside” the resulting mesh. This distinction is extremely important since it ensures the construction of consistently coherent, closed volumes at all times when using Extrude.

However, I think "belong to only one face in the complete mesh" is wrong, because the faces are also duplicated when I extrude a grid primitive, for example.
If I can understand the behavior in edit mode properly, I will be able to implement it relatively easily I think.

Merge branch 'master' into temp-geometry-nodes-extrude-mesh
Move normal field input to geometry set files

If I can understand the behavior in edit mode properly, I will be able to implement it relatively easily I think.

Okay, here is my current hypothesis: Duplicate the extruded faces iff all of the extruded edges making up the boundary region do not connect to any other faces (they are boundary edges). I'll go ahead and try implementing that next.
I think this is actually just a rephrasing of what @Simon Thommes (simonthommes) said actually, good.

In D13709#364267, @Hans Goudey (HooglyBoogly) wrote:

Okay, here is my current hypothesis: Duplicate the extruded faces if all of the extruded edges making up the boundary region do not connect to any other faces.

Thanks for looking into it, this is a good way of phrasing it, I think. The addition of

(they are boundary edges)

is not always accurate though, I think. There is also the case that the boundary region does not have boundary edges, e.g. a closed mesh like a whole cube.

Long term the edge extrusion mode could also support the individual mode, right?

source/blender/blenkernel/BKE_mesh.h
995 ↗	(On Diff #46702)	Doesn't seem like an api we can just add without approval by others. Also when adding an api like this we should think about how a similar api could be used for other dna structs. I'd say, just don't add these utility methods here yet.
source/blender/blenlib/BLI_color.hh
99 ↗	(On Diff #46702)	This would turn it into a non trivial type, which might not be what we want. Setting these default could cause overhead in situations where we allocate large color arrays, because they will always be initialized by default.
source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
88	Assumes that the attribute is filled with `false` by default. Would have to check if we can safely make this assumption. Either way it would be good to add that as a comment or assert.
191	`/* New vertices copy the attribute values from their source vertex. */`
200	`/* New edges get their attributes by mixing the values of edges connected to the source vertex. */` Need to think about how edge selections are propagated here. Maybe another mixer for bools could be useful here.
440	typo (`of all of an`)
457	`/* New vertices copy the attributes from their original vertices. */`
464	/* Two cases: * - Edges parallel to original edges: Copy the edge attributes from the original edges. * - Edges connected to original vertices: Mix values of selected edges that are connected to the original vertex. */
487	`/* A new face gets its attribute value by mixing the values of faces connected to the original edge. */`
492	How does this help with multi-threading? You could use a mixer just for the current `range`.
519	Talking about `offset` here might make this more confusing, and is also not really correct.
614	This case seems to be handled incorrectly: extrude_faces_bug.blend791 KBDownload When enabling the node and applying it, the following issue occurs: See how there is an edge missing at the top. And when I move the lower edge, it actually also moves the edge above. Guess the problem is that this is a third kind of edge which is not yet handled (the other two kinds are edges that are just moved and edges that are extruded).

Jacques Lucke (JacquesLucke) requested changes to this revision.Jan 7 2022, 12:02 PM

This revision now requires changes to proceed.Jan 7 2022, 12:02 PM

We should try find a general rule for how selections are propagated during extrusion. Relying on the default mixer is probably not reliable enough.

source/blender/blenkernel/BKE_geometry_set.hh
1091 ↗	(On Diff #46702)	This refactor can also be done in a separate commit.
source/blender/blenkernel/intern/geometry_set.cc
597 ↗	(On Diff #46702)	Wrong name.

unizen added a subscriber: unizen.Jan 9 2022, 2:45 AM

heini (ogonek) added a subscriber: heini (ogonek).Jan 10 2022, 10:09 AM

William Reynish (billreynish) awarded a token.Jan 10 2022, 11:53 AM

Can we get an "Inset" option added to the extrude node? This would help greatly as Inset and Extrusion are often used together, and this would also make unique Inset node unnecessary. When it comes to modeling operators in Blender, the Inset Operator is essentially just more capable Extrude Operator, so I think it makes more sense to combine the two in one node...?

In D13709#365554, @Ludvik Koutny (rawalanche) wrote:

Can we get an "Inset" option added to the extrude node? This would help greatly as Inset and Extrusion are often used together, and this would also make unique Inset node unnecessary. When it comes to modeling operators in Blender, the Inset Operator is essentially just more capable Extrude Operator, so I think it makes more sense to combine the two in one node...?

I agree that there should be a scale option on the extrude but I don't think it should replace an inset node.
Inset should have multiple modes like centre / percent / edge distance / proportional etc as well as a normal offset (similar to pressing ctrl while insetting in 3D).

I know there's a push for nodes to be single function but I think when we start getting into actual tools that people use that that could become an impedance.
Most people aren't going to be relying on custom groups to give more familiar functionality.

We are working on a separate node for scaling that should do the job much better than a simple scale option in the extrude node: D13757.
We intend to get a separate inset node in the future. It's a slightly different operation.

Jacques Lucke (JacquesLucke) mentioned this in D13757: Geometry Nodes: New Scale Elements node..Jan 11 2022, 5:52 PM

kursad k (kursadk) awarded a token.Jan 12 2022, 6:35 AM

I noticed some counter-intuitive work in edge data inheritance

is there any chance that new polygons will inherit edge data?

Yegor R (Branskugel) awarded a token.Jan 16 2022, 9:46 AM

silex (silex) awarded a token.Jan 16 2022, 10:52 AM

Hans Goudey (HooglyBoogly) marked 15 inline comments as done.Jan 21 2022, 7:05 AM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/BKE_geometry_set.hh
1091 ↗	(On Diff #46702)	I moved this to D13779.
source/blender/blenkernel/BKE_mesh.h
995 ↗	(On Diff #46702)	Yeah, you're right, I should have removed this before adding you as a reviewer, since I was mostly just seeing what it would look like. Eventually it might be better to have a wrapper class that's to a mesh with methods like `edges()`. Should definitely be considered more generally though.
source/blender/blenlib/BLI_color.hh
99 ↗	(On Diff #46702)	Good point! And this is unnecessary now anyway, with the change to `CustomData_realloc`.
source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
492	Hmm, of course, right. This comment comes from refactoring I did to use the `edge_to_poly_map` here, but indeed, it would be much better to use a mixer for the entire chunk.
519	I'm not sure that it's incorrect, but I can see why it's not helpful too, I'll reword it.

Lots of cleanup
Use VectorSet instead of allocating full arrays in many cases
Improve comments
Use helper functions for defining edges and faces
Fix problem with inner edges also connected to unselected faces
Rename "Strength" input to "Offset Scale". I think "Strength" is a vague term that doesn't sound quite right in this case. "Offset Scale" is a bit longer, but it's more helpful.

Variable names and comments

Still checking. The selection propagation does not seem correct yet. Maybe you need a non-default mixer that mixes bools using OR.

This file looks wrong. See how the symmetry of the selection is not maintained. Maybe it's also an issue with domain interpolation, needs to be checked in more detail.

wrong_selection_propagation.blend784 KBDownload

A bit more cleanup, mostly to comments
Fix face individual connecting edges attribute interpolation

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jan 21 2022, 9:26 PM

Is there going to be a test build or will this just merge in?

I just tested it.
I believe It definitely lacks some ticks:
“Even thickness”, so it can be used like the solidify modifier, and wouldn’t pinch at corners. Or… it could just always work correctly and not pinch at corners. From many years of using blender (Since ~1.45), never have I ever seen use for this pinching at corners behavior. Always a clutch.... Shouldn't be a thing neither for the Extrude tool, nor the node...
“Keep original”, so the original faces are still there, and connected, and no workaround is required.

This revision now requires changes to proceed.Jan 21 2022, 11:43 PM

"Request changes" is meant for developers. There are other places for user feedback about in-development features: Chat, devtalk, design tasks, etc.

“Even thickness”, so it can be used like the solidify modifier, and wouldn’t pinch at corners.

This patch is structured in a way that should make that easy to add. However, it can be handled separately, to keep this change smaller/simpler.
In a procedural context where you can explicitly define the extrusion for every face, it's important to be able to control that sort of thing.

“Keep original”, so the original faces are still there, and connected, and no workaround is required.

We discussed that this week, and the conclusion of that discussion is in the patch description (the paragraph starting with "In face mode").
The planned solidify node will handle that use case much better.

Keep in mind that options can always be added in the future, but removing them is much more difficult.
Because of that, it's better to err on the side of "simpler" when there's a large new piece of code like this.

This revision now requires review to proceed.Jan 21 2022, 11:58 PM

Mix boolean attributes with a new "or" mixer

Corner: Averaged values of corresponding corners in all faces connected to the selected edge. For booleans, the corner is selected if at least half of those corners are selected.

That's outdated, I guess?

While not urgent, the commit message could mention that the id attribute does not have any special handling.

Seems to work well in my tests and the code looks good as well, good job.

source/blender/blenkernel/BKE_attribute_math.hh
367	Typo (`propatation`)
source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
1000	Looks like `vert_offsets` is only used in this branch but is still computed in every case.
1195	typo (`correspoinding`)

This revision is now accepted and ready to land.Jan 23 2022, 4:15 PM

Hans Goudey (HooglyBoogly) marked an inline comment as done.Jan 24 2022, 4:59 AM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
1000	Hmm, not sure I see this, `vert_offsets` is only computed inside `if (!poly_offsets.is_single())` above.

Hans Goudey (HooglyBoogly) marked an inline comment as done.Jan 24 2022, 4:59 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Hans Goudey (HooglyBoogly) set the repository for this revision to rB Blender.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jan 24 2022, 5:01 AM

Jacques Lucke (JacquesLucke) added inline comments.Jan 24 2022, 5:05 AM

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
1000	Ah right. I wonder why it is computed up there and not here where it is used.

Closed by commit rB95981c987648: Geometry Nodes: Extrude Mesh Node (authored by Hans Goudey (HooglyBoogly)). · Explain WhyJan 24 2022, 5:43 AM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB95981c987648: Geometry Nodes: Extrude Mesh Node.

Hans Goudey (HooglyBoogly) added inline comments.Jan 24 2022, 5:48 AM

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
1000	In the comment up there ;) This must be done before the mesh's custom data layers are reallocated, in case the virtual array references on of them.

Hans Goudey (HooglyBoogly) added inline comments.Jan 24 2022, 5:49 AM

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc
1000	Oh, sorry, that's in the other function! I kept running into that problem. Some of the code is similar between edges and faces, but only "coincidentally", i.e. not enough to really share much code.

Mingwei Lim (cmzw) awarded a token.Jan 24 2022, 9:14 AM

Duarte Farrajota Ramos (duarteframos) awarded a token.Jan 24 2022, 11:26 AM

炎臻 (PrettyFireNOI7) awarded a token.Jan 25 2022, 9:42 AM

Luca Saiani (lucasaiani) added a subscriber: Luca Saiani (lucasaiani).Aug 26 2022, 12:58 PM

Geometry Nodes: Extrude Mesh Node
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 47366

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_attribute_math.hh

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/customdata.cc

source/blender/blenkernel/intern/node.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/modifiers/intern/MOD_nodes_evaluator.cc

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/CMakeLists.txt

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc

Geometry Nodes: Extrude Mesh NodeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 47366

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_attribute_math.hh

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/customdata.cc

source/blender/blenkernel/intern/node.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/modifiers/intern/MOD_nodes_evaluator.cc

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/CMakeLists.txt

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc

Geometry Nodes: Extrude Mesh Node
ClosedPublic
Actions

Revision Contents
Changeset List