Geometry Nodes: align input and output sockets in simulation and repeat zone nodes #118335

Jacques Lucke · 2024-02-15T16:47:55+01:00

Jacques Lucke commented

2024-02-15 16:47:55 +01:00

This changes the drawing of the zone nodes to align corresponding input and output sockets. The resulting nodes are smaller and it's easier to see how data is passed through them.

Drawing aligned sockets is already technically supported for quite a while already, but we haven't used it so far. Using them for zone nodes seems to provide benefits only. How we use aligned sockets in other nodes still has to be discussed more.

This patch only changes run-time data. It doesn't affect what is written to .blend files.

In the node declaration, aligned sockets are created by tagging a socket so that it is aligned with the previous socket. This is a bit different from what we had before where a single socket declaration would be used for an input and output socket. I think the approach used here works better, especially in a potential future scenario where the input and output socket has a different type.

Before:

After:

This changes the drawing of the zone nodes to align corresponding input and output sockets. The resulting nodes are smaller and it's easier to see how data is passed through them. Drawing aligned sockets is already technically supported for quite a while already, but we haven't used it so far. Using them for zone nodes seems to provide benefits only. How we use aligned sockets in other nodes still has to be discussed more. This patch only changes run-time data. It doesn't affect what is written to .blend files. In the node declaration, aligned sockets are created by tagging a socket so that it is aligned with the previous socket. This is a bit different from what we had before where a single socket declaration would be used for an input and output socket. I think the approach used here works better, especially in a potential future scenario where the input and output socket has a different type. Before: ![image](/attachments/1a6b1720-059e-4ab1-8a01-061bb5daad60) After: ![image](/attachments/ab756d37-8a4a-413e-b322-bbacaa06c7d8)

image.png

40 KiB

image.png

30 KiB

🎉 7

Jacques Lucke added 2 commits 2024-02-15 16:48:06 +01:00

110f487ab6 cleanup

9e3bb76e1b fix

Jacques Lucke requested review from Dalai Felinto 2024-02-15 16:53:50 +01:00

Jacques Lucke requested review from Simon Thommes 2024-02-15 16:53:51 +01:00

Jacques Lucke requested review from Hans Goudey 2024-02-15 16:53:51 +01:00

Jacques Lucke requested review from Lukas Tönne 2024-02-15 16:54:00 +01:00

Lukas Tönne requested changes 2024-02-15 17:10:50 +01:00

Lukas Tönne left a comment

Code-wise the change for aligned sockets looks fine, nice to see the feature still works.
I don't agree with removing the is_valid check (see inline comment), should probably be a separate PR.
Design-wise i'd like to confirm with UI team or whoever is responsible, last time adding the feature didn't end so well and it had to be disabled again.

Code-wise the change for aligned sockets looks fine, nice to see the feature still works. I don't agree with removing the `is_valid` check (see inline comment), should probably be a separate PR. Design-wise i'd like to confirm with UI team or whoever is responsible, last time adding the feature didn't end so well and it had to be disabled again.

source/blender/nodes/NOD_node_declaration.hh Outdated

						
				@ -467,4 +475,2 @@

				  friend NodeDeclarationBuilder;

				  /** Returns true if the declaration is considered valid. */

				  bool is_valid() const;

Lukas Tönne commented

2024-02-15 17:06:29 +01:00

Not sure i agree with removing this. It tests for socket/panel ordering which is not enforced through syntax alone. As a simple test move the Principled BSDF output down one line - it will trip the validity check here.
1bf6d8b0b9/source/blender/nodes/shader/nodes/node_shader_bsdf_principled.cc (L27)

We could maybe move this to a utility function, but i think it's worth keeping. In any case, it's also an unrelated change.

Not sure i agree with removing this. It tests for socket/panel ordering which is not enforced through syntax alone. As a simple test move the Principled BSDF output down one line - it will trip the validity check here. https://projects.blender.org/blender/blender/src/commit/1bf6d8b0b905fbac4235e2ac8329538997a03d38/source/blender/nodes/shader/nodes/node_shader_bsdf_principled.cc#L27 We could maybe move this to a utility function, but i think it's worth keeping. In any case, it's also an unrelated change.

JacquesLucke marked this conversation as resolved

Hans Goudey approved these changes 2024-02-15 17:48:31 +01:00

Dalai Felinto commented

2024-02-15 19:01:39 +01:00

If @pablovazquez is fine with it I'm okay.

Thanks for keeping me in the loop.

If @pablovazquez is fine with it I'm okay. Thanks for keeping me in the loop.

👍 1

Pablo Vazquez approved these changes 2024-02-15 19:17:27 +01:00

Pablo Vazquez left a comment

+1. Glad to see this functionality finally in use.

I hope at some point we reconsider to do it for other nodes.

+1. Glad to see this functionality finally in use. I hope at some point we reconsider to do it for other nodes.

Jacques Lucke added 2 commits 2024-02-15 20:51:31 +01:00

1a69e9e5bb Merge branch 'main' into inline-zone-sockets

2a22e2eb03 bring back is_valid function, only selectively disable parts of it

Jacques Lucke requested review from Lukas Tönne 2024-02-15 20:51:52 +01:00

Lukas Tönne approved these changes 2024-02-15 22:41:22 +01:00

Simon Thommes reviewed 2024-02-16 11:29:31 +01:00

Simon Thommes left a comment

Since we went over the whole inline sockets idea before, I feel like it would make sense to already consider the general order of sockets that we ended up with. The it is right now would mean there are non-inlined sockets before inlined ones in the same panel.

So I'd suggest to put the inlined sockets into a panel like we discussed then.

But I don't feel strongly about this, since this would be the only nodes so far and we don't really do panels in builtin nodes yet. Just a suggestion.

Since we went over the whole inline sockets idea before, I feel like it would make sense to already consider the general order of sockets that we ended up with. The it is right now would mean there are non-inlined sockets before inlined ones in the same panel. So I'd suggest to put the inlined sockets into a panel like we discussed then. But I don't feel strongly about this, since this would be the only nodes so far and we don't really do panels in builtin nodes yet. Just a suggestion.

Manuel Grad commented

2024-02-16 11:48:37 +01:00

First-time contributor

How about always putting Geometry at the top like so?:

How about always putting Geometry at the top like so?: ![align.jpg](/attachments/d0010a6d-d09d-4428-801b-24e7a34487a9)

align.jpg

53 KiB

Simon Thommes commented

2024-02-16 11:56:56 +01:00

How about always putting Geometry at the top like so?:

I wouldn't do this for multiple reasons:

there's nothing inherently special about the first geometry socket, since there can be multiple
this disconnects the attributes stored on the geometry from the geometry socket itself
this makes the readability worse with single in/out sockets being in between inline sockets, which we should not do generally

> How about always putting Geometry at the top like so?: > > ![align.jpg](/attachments/d0010a6d-d09d-4428-801b-24e7a34487a9) I wouldn't do this for multiple reasons: - there's nothing inherently special about the first geometry socket, since there can be multiple - this disconnects the attributes stored on the geometry from the geometry socket itself - this makes the readability worse with single in/out sockets being in between inline sockets, which we should not do generally

👍 2

Jacques Lucke commented

2024-02-16 16:18:46 +01:00

So I'd suggest to put the inlined sockets into a panel like we discussed then.

Fine with me, do you have a suggestion for how the panels should be called?

> So I'd suggest to put the inlined sockets into a panel like we discussed then. Fine with me, do you have a suggestion for how the panels should be called?

Hans Goudey commented

2024-02-16 16:22:03 +01:00

IMO putting all the sockets into panels doesn't really improve the design, it just takes up extra space. I don't see people wanting to collapse them, and they aren't some different concept that needs an explanatory label.

I think we should define the guidelines around what looks and works best in real situations. Adding panels here seems more like the other way around-- we currently have the guideline for more abstract reasons of "consistency".

IMO putting all the sockets into panels doesn't really improve the design, it just takes up extra space. I don't see people wanting to collapse them, and they aren't some different concept that needs an explanatory label. I think we should define the guidelines around what looks and works best in real situations. Adding panels here seems more like the other way around-- we currently have the guideline for more abstract reasons of "consistency".

Jacques Lucke commented

2024-02-19 13:23:14 +01:00

Talked to Simon. It's ok to not at panels right now. Maybe something we want to do later as we gain more experience and have more cases to be consistent with.

Jacques Lucke merged commit 1f30f41af8 into main

2024-02-19 13:32:09 +01:00

Jacques Lucke referenced this issue from a commit

2024-02-19 13:32:11 +01:00

Geometry Nodes: align input and output sockets in simulation and repeat zone nodes

Jacques Lucke deleted branch inline-zone-sockets

2024-02-19 13:32:11 +01:00

Jacques Lucke referenced this pull request

2024-05-06 19:19:59 +02:00

Geometry Nodes: use aligned sockets in bake node #121496

Jacques Lucke referenced this issue from a commit

2024-05-07 11:23:35 +02:00