Geometry Nodes: Add "Corners of Edge" node #107968

Merged
Hans Goudey merged 15 commits from F_Scociety/blender:corners_of_edge into main 2023-05-31 15:25:54 +02:00

Adds the "Corners of Edge" topology node to geometry nodes.

With combining this node with the "Face of Corner" node would allow to get informations about the connected Faces to an edge.

In the Demo video, i store a wave texture as an colored Attribute to the Faces. Using the Corners of Edge node I get the two faces of each edge, and compare their difference. Then i create the lines at the edges, with high contrast areas.

This is the implementation of the proposial (3. option): #29379

Adds the "Corners of Edge" topology node to geometry nodes. With combining this node with the "Face of Corner" node would allow to get informations about the connected Faces to an edge. In the Demo video, i store a wave texture as an colored Attribute to the Faces. Using the Corners of Edge node I get the two faces of each edge, and compare their difference. Then i create the lines at the edges, with high contrast areas. This is the implementation of the proposial (3. option): [#29379](https://devtalk.blender.org/t/geometrynodes-proposial-new-corners-of-edge-node/29379)
Soeren Schmidt-Clausen added 2 commits 2023-05-16 12:57:55 +02:00
Iliya Katushenock added this to the Nodes & Physics project 2023-05-16 13:21:02 +02:00
Iliya Katushenock added the
Interest
Geometry Nodes
label 2023-05-16 13:21:07 +02:00
Soeren Schmidt-Clausen requested review from Hans Goudey 2023-05-16 13:45:47 +02:00
Member

The code looks good to me. It would be really helpful if this PR included a demo/test file. That could be added to the regression test suite and help show people how the node can be used. Thanks!

The code looks good to me. It would be really helpful if this PR included a demo/test file. That could be added to the regression test suite and help show people how the node can be used. Thanks!

What is the meaning of Total output? Can this be replaced by using the Edge Neighbors node?

What is the meaning of `Total` output? Can this be replaced by using the `Edge Neighbors` node?
Member

What is the meaning of Total output? Can this be replaced by using the Edge Neighbors node?

The total output is the same as for the other topology nodes. Best to be consistent here. This node would probably replace the Edge Neighbors node eventually, rather than the other way around.

> What is the meaning of `Total` output? Can this be replaced by using the `Edge Neighbors` node? The total output is the same as for the other topology nodes. Best to be consistent here. This node would probably replace the Edge Neighbors node eventually, rather than the other way around.
Author
Contributor

The code looks good to me. It would be really helpful if this PR included a demo/test file. That could be added to the regression test suite and help show people how the node can be used. Thanks!

Added the a video, file and a preview how the nodes looks, no sure if I can embed it between the text.
I also changed the ID registration to 2103, to not conflict with the "Signed Distance" node.

> The code looks good to me. It would be really helpful if this PR included a demo/test file. That could be added to the regression test suite and help show people how the node can be used. Thanks! Added the a video, file and a preview how the nodes looks, no sure if I can embed it between the text. I also changed the ID registration to 2103, to not conflict with the "Signed Distance" node.
Soeren Schmidt-Clausen added 1 commit 2023-05-16 18:18:24 +02:00
Hans Goudey requested changes 2023-05-16 18:23:33 +02:00
@ -426,6 +426,7 @@ class NODE_MT_geometry_node_mesh_topology(Menu):
layout = self.layout
node_add_menu.add_node_type(layout, "GeometryNodeCornersOfFace")
node_add_menu.add_node_type(layout, "GeometryNodeCornersOfVertex")
node_add_menu.add_node_type(layout, "GeometryNodeCornersOfEdge")
Member

Alphabetical order puts this above GeometryNodeCornersOfFace

Alphabetical order puts this above `GeometryNodeCornersOfFace`
F_Scociety marked this conversation as resolved
@ -1283,6 +1283,7 @@ void BKE_nodetree_remove_layer_n(struct bNodeTree *ntree, struct Scene *scene, i
#define GEO_NODE_MESH_TOPOLOGY_OFFSET_CORNER_IN_FACE 1180
#define GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_FACE 1181
#define GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_VERTEX 1182
#define GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_EDGE 2103
Member

Unlike the others, this list is sorted chronologically

Unlike the others, this list is sorted chronologically
F_Scociety marked this conversation as resolved
@ -377,6 +377,7 @@ DefNode(GeometryNode, GEO_NODE_MESH_TO_SDF_VOLUME, def_geo_mesh_to_sdf_volume, "
DefNode(GeometryNode, GEO_NODE_MESH_TO_VOLUME, def_geo_mesh_to_volume, "MESH_TO_VOLUME", MeshToVolume, "Mesh to Volume", "Create a fog volume with the shape of the input mesh's surface")
DefNode(GeometryNode, GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_FACE, 0, "CORNERS_OF_FACE", CornersOfFace, "Corners of Face", "Retrieve corners that make up a face")
DefNode(GeometryNode, GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_VERTEX, 0, "CORNERS_OF_VERTEX", CornersOfVertex, "Corners of Vertex", "Retrieve face corners connected to vertices")
DefNode(GeometryNode, GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_EDGE, 0, "CORNERS_OF_EDGE", CornersOfEdge, "Corners of Edge", "Retrieve face corners connected to edges")
Member

Alphabetical order

Alphabetical order
F_Scociety marked this conversation as resolved
@ -132,6 +132,7 @@ set(SRC
nodes/node_geo_mesh_to_volume.cc
nodes/node_geo_mesh_topology_corners_of_face.cc
nodes/node_geo_mesh_topology_corners_of_vertex.cc
nodes/node_geo_mesh_topology_corners_of_edge.cc
Member

Alphabetical order

Alphabetical order
F_Scociety marked this conversation as resolved
@ -116,6 +116,7 @@ void register_geometry_nodes()
register_node_type_geo_mesh_to_volume();
register_node_type_geo_mesh_topology_corners_of_face();
register_node_type_geo_mesh_topology_corners_of_vertex();
register_node_type_geo_mesh_topology_corners_of_edge();
Member

Alphabetical order

Alphabetical order
F_Scociety marked this conversation as resolved
@ -113,6 +113,7 @@ void register_node_type_geo_mesh_to_sdf_volume();
void register_node_type_geo_mesh_to_volume();
void register_node_type_geo_mesh_topology_corners_of_face();
void register_node_type_geo_mesh_topology_corners_of_vertex();
void register_node_type_geo_mesh_topology_corners_of_edge();
Member

Alphabetical order

Alphabetical order
F_Scociety marked this conversation as resolved
@ -0,0 +33,4 @@
.description(N_("The number of faces or corners connected to each edge"));
}
static void conedge_span(const Span<int> src, MutableSpan<int64_t> dst)
Member

Heh, find and replace mistake here

Heh, find and replace mistake here
Author
Contributor

Iam sorry, but i cant find anything wrong about this.

Iam sorry, but i cant find anything wrong about this.

Is the existence of conedge_span really necessary?

Is the existence of `conedge_span` really necessary?
Member

conedge_span should be convert_span. I assume you did a vert -> edge find and replace

`conedge_span` should be `convert_span`. I assume you did a `vert` -> `edge` find and replace
Member

Is the existence of conedge_span really necessary?

This is the same in two other existing topology nodes. It's not really worth changing in this PR

> Is the existence of conedge_span really necessary? This is the same in two other existing topology nodes. It's not really worth changing in this PR
Author
Contributor

conedge_span should be convert_span. I assume you did a vert -> edge find and replace

jep, corrected it. this one was actually manual, but misunderstood the name

> `conedge_span` should be `convert_span`. I assume you did a `vert` -> `edge` find and replace jep, corrected it. this one was actually manual, but misunderstood the name
F_Scociety marked this conversation as resolved
@ -0,0 +182,4 @@
uint64_t hash() const final
{
return 253098745374645;
Member

I think I might have told you something different before, but this hash() function can just be removed, so it doesn't try to deduplicate this field input at all. Same with is_equal_to. (Currently they don't take the inputs into account, which is fine though).

I think I might have told you something different before, but this `hash()` function can just be removed, so it doesn't try to deduplicate this field input at all. Same with `is_equal_to`. (Currently they don't take the inputs into account, which is fine though).
F_Scociety marked this conversation as resolved
Iliya Katushenock reviewed 2023-05-16 18:34:49 +02:00
Iliya Katushenock left a comment
Member

/ ops, i can't make a sequence of comments... /

/ ops, i can't make a sequence of comments... /
Iliya Katushenock reviewed 2023-05-16 18:36:03 +02:00
Iliya Katushenock reviewed 2023-05-16 18:36:14 +02:00
@ -0,0 +89,4 @@
for (const int selection_i : mask.slice(range)) {
const int edge_i = edge_indices[selection_i];
const int index_in_sort = indices_in_sort[selection_i];
if (!edge_range.contains(edge_i)) {

For some reason it seems to me that there is a logical error in the use of context. Can you elaborate on the meaning of !edge_range.contains(edge_i)?

For some reason it seems to me that there is a logical error in the use of context. Can you elaborate on the meaning of `!edge_range.contains(edge_i)`?
Author
Contributor

it checks if the edge_i(index) is not inside the edge_range. I looked at other topology nodes and they have this too, so i think its prevents crashes, when the evalutation failed.

it checks if the edge_i(index) is not inside the edge_range. I looked at other topology nodes and they have this too, so i think its prevents crashes, when the evalutation failed.

Do I understand correctly that if we calculate this for points, then each point corresponds to a random edge (in tot_points <= tot_edges)?

Do I understand correctly that if we calculate this for points, then each point corresponds to a random edge (in tot_points <= tot_edges)?
mod_moder marked this conversation as resolved
Iliya Katushenock reviewed 2023-05-16 18:36:27 +02:00
@ -0,0 +61,4 @@
{
const IndexRange edge_range(mesh.totedge);
const Span<int> corner_edges = mesh.corner_edges();
Array<Vector<int>> edge_to_loop_map = bke::mesh_topology::build_edge_to_loop_map(corner_edges,

edge_to_loop_map can be const

`edge_to_loop_map` can be const
F_Scociety marked this conversation as resolved
Soeren Schmidt-Clausen added 1 commit 2023-05-16 18:38:46 +02:00
Soeren Schmidt-Clausen added 1 commit 2023-05-16 19:05:19 +02:00
Soeren Schmidt-Clausen requested review from Hans Goudey 2023-05-16 19:05:32 +02:00
Soeren Schmidt-Clausen added 1 commit 2023-05-16 22:19:55 +02:00
Hans Goudey approved these changes 2023-05-16 22:38:03 +02:00
@ -1281,6 +1281,7 @@ void BKE_nodetree_remove_layer_n(struct bNodeTree *ntree, struct Scene *scene, i
#define GEO_NODE_CURVE_TOPOLOGY_CURVE_OF_POINT 1178
#define GEO_NODE_CURVE_TOPOLOGY_POINTS_OF_CURVE 1179
#define GEO_NODE_MESH_TOPOLOGY_OFFSET_CORNER_IN_FACE 1180
#define GEO_NODE_MESH_TOPOLOGY_CORNERS_OF_EDGE 2103
Member

Should have been clearer here. This new define should go at the end of the list, below GEO_NODE_INPUT_SIGNED_DISTANCE

Should have been clearer here. This new define should go at the end of the list, below `GEO_NODE_INPUT_SIGNED_DISTANCE`
Author
Contributor

I see, thought i would be better to keep the names together, but I understand

I see, thought i would be better to keep the names together, but I understand
F_Scociety marked this conversation as resolved
Soeren Schmidt-Clausen added 1 commit 2023-05-16 22:41:41 +02:00
b97975776e Update source/blender/blenkernel/BKE_node.h
moved node define to be more organized
Soeren Schmidt-Clausen requested review from Hans Goudey 2023-05-17 15:37:41 +02:00
Soeren Schmidt-Clausen added 1 commit 2023-05-18 18:37:10 +02:00
Hans Goudey approved these changes 2023-05-18 18:37:46 +02:00
Hans Goudey left a comment
Member

Thanks! I'll check with Jacques before committing this, but I think it looks good.

Thanks! I'll check with Jacques before committing this, but I think it looks good.
Soeren Schmidt-Clausen added 1 commit 2023-05-23 12:27:06 +02:00
Hans Goudey changed title from GeometryNodes: Add "Corners of Edge" node to Geometry Nodes: Add "Corners of Edge" node 2023-05-23 16:31:22 +02:00
Hans Goudey reviewed 2023-05-23 17:13:09 +02:00
@ -0,0 +11,4 @@
static void node_declare(NodeDeclarationBuilder &b)
{
b.add_input<decl::Int>(N_("Edge Index"))
Member

Jacques and I discussed the socket descriptions a bit. Some suggestions:

  • Weights: Values used to sort the corners using the edge directly
  • Corner Index: A corner followed by the input edge in its face's winding order, chosen by the sort index
Jacques and I discussed the socket descriptions a bit. Some suggestions: - Weights: `Values used to sort the corners using the edge directly` - Corner Index: `A corner followed by the input edge in its face's winding order, chosen by the sort index`
Author
Contributor

Hm, What about:

  • Weights: Values that sort the corners attached to the edge
  • Corner Index: A corner of the input edge in its face's winding order, chosen by the sort index

Iam not sure if the current description for the weights socket has to be changed at all: Values used to sort corners attached to the edge. Uses indices by default

Hm, What about: - Weights: `Values that sort the corners attached to the edge` - Corner Index: `A corner of the input edge in its face's winding order, chosen by the sort index` Iam not sure if the current description for the weights socket has to be changed at all: `Values used to sort corners attached to the edge. Uses indices by default`
Member

Yeah, those work fine for me too!

Yeah, those work fine for me too!
F_Scociety marked this conversation as resolved
Soeren Schmidt-Clausen added 3 commits 2023-05-30 16:07:11 +02:00
Author
Contributor

Got the latest main today, and saw that Jacques Lucke did a refactor, which changed the things a bit around, so I adapter the code. Everything seems to work again.

Got the latest main today, and saw that Jacques Lucke did a refactor, which changed the things a bit around, so I adapter the code. Everything seems to work again.
Soeren Schmidt-Clausen requested review from Hans Goudey 2023-05-30 16:10:04 +02:00
Soeren Schmidt-Clausen added 1 commit 2023-05-30 21:11:22 +02:00
Soeren Schmidt-Clausen added 1 commit 2023-05-31 10:03:06 +02:00
Soeren Schmidt-Clausen added 1 commit 2023-05-31 10:08:36 +02:00
Hans Goudey merged commit fdc3ed798d into main 2023-05-31 15:25:54 +02:00
Soeren Schmidt-Clausen deleted branch corners_of_edge 2023-06-02 16:37:17 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107968
No description provided.