This patch tweaks how Virtual Sockets are displayed to be better recognizable.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- geometry_nodes_virtual_sockets_color (branched from master)
- Build Status
Buildable 12588 Build 12588: arc lint + arc unit
Event Timeline
Comment Actions
Not sure if it's WIP or not..
| source/blender/editors/space_node/drawnode.c | ||
|---|---|---|
| 3834–3835 | consider copy_v4_v4 . Also, instead of magic numbers, define this and virtual one as const RGBA representation somewhere nearby. | |
| source/blender/editors/space_node/node_draw.c | ||
| 804–807 | If only idname is needed, consider asking for just that. | |
| 820 | Use STREQLEN macro | |
Comment Actions
nitpicks
| source/blender/editors/space_node/drawnode.c | ||
|---|---|---|
| 3648 | I guess clang-format (run by make format) could align this. | |
| source/blender/editors/space_node/node_draw.c | ||
| 805 | selected can be const too. | |
| 805 | socked_idname -> socket_idname | |
| 818 | Needs space before Until. | |
| 818 | Needs period after "here". | |
| source/blender/nodes/intern/node_common.c | ||
| 604 ↗ | (On Diff #32664) | unrelated change |
Comment Actions
@Pablo Vazquez (pablovazquez) You might want to have a look here.
Visually this is a nice improvement, the current socket looks pretty bad.
However I'm not a fan of the way this is implemented, sticking a string comparison in low level code like this is probably not the way to go.
I wonder if adding a new socket shape and using it here would make sense.
| source/blender/editors/space_node/node_relationships.c | ||
|---|---|---|
| 52–53 ↗ | (On Diff #32895) | This snuck in again. |
Comment Actions
Great improvement! Visually it's a +1 from me, code-wise I leave it to you @Hans Goudey (HooglyBoogly)
Thanks for working on it Fabian!
Comment Actions
@Hans Goudey (HooglyBoogly) Just let me add some context:
The problem space here is how we get a different outline color for an individual socket. The fill color is returned by it draw_color function of each socket. That function samples the color from an Array based on the socket->type. The color for the outline is fixed at the moment and just uses the themes font color.
My first Idea was to add an draw_outline_color function to bNodeSocket simillar to the draw_color function. That would in my opinion be the best way to do it, but I thought maybe thats a bit overdone for this patch.
Second "problem" is that the socket type of "Virtual Socket" is -1 so we can't use it as index for a color in an array.
But.. What I don't understand myself is, why I haven't just compared the socket->type == -1 instead of comparing the id_name string. Would that be a better solution for you?
Comment Actions
- Reverted unneeded changes to node_relationship.c
- Replaced string compare of socket->id_name by int compare socket->type to identify virtual socket.
Comment Actions
Sorry. I seems that i have accidently mixed up some branches. Have to disentangle them before uploading a new diff.
Comment Actions
- Disentangled mixup with base branch.
- Changed socket comparision from socket->id_name to socket->type
Comment Actions
However I'm not a fan of the way this is implemented, sticking a string comparison in low level code like this is probably not the way to go.
@Hans Goudey (HooglyBoogly) I changed socket comparison from STREQLEN() to (socket_type == -1). Please tell me if you like that better.
Comment Actions
Seems more reasonable now. Still too bad about the extra special case but I don't see a good way to avoid it.
| source/blender/editors/space_node/node_draw.c | ||
|---|---|---|
| 804–808 | Does this compile? Looks like you should be passing int rather than int * | |
Comment Actions
Could it be that this patch makes sockets lose anti-alising? I'm testing the zip bundle patch from D10067
Maybe it's a different patch from the bundle. I'm a bit lost which patch does what.
At the bottom is how they look in the temp-nodes-redesign branch, which in terms of sockets it's basically same as master but with a thicker outline and fully opaque color (instead of the current half-transparent).
Comment Actions
node_draw.c
- 32 - your changes do not require adding BKE_curve.h
- 819 - socket_type comparison to "-1" could use enum SOCK_CUSTOM
- 886 - avoid adding new lines that are not associated with your changes
Comment Actions
Interresting! Good you found this. Haven't noticed yet. On my hi-dpi monitor its not really visible. This is for sure a problem with another patch => D10181 because of replacing the shader with a new one to be able to draw the new shape for Multi Input Sockets. I will fix this soon in https://developer.blender.org/D10181
This comment was removed by Fabian Schempp (fabian_schempp).