Geometry Nodes: Menu Switch Node #113445

Merged
Lukas Tönne merged 190 commits from LukasTonne/blender:nodes-menu-switch into main 2024-01-26 12:40:14 +01:00
Member

This patch adds support for Menu Switch nodes and enum definitions in node trees more generally. The design is based on the outcome of the 2022 Nodes Workshop.

The Menu Switch node is an advanced version of the Switch node which has a customizable menu input socket instead of a simple boolean. The items of this menu are owned by the node itself. Each item has a name and description and unique identifier that is used internally. A menu socket represents a concrete value out of the list of items.

Screenshot_20240125_160838
Menu Switch node selecting between geometries

Screenshot_20231013_155848
Enum definition in the node editor side bar

image
Enum exposed in a node group

To enable selection of an enum value for unconnected sockets the menu is presented as a dropdown list like built-in enums. When the socket is connected a shared pointer to the enum definition is propagated along links and stored in socket default values. This allows node groups to expose a menu from an internal menu switch as a parameter. The enum definition is a runtime copy of the enum items in DNA that allows sharing.

A menu socket can have multiple connections, which can lead to ambiguity. If two or more different menu source nodes are connected to a socket it gets marked as undefined. Any connection to an undefined menu socket is invalid as a hint to users that there is a problem. A warning/error is also shown on nodes with undefined menu sockets.

image
Ambiguous switch node connection

At runtime the value of a menu socket is the simple integer identifier. This can also be a field in geometry nodes. The identifier is unique within each enum definition, and it is persistent even when items are added, removed, or changed. Changing the name of an item does not affect the internal identifier, so users can rename enum items without breaking existing input values. This also persists if, for example, a linked node group is temporarily unavailable.

This patch adds support for _Menu Switch_ nodes and enum definitions in node trees more generally. The design is based on the outcome of the [2022 Nodes Workshop](https://code.blender.org/2022/11/geometry-nodes-workshop-2022/#menu-switch). The _Menu Switch_ node is an advanced version of the _Switch_ node which has a customizable **menu input socket** instead of a simple boolean. The _items_ of this menu are owned by the node itself. Each item has a name and description and unique identifier that is used internally. A menu _socket_ represents a concrete value out of the list of items. <table><tr> <td> ![Screenshot_20240125_160838](/attachments/24227c16-8805-414e-8f31-b233c8c2da33) *Menu Switch node selecting between geometries* </td> <td> ![Screenshot_20231013_155848](/attachments/e27e2100-0f4c-434e-ac3b-5801f0f2901d) *Enum definition in the node editor side bar* </td> <td> ![image](/attachments/f4039257-bf3b-4b12-831f-4b5736a02f7f) *Enum exposed in a node group* </td> </tr></table> To enable selection of an enum value for unconnected sockets the menu is presented as a dropdown list like built-in enums. When the socket is connected a shared pointer to the enum definition is propagated along links and stored in socket default values. This allows node groups to expose a menu from an internal menu switch as a parameter. The enum definition is a runtime copy of the enum items in DNA that allows sharing. A menu socket can have multiple connections, which can lead to ambiguity. If two or more different menu source nodes are connected to a socket it gets marked as _undefined_. Any connection to an undefined menu socket is invalid as a hint to users that there is a problem. A warning/error is also shown on nodes with undefined menu sockets. ![image](/attachments/160162e1-9781-4545-acb6-b317cb07322c) *Ambiguous switch node connection* At runtime the value of a menu socket is the simple integer identifier. This can also be a field in geometry nodes. The identifier is unique within each enum definition, and it is persistent even when items are added, removed, or changed. Changing the name of an item does not affect the internal identifier, so users can rename enum items without breaking existing input values. This also persists if, for example, a linked node group is temporarily unavailable.
Lukas Tönne added 19 commits 2023-10-09 12:32:16 +02:00
Lukas Tönne added 5 commits 2023-10-09 13:51:43 +02:00
77466127ff WIP: Added new socket type to represent enum values.
Still needs implementation for RNA, declarations, and modifier
integration.

The socket stores an int identifier as a default input value, as well as
a weak reference to the enum definition (node tree pointer + node
identifier). Identifiers for enum values are unique and only used once,
so that library-linked files can be changed without breaking enum values
or creating ambiguous identifiers.

The reference will be updated by the NodeTreeMainUpdater
when node connections change. Enum references propagate right-to-left,
taking care to verify that all connections use the same enum or the
reference becomes invalid.

The reference can be resolved at runtime to get the full list of enum
values. This will be wrapped in a RNA enum as well. The IDProperty on
the modifier side will probably be a simple string, at least for now.
Lukas Tönne added 3 commits 2023-10-09 14:57:34 +02:00
Lukas Tönne added 3 commits 2023-10-10 12:50:26 +02:00
Lukas Tönne added 1 commit 2023-10-10 14:28:39 +02:00
Lukas Tönne added 2 commits 2023-10-10 17:40:18 +02:00
Lukas Tönne added 1 commit 2023-10-10 18:40:08 +02:00
Lukas Tönne added 1 commit 2023-10-10 22:44:24 +02:00
Lukas Tönne added 2 commits 2023-10-11 09:06:02 +02:00
Lukas Tönne added 1 commit 2023-10-11 09:46:32 +02:00
Lukas Tönne added 1 commit 2023-10-11 10:43:35 +02:00
Lukas Tönne added 1 commit 2023-10-11 10:49:37 +02:00
Lukas Tönne added 1 commit 2023-10-11 11:11:11 +02:00
7aa5853e53 Fix inconsistent lazy function for menu switch socket usage.
This cannot be a static lazy function because the enum items count is
variable.
Lukas Tönne added 2 commits 2023-10-11 11:45:12 +02:00
Lukas Tönne added 3 commits 2023-10-12 16:13:35 +02:00
Lukas Tönne added 2 commits 2023-10-13 15:45:57 +02:00
Lukas Tönne added 1 commit 2023-10-13 16:04:10 +02:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113445) when ready.

I think the boolean should also expose the boolean value in the socket, right now I have to use a Boolean node:

image


Real example (Layer selector for my tests):

image

I think the boolean should also expose the boolean value in the socket, right now I have to use a Boolean node: ![image](/attachments/27902e42-cdb8-4d56-869b-baee89943bdb) --- Real example (Layer selector for my tests): ![image](/attachments/1af56c5e-718a-41ef-889c-7f92e9f15a3b)

This has so much potential. Good job already, it is looking great.

And here an example file where I would expect to see both enum options, but that the UI indicates an "error":

image

The file is here: menu-node.blend


The fact that the first one doesn't show seems a bug even with the current design. It happens when I have two enums, and connect each one in a different Group Input node.

That aside, I would actually expect both enums to work:

image

Specially in this case since they all come from the same Node Group. But probably even if they were from different node groups and simply share the "names".

This has so much potential. Good job already, it is looking great. And here an example file where I would expect to see both enum options, but that the UI indicates an "error": ![image](/attachments/b0769674-a179-4b54-8ec5-67ebad68387c) The file is here: [menu-node.blend](/attachments/02ab96ce-51f6-481f-b22e-0aedefb591f6) --- The fact that the first one doesn't show seems a bug even with the current design. It happens when I have two enums, and connect each one in a different Group Input node. That aside, I would actually expect both enums to work: ![image](/attachments/01bdf664-c922-4534-a390-38cc42b701bb) Specially in this case since they all come from the same Node Group. But probably even if they were from different node groups and simply share the "names".
Lukas Tönne added 3 commits 2023-10-16 10:46:44 +02:00
Author
Member

I think the boolean should also expose the boolean value in the socket, right now I have to use a Boolean node:

Fixed - this was copied from the Switch node where i guess having 3 boolean input buttons could get a bit confusing and it only really makes sense when the inputs are fields. For an enum selector it's more useful, in the sense of "Return true for a subset of enum values".

> I think the boolean should also expose the boolean value in the socket, right now I have to use a Boolean node: Fixed - this was copied from the Switch node where i guess having 3 boolean input buttons could get a bit confusing and it only really makes sense when the inputs are fields. For an enum selector it's more useful, in the sense of "Return true for a subset of enum values".
Lukas Tönne added 1 commit 2023-10-16 11:23:05 +02:00
01c75e2041 Fix enum propagation to node group interfaces.
When multiple group input nodes are used and some of them have "invalid"
references (not yet set) they should not reset existing references.
Otherwise the success of propagation will be order-dependent.
Author
Member

And here an example file where I would expect to see both enum options, but that the UI indicates an "error"

Fixed. It was indeed order-dependent because one group input could reset the enum from a previous group input.

> And here an example file where I would expect to see both enum options, but that the UI indicates an "error" Fixed. It was indeed order-dependent because one group input could reset the enum from a previous group input.
Lukas Tönne added 1 commit 2023-10-16 11:59:36 +02:00
Lukas Tönne added 2 commits 2023-10-16 12:32:29 +02:00
Lukas Tönne added 1 commit 2023-10-16 12:40:34 +02:00
Lukas Tönne added 1 commit 2023-10-16 12:58:45 +02:00
Lukas Tönne added 1 commit 2023-10-16 13:04:54 +02:00
Lukas Tönne added 1 commit 2023-10-16 16:25:41 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
fedf726642
Make the bNodeTree pointer in enum sockets into a truly weak reference.
No refcounting is needed for this pointer, we do not want to keep the
node tree alive through an enum socket reference. Currently the tree
is always referenced by an internal node group that the socket is part
of, but even without that the enum reference should be allowed to become
invalid.
Member

Haven't tried it yet, but based on the images it looks great.

Personally, I don't think we should be using a new socket shape here. To me, enums are not different enough compared to other single values to justify that. In many ways they are just a better integer or string. I know that this was part of the original mockups. I think we should just use a colored circle socket shape here. Maybe you can just add that as a discussion point in the todo list?

Haven't tried it yet, but based on the images it looks great. Personally, I don't think we should be using a new socket shape here. To me, enums are not different enough compared to other single values to justify that. In many ways they are just a better integer or string. I know that this was part of the original mockups. I think we should just use a colored circle socket shape here. Maybe you can just add that as a discussion point in the todo list?

Just to add the rational for the different socket type here. Though the discussion itself is to be held elsewhere:

This socket works very differently than the other ones. It goes right-to-left, as well as left-to-right.

I think we should even show the link (the connecting line) differently as well. But for now having at least the socket helps to tell it apart.

Just to add the rational for the different socket type here. Though the discussion itself is to be held elsewhere: This socket works very differently than the other ones. It goes right-to-left, as well as left-to-right. I think we should even show the link (the connecting line) differently as well. But for now having at least the socket helps to tell it apart.
Lukas Tönne added 1 commit 2023-10-17 19:18:31 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
bf2402c1ca
Merge branch 'main' into nodes-menu-switch

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113445) when ready.
Member

Here are my reasons for not using a different socket shapes or link drawing:

  • Enums behave just like other "primitive types" (e.g. float) in that they exist as single values, as fields and potentially as lists, images and grids. We'd probably need special visual language for those as well then, which adds another layer of complexity. Oh and that not only applies to socket shapes, but also links for which we use visual language to differentiate single values, fields and in the future maybe multi-links ("udons") and dynamically typed links.
  • Enum do have a right-to-left component at inference time, but the data during evaluation also just flows left-to-right just like any other data.
  • The right-to-left behavior is quite comparable, even if not exactly the same, to propagating socket subtypes (e.g. float socket units). It's also similar to how attribute search in string sockets work (or at least should work). Another case where we have this already is that single-value inputs propagate backwards. All that is to say that this behavior is less special than it may seem at first. And it will likely also become less special over time as nodes become more dynamic and more inferencing becomes necessary.
  • Enums do get a new color which already shows that it is something different.
  • Even if we would find a solution that makes the enum socket shape work for fields, lists, etc., I still think it's not worth it. That is because I expect the number of enum links to be very small compared to other types. So it opens up a huge design problem with very little practictal benefit.
Here are my reasons for not using a different socket shapes or link drawing: * Enums behave just like other "primitive types" (e.g. float) in that they exist as single values, as fields and potentially as lists, images and grids. We'd probably need special visual language for those as well then, which adds another layer of complexity. Oh and that not only applies to socket shapes, but also links for which we use visual language to differentiate single values, fields and in the future maybe multi-links ("udons") and dynamically typed links. * Enum do have a right-to-left component at inference time, but the data during evaluation also just flows left-to-right just like any other data. * The right-to-left behavior is quite comparable, even if not exactly the same, to propagating socket subtypes (e.g. float socket units). It's also similar to how attribute search in string sockets work (or at least should work). Another case where we have this already is that single-value inputs propagate backwards. All that is to say that this behavior is less special than it may seem at first. And it will likely also become less special over time as nodes become more dynamic and more inferencing becomes necessary. * Enums do get a new color which already shows that it is something different. * Even if we would find a solution that makes the enum socket shape work for fields, lists, etc., I still think it's not worth it. That is because I expect the number of enum links to be very small compared to other types. So it opens up a huge design problem with very little practictal benefit.
Lukas Tönne added a new dependency 2023-11-01 11:26:38 +01:00
Lukas Tönne added 1 commit 2023-11-01 11:31:53 +01:00
Lukas Tönne added 1 commit 2023-11-01 11:57:17 +01:00
Lukas Tönne added 28 commits 2023-11-01 16:42:05 +01:00
736bb2f952 New python type `idprop.types.EnumValue`.
This is used to disambiguate ID property types on assignment.
A plain integer value will always create an integer ID property, so this
new type allows creating enum ID properties specifically.
266dec7c9e Implement RNA enum item list in rna_access instead of a regular callback.
The callback does not actually work because the data pointer is always
the data that owns the IDProperty rather than the property itselfs.
Since this can be added to any ID or similar property-supporting types
there is no general way of finding the actual IDProperty.
fd933ee7cb Implement RNA enum item list in rna_access instead of a regular callback.
The callback does not actually work because the data pointer is always
the data that owns the IDProperty rather than the property itselfs.
Since this can be added to any ID or similar property-supporting types
there is no general way of finding the actual IDProperty.
buildbot/vexp-code-patch-coordinator Build done. Details
d29aa21b1e
Enable property updates for enum sockets even without interface_changed.
The `interface_changed` flag is only set for certain updates like socket
changes or input property changes. Adding/removing enum items does not
set this flag. To reflect enum item changes in the modifier the tree
update has to allow id property updates for enum sockets even when the
interface_changed flag is not set.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113445) when ready.
Dalai Felinto reviewed 2023-11-02 17:07:56 +01:00
Dalai Felinto left a comment
Owner

Hi Lukas, nice work. Another round of bugs/findings:

  • 1. Items in the Add Menu should be in alphabetical order (which is not the case at the moment):

image

  • 2. Item should be called "Menu", not Enum:

image

  • 3. Crash when dragging/adding the socket from the Group Input which is already connected to a Menu Switch

image

- [ ] 4. Drawing glitch sometimes: reported separately #114461

  • 5. Would be possible to show a message like "No items added"?

image

Hi Lukas, nice work. Another round of bugs/findings: - [x] 1. Items in the Add Menu should be in alphabetical order (which is not the case at the moment): ![image](/attachments/dcd85fee-b71e-4c7d-9d73-571aa5cc4485) - [x] 2. Item should be called "Menu", not Enum: ![image](/attachments/1a516749-a5cb-4c5a-8c96-29848297c0dc) - [x] 3. Crash when dragging/adding the socket from the Group Input which is already connected to a Menu Switch ![image](/attachments/790bd440-ea66-4def-8bc6-8bb443490a3c) ~~- [ ] 4. Drawing glitch sometimes:~~ reported separately #114461 - [x] 5. Would be possible to show a message like "No items added"? ![image](/attachments/c662d93a-2468-4cf7-8100-39ada1b748ab)

I'm also not in love with calling the socket "Switch":

image

Maybe we could call it "Menu" ?

I'm also not in love with calling the socket "Switch": ![image](/attachments/0c1c9277-a488-4fd5-99a0-682a521f15a8) Maybe we could call it "Menu" ?
Lukas Tönne added 4 commits 2023-11-03 11:14:54 +01:00
Lukas Tönne reviewed 2023-11-03 11:34:12 +01:00
@ -103,4 +103,17 @@ class StringBakeItem : public BakeItem {
}
};
class EnumBakeItem : public BakeItem {
Author
Member

Can just use the PrimitiveBakeItem for this, it just stores an int.

Can just use the PrimitiveBakeItem for this, it just stores an int.
Lukas Tönne reviewed 2023-11-03 11:38:54 +01:00
@ -993,2 +996,4 @@
break;
}
case SOCK_ENUM:
uiItemL(sub, "ENUM SOCKET TODO", ICON_NONE);
Author
Member

Implement menu dropdown for material views.

Implement menu dropdown for material views.
Lukas Tönne added 3 commits 2023-11-03 11:50:26 +01:00
2190ac24f4 Renamed "Enum" socket type to "Menu".
The internal types for enum definitions and id properties are still
called enums, since that is the technical correct term (compare also:
"rotation" socket vs. quaternion data type).
Lukas Tönne added 2 commits 2023-11-03 12:01:31 +01:00
16db837216 Fix outdated socket type name.
This crashes because the enum references are not getting cleared.
Author
Member

Crash when dragging/adding the socket from the Group Input which is already connected to a Menu Switch

The node that shows up in the link search here is the regular boolean Switch node, not the Menu Switch. Menu switch does not currently supply any link search results, that's a todo.

The reason for the crash is a bit ugly: When the switch node provides a search result it sets the data type of the prospective Switch to whatever the dragged socket type is - ignoring that the new Menu socket type is not actually supported (yet). When the node is then created it does not find a matching input socket in its set of inputs that it could make available. Subsequently trying to make a link to the unavailable socket fails and crashes.

  1. Switch node should be more careful about what sockets it accepts, and not show link options for unsupported types.
  2. Link drag operator should be more robust and make sure that a socket actually exists before trying to make links. Of course nodes should not provide invalid search results, but the operator should also handle broken cases with appropriate error messages instead of crashing.
  3. Eventually Switch node should support the Menu socket type as well. Other generic nodes also need updates for this.

EDIT: Actually the crash is just an assert and the link-drag operator itself is working correctly. The nodes themselves declare that a socket is available. It's just the switch node which assumes that it can handle all socket types, so fixing that should be enough.

EDIT 2: Fixed now. The Switch node has an explicit list of supported socket types now, so adding a new socket type in future should not immediately break the Switch node. Menu sockets are also supported now.
Screenshot_20231103_134422
(note: field inferencing for the output socket seems broken, looking into it)

> Crash when dragging/adding the socket from the Group Input which is already connected to a Menu Switch The node that shows up in the link search here is the regular boolean `Switch` node, not the `Menu Switch`. Menu switch does not currently supply any link search results, that's a todo. The reason for the crash is a bit ugly: When the switch node provides a search result it sets the data type of the prospective `Switch` to whatever the dragged socket type is - ignoring that the new `Menu` socket type is not actually supported (yet). When the node is then created it does not find a matching input socket in its set of inputs that it could make available. Subsequently trying to make a link to the unavailable socket fails and crashes. 1. Switch node should be more careful about what sockets it accepts, and not show link options for unsupported types. 2. Link drag operator should be more robust and make sure that a socket actually exists before trying to make links. Of course nodes should not provide invalid search results, but the operator should also handle broken cases with appropriate error messages instead of crashing. 3. Eventually Switch node should support the `Menu` socket type as well. Other generic nodes also need updates for this. EDIT: Actually the crash is just an assert and the link-drag operator itself is working correctly. The nodes themselves declare that a socket is available. It's just the switch node which assumes that it can handle all socket types, so fixing that should be enough. EDIT 2: Fixed now. The Switch node has an explicit list of supported socket types now, so adding a new socket type in future should not immediately break the Switch node. Menu sockets are also supported now. ![Screenshot_20231103_134422](/attachments/4c75875c-37c4-48d5-a2ee-8c2f0177ad85) (note: field inferencing for the output socket seems broken, looking into it)
Author
Member

Drawing glitch sometimes:

I can't repro this. Maybe it only happened for the square sockets? Could be hardware- or driver-dependent.

> Drawing glitch sometimes: I can't repro this. Maybe it only happened for the square sockets? Could be hardware- or driver-dependent.
Lukas Tönne added 1 commit 2023-11-03 12:33:40 +01:00
Author
Member

Would be possible to show a message like "No items added"?

I just added a label "No Items" for now. The details can be a bit fiddly, i'm also showing the label here, which we don't do for the dropdown (which then only shows "Default Value" for reasons explained before). It's a bit difficult to get it consistent in all cases.

Screenshot_20231103_123303

> Would be possible to show a message like "No items added"? I just added a label "No Items" for now. The details can be a bit fiddly, i'm also showing the label here, which we don't do for the dropdown (which then only shows "Default Value" for reasons explained before). It's a bit difficult to get it consistent in all cases. ![Screenshot_20231103_123303](/attachments/e3d59ce8-f77c-4d66-ad3f-10e0f133338a)
Lukas Tönne added 1 commit 2023-11-03 12:38:06 +01:00
Lukas Tönne added 1 commit 2023-11-03 13:41:40 +01:00
33c2353c59 Fix the Switch node to avoid crashing with unsupported socket types.
Added an explicit list of supported socket types, so that adding a new
node socket type will not immediately break the switch node.

Menu sockets are supported now.
Lukas Tönne added 1 commit 2023-11-03 14:07:46 +01:00
Lukas Tönne added 2 commits 2023-11-03 15:43:41 +01:00
dd655bcd3c Fix various crashes caused by static multifunctions in Menu Switch node.
Unlike most other node multifunctions, the menu switch function depends
on the concrete enum definition. The input value is compared to the item
identifier values, which are generally different for each instance of
the node. So the multifunction and its signature have to be constructed
on a case-by-case basis and cannot be constructed in advance.
buildbot/vexp-code-patch-coordinator Build done. Details
a6aa7a3f94
Yet another ELEM list missing menu sockets.
  • 1. Crash if I use ctrl+scrollwheel to go beyond the range of the menu (only happens in the modifier UI): primitive.blend

  • 2. Crash if I mouse over an empty menu in the Modifier UI: empty_menu.blend

In this case I don't think we should even show the menu there to be honest, or it should show the red warning as when using a non-existent attribute.

  • 3. When we delete a menu Item which is the default, we can end up with the Default being blank: error.blend

image

  • 4. On a similar note, if the Modifier had a value set which no longer exists it shows blank and will crash (same reason as (2)): error.blend
- [x] 1. Crash if I use ctrl+scrollwheel to go beyond the range of the menu (only happens in the modifier UI): [primitive.blend](/attachments/58d27ab4-c8aa-4db1-8613-a23af2317419) - [x] 2. Crash if I mouse over an empty menu in the Modifier UI: [empty_menu.blend](/attachments/04f10a59-3c81-405f-96f5-6d44eadb33f1) In this case I don't think we should even show the menu there to be honest, or it should show the red warning as when using a non-existent attribute. - [ ] 3. When we delete a menu Item which is the default, we can end up with the Default being blank: [error.blend](/attachments/01b6b44e-7f1d-4774-bda1-dc42ab400c47) ![image](/attachments/b27dcf11-dc5a-4d4e-8ab1-3dc0a5aba2e5) - [x] 4. On a similar note, if the Modifier had a value set which no longer exists it shows blank and will crash (same reason as (2)): [error.blend](/attachments/01b6b44e-7f1d-4774-bda1-dc42ab400c47)
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113445) when ready.
Lukas Tönne added 11 commits 2023-12-14 14:32:04 +01:00
Lukas Tönne added 1 commit 2023-12-14 14:51:13 +01:00
Lukas Tönne added 1 commit 2023-12-14 15:13:39 +01:00
639262ffd3 Sanity check for enum item strings.
Identifier and name strings must never be null (assert). Description
can be null, so appropriate checks must be used while copying.
Lukas Tönne added 2 commits 2023-12-14 15:14:56 +01:00
ec606446f4 Sanity check for enum item strings.
Identifier and name strings must never be null (assert). Description
can be null, so appropriate checks must be used while copying.
Lukas Tönne added 3 commits 2023-12-14 16:15:26 +01:00
cf8a4e31bd Map IDP_INT properties to PROP_ENUM when it has enum items.
This will automatically display a custom int property as a dropdown
button when enum items are added.
01d65b15fe Map IDP_INT properties to PROP_ENUM when it has enum items.
This will automatically display a custom int property as a dropdown
button when enum items are added.
Author
Member

Crash if I use ctrl+scrollwheel to go beyond the range of the menu

This appears to be a bug in RNA: item.identifier == nullptr indicates the end of the items list (as opposed to identifier[0]=='\0' which is a separator), but there is at least one function that does not take this into account:
4a34dcbb69/source/blender/makesrna/intern/rna_access.cc (L3646)

I haven't found another place where i can trigger this case yet, but there are multiple mechanisms interacting here. For example, when scrolling in the properties editor it tends to flip through all the panels when reaching the end of a enum list or something.

Was a bug in the item counting of the ID prop -> RNA conversion, it only returns the actual item count now.

> Crash if I use ctrl+scrollwheel to go beyond the range of the menu ~~This appears to be a bug in RNA: `item.identifier == nullptr` indicates the end of the items list (as opposed to `identifier[0]=='\0'` which is a separator), but there is _at least_ one function that does not take this into account:~~ https://projects.blender.org/blender/blender/src/commit/4a34dcbb6970c381f5cd9469e9697ab1d45fec0d/source/blender/makesrna/intern/rna_access.cc#L3646 ~~I haven't found another place where i can trigger this case yet, but there are multiple mechanisms interacting here. For example, when scrolling in the properties editor it tends to flip through all the panels when reaching the end of a enum list or something.~~ Was a bug in the item counting of the ID prop -> RNA conversion, it only returns the actual item count now.
Lukas Tönne added 3 commits 2023-12-14 17:14:25 +01:00
24bdada1ce Fix RNA enum items count returned for ID properties.
The terminator item must not be included in the items count.
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
25deae63de
Fix RNA enum items count returned for ID properties.
The terminator item must not be included in the items count.
Lukas Tönne added 1 commit 2023-12-14 17:40:20 +01:00
Author
Member

When we delete a menu Item which is the default, we can end up with the Default being blank

@dfelinto I think this is acceptable?

Selecting a different item for group input defaults and the modifier would require walking over all of Blender data to find potentially affected properties, and it still wouldn't be able to handle the linked data case. The new item would be quite arbitrary, we're just assuming that the next item is a suitable replacement. It's conceptually the same as changing the min/max range of a float value and expect all group nodes to update their inputs to accommodate that change.

The crash should be fixed now.

EDIT: I'll think about this some more, maybe it's feasible.

> When we delete a menu Item which is the default, we can end up with the Default being blank @dfelinto I think this is acceptable? Selecting a different item for group input defaults and the modifier would require walking over all of Blender data to find potentially affected properties, and it still wouldn't be able to handle the linked data case. The new item would be quite arbitrary, we're just assuming that the next item is a suitable replacement. It's conceptually the same as changing the min/max range of a float value and expect all group nodes to update their inputs to accommodate that change. The crash should be fixed now. EDIT: I'll think about this some more, maybe it's feasible.
Lukas Tönne added 1 commit 2023-12-15 10:23:36 +01:00
Lukas Tönne changed title from WIP: Implementation of Menu Switch node to Implementation of Menu Switch node 2023-12-15 10:24:05 +01:00
Iliya Katushenock added this to the Nodes & Physics project 2023-12-15 10:32:09 +01:00
Iliya Katushenock added the
Interest
Geometry Nodes
label 2023-12-15 10:32:19 +01:00
Author
Member

When we delete a menu Item which is the default, we can end up with the Default being blank

We basically want some kind of automatic versioning here to ensure users can remove an item and still have somewhat working nodes wherever the menu gets propagated. Node groups do this for sockets on file load and on tree updates to ensure that links are not removed. We could do a similar thing for menu sockets, but there are potential issues to consider:

  1. Confirming validity of an enum value and finding valid default item requires linear search. Not usually an issue because enum lists are quite small, but it might add up.
  2. We have to do this not just for node groups but for any node with a menu (input) socket.
> When we delete a menu Item which is the default, we can end up with the Default being blank We basically want some kind of automatic versioning here to ensure users can remove an item and still have somewhat working nodes wherever the menu gets propagated. Node groups do this for sockets on file load and on tree updates to ensure that links are not removed. We could do a similar thing for menu sockets, but there are potential issues to consider: 1. Confirming validity of an enum value and finding valid default item requires linear search. Not _usually_ an issue because enum lists are quite small, but it might add up. 2. We have to do this not just for node groups but for any node with a menu (input) socket.
Jacques Lucke requested changes 2023-12-15 21:14:37 +01:00
Jacques Lucke left a comment
Member
  • New socket type has to be handled in NodeDeclarationBuilder::add_input and add_output.
  • There should not be an attribute switch for the enum property in the modifier, even when the enum is a field. That's be cause we don't support enum attributes.
  • Have you considered to use the socket items abstraction like e.g. IndexSwitchItemsAccessor? It feels like this way you could avoid a lot of the boilerplate.
  • I still don't fully understand why bNodeSocketValueMenu has to keep a pointer to the bNodeTree. It feels like it could just have some std::shared_ptr<NodeEnumItems> at run-time, that contains the values directly.
* [x] New socket type has to be handled in `NodeDeclarationBuilder::add_input` and `add_output`. * [x] There should not be an attribute switch for the enum property in the modifier, even when the enum is a field. That's be cause we don't support enum attributes. * Have you considered to use the socket items abstraction like e.g. `IndexSwitchItemsAccessor`? It feels like this way you could avoid a lot of the boilerplate. * I still don't fully understand why `bNodeSocketValueMenu` has to keep a pointer to the `bNodeTree`. It feels like it could just have some `std::shared_ptr<NodeEnumItems>` at run-time, that contains the values directly.
@ -315,3 +330,4 @@
return;
}
/* XXX This isn't great yet, it's fast but could be avoided if
Member

I think the reason here is not good enough for a special case. I don't think we need to pass the node tree to every API function to solve this. The update flag can also be set at a higher level (e.g. in rna or operator code).

I think the reason here is not good enough for a special case. I don't think we need to pass the node tree to every API function to solve this. The update flag can also be set at a higher level (e.g. in rna or operator code).
Author
Member

Removed the flag and now API calls tag the node tree directly.

Removed the flag and now API calls tag the node tree directly.
LukasTonne marked this conversation as resolved
@ -372,1 +392,3 @@
}
if (md->type == eModifierType_Nodes) {
MOD_nodes_update_interface(
object, (NodesModifierData *)md, /*recalc=*/result.interface_changed);
Member

Why would we ever want to update the modifier if interface_changed is false? If an exposed enum changed, interface_changed should be true.

Why would we ever want to update the modifier if `interface_changed` is false? If an exposed enum changed, `interface_changed` should be true.
Author
Member

IIRC i added this because the modifier ID properties need to be updated when the enum definition changes, which is not covered by the interface_changed flag. So the MOD_nodes_update_interface now gets called more often, potentially increasing overhead. However, only part of what MOD_nodes_update_interface does is necessary for enum updates, so the recalc flag is a way to exclude unnecessary work.

I'll try to split MOD_nodes_update_interface into an "interface" part and a "non-interface" part, that might make it a bit simpler. I think i tried that at some point and it got a bit messy, but i don't remember why exactly.

IIRC i added this because the modifier ID properties need to be updated when the enum definition changes, which is not covered by the `interface_changed` flag. So the `MOD_nodes_update_interface` now gets called more often, potentially increasing overhead. However, only part of what `MOD_nodes_update_interface` does is necessary for enum updates, so the `recalc` flag is a way to exclude unnecessary work. I'll try to split `MOD_nodes_update_interface` into an "interface" part and a "non-interface" part, that might make it a bit simpler. I think i tried that at some point and it got a bit messy, but i don't remember why exactly.
Author
Member

I made two separate update functions out of MOD_nodes_update_interface now, let me know if that looks ok: b0e76d5317

I made two separate update functions out of `MOD_nodes_update_interface` now, let me know if that looks ok: b0e76d531742c5d348babb0d968ab62950a442c3
LukasTonne marked this conversation as resolved
@ -781,0 +861,4 @@
for (const bNodeSocket *src : src_span) {
if (src->is_available() && src->type == SOCK_MENU) {
update_socket_enum_definition(*dst.default_value_typed<bNodeSocketValueMenu>(),
Member

Please use this-> for method calls in this file.

Please use `this->` for method calls in this file.
LukasTonne marked this conversation as resolved
@ -1199,6 +1199,7 @@ static const float std_node_socket_colors[][4] = {
{0.62, 0.31, 0.64, 1.0}, /* SOCK_TEXTURE */
{0.92, 0.46, 0.51, 1.0}, /* SOCK_MATERIAL */
{0.65, 0.39, 0.78, 1.0}, /* SOCK_ROTATION */
{1.00, 1.00, 1.00, 1.0}, /* SOCK_ROTATION */
Member

Wrong comment.

Wrong comment.
LukasTonne marked this conversation as resolved
@ -0,0 +85,4 @@
case SOCK_MENU:
/* Technically possible perhaps to select an enum based on
* another enum, but not supported for now. */
BLI_assert_unreachable();
Member

I don't really see a good reason for not supporting this now. It seems like something that should just work, or is there any complication?

I don't really see a good reason for not supporting this now. It seems like something that should just work, or is there any complication?

For some socket only one enum definition can be exist in context of evaluation, right?

For some socket only one enum definition can be exist in context of evaluation, right?
Author
Member

I still need to implement better right-to-left propagation within the node for this to work. If the menu switch is set to "Menu" socket type the output is a menu. Initially this output is an empty menu. Connecting the output to an existing enum definition (i.e. another menu switch node) should propagate the enum definition to all "case" inputs but not to the condition input (first input).

Right now there is not much flexibility in how nodes propagate from outputs to inputs, it just copies the first menu output items to all the inputs, thereby creating a conflict with the source enum definition of the node itself. This method works fine for simple nodes like reroutes and boolean/index switches. We can implement this on a case-by-case basis for now.

e0a158e5db/source/blender/blenkernel/intern/node_tree_update.cc (L973-L979)

I still need to implement better right-to-left propagation within the node for this to work. If the menu switch is set to "Menu" socket type the output is a menu. Initially this output is an empty menu. Connecting the output to an existing enum definition (i.e. another menu switch node) _should_ propagate the enum definition to all "case" inputs _but not to the condition input_ (first input). Right now there is not much flexibility in how nodes propagate from outputs to inputs, it just copies the first menu output items to all the inputs, thereby creating a conflict with the source enum definition of the node itself. This method works fine for simple nodes like reroutes and boolean/index switches. We can implement this on a case-by-case basis for now. https://projects.blender.org/blender/blender/src/commit/e0a158e5db4564fbf0e03a6e9c100f6ed010783c/source/blender/blenkernel/intern/node_tree_update.cc#L973-L979
@ -891,6 +988,11 @@ void update_output_properties_from_node_tree(const bNodeTree &tree,
const bNodeSocketType *typeinfo = socket.socket_typeinfo();
const eNodeSocketDatatype socket_type = typeinfo ? eNodeSocketDatatype(typeinfo->type) :
SOCK_CUSTOM;
/* Enum sockets get updated every time, because enum item changes do not trigger a full
Member

What does "every time", "enum item changes" and "full interface update" mean here exactly?

What does "every time", "enum item changes" and "full interface update" mean here exactly?
Author
Member

Reverted by changes to MOD_nodes_update_interface.

Reverted by changes to `MOD_nodes_update_interface`.
LukasTonne marked this conversation as resolved
Author
Member

It feels like it could just have some std::shared_ptr at run-time, that contains the values directly.

This kinda got lost, i haven't really thought about it while doing the enum props. One potential problem would be that it leaves DNA data deallocation to the std::shared_ptr, rather than the conventional node storage callbacks. Might work as a std::unique_ptr with a custom deleter. Have to check with core devs.

> It feels like it could just have some std::shared_ptr<NodeEnumItems> at run-time, that contains the values directly. This kinda got lost, i haven't really thought about it while doing the enum props. One potential problem would be that it leaves DNA data deallocation to the `std::shared_ptr`, rather than the conventional node storage callbacks. Might work as a `std::unique_ptr` with a custom deleter. Have to check with core devs.
Member

I didn't mean to store dna data in the shared_ptr, but just normal run-time data. Can also be any other kind of pointer, I only wanted shared memory because the enum items are never modified but always set from scratch.

struct RuntimeNodeEnumItem {
  int id;
  std::string description;
  std::string name;
  /* ... */
};

struct RuntimeNodeEnumItems : public ImplicitSharingMixin {
  Vector<RuntimeNodeEnumItem> items;
};

struct bNodeSocketValueMenu {
  int current_value;
  const RuntimeNodeEnumItems *items;
};

In this example I'm using implicit-sharing because that's currently the easiest way to embed a user-count into a struct so that it can be shared without allocating an additional std::shared_ptr (this is similar to AnonymousAttributeID). Since the data can be considered to be immutable, we don't need all the copy-on-write stuff here.

The idea is that in propagate_enum_definitions we create a new RuntimeNodeEnumItems (if it changed) and propagate it to all sockets that use it (increasing the user-count for every socket that references it). This way, the socket does not need a reference to the node tree, which feels somewhat brittle to me. On file-load the bNodeSocketValueMenu::items pointer is set to null.

The "conventional node storage callbacks" would just have to increase/decrease the user count of the items.

I didn't mean to store dna data in the `shared_ptr`, but just normal run-time data. Can also be any other kind of pointer, I only wanted shared memory because the enum items are never modified but always set from scratch. ```cpp struct RuntimeNodeEnumItem { int id; std::string description; std::string name; /* ... */ }; struct RuntimeNodeEnumItems : public ImplicitSharingMixin { Vector<RuntimeNodeEnumItem> items; }; struct bNodeSocketValueMenu { int current_value; const RuntimeNodeEnumItems *items; }; ``` In this example I'm using implicit-sharing because that's currently the easiest way to embed a user-count into a struct so that it can be shared without allocating an additional `std::shared_ptr` (this is similar to `AnonymousAttributeID`). Since the data can be considered to be immutable, we don't need all the copy-on-write stuff here. The idea is that in `propagate_enum_definitions` we create a new `RuntimeNodeEnumItems` (if it changed) and propagate it to all sockets that use it (increasing the user-count for every socket that references it). This way, the socket does not need a reference to the node tree, which feels somewhat brittle to me. On file-load the `bNodeSocketValueMenu::items` pointer is set to null. The "conventional node storage callbacks" would just have to increase/decrease the user count of the `items`.
Lukas Tönne changed title from Implementation of Menu Switch node to Menu Switch Node 2023-12-18 09:12:33 +01:00
Lukas Tönne added 2 commits 2023-12-18 12:58:38 +01:00
Lukas Tönne added 2 commits 2023-12-18 14:12:59 +01:00
Lukas Tönne added 1 commit 2023-12-18 14:47:51 +01:00
Lukas Tönne added 1 commit 2023-12-18 14:58:18 +01:00
Lukas Tönne added 2 commits 2023-12-18 15:19:56 +01:00
Lukas Tönne added 1 commit 2023-12-18 16:44:34 +01:00
Author
Member

There should not be an attribute switch for the enum property in the modifier, even when the enum is a field. That's be cause we don't support enum attributes.

I've added a special case to input_has_attribute_toggle, hope that's enough. Otherwise the operator button seems to just be based on the assumption that every type which can be a field can also be an attribute.

Have you considered to use the socket items abstraction like e.g. IndexSwitchItemsAccessor? It feels like this way you could avoid a lot of the boilerplate.

I will leave this code as-is for now. I'm not fully convinced that it will make the code much simpler, given the somewhat different storage of enum items compared to other zone nodes. This can be refactored later on without the rest of the code complicating the picture.

I still don't fully understand why bNodeSocketValueMenu has to keep a pointer to the bNodeTree. It feels like it could just have some std::shared_ptr at run-time, that contains the values directly.

Using runtime implicit sharing data now, works great.

> There should not be an attribute switch for the enum property in the modifier, even when the enum is a field. That's be cause we don't support enum attributes. I've added a special case to `input_has_attribute_toggle`, hope that's enough. Otherwise the operator button seems to just be based on the assumption that every type which can be a field can also be an attribute. > Have you considered to use the socket items abstraction like e.g. IndexSwitchItemsAccessor? It feels like this way you could avoid a lot of the boilerplate. I will leave this code as-is for now. I'm not fully convinced that it will make the code much simpler, given the somewhat different storage of enum items compared to other zone nodes. This can be refactored later on without the rest of the code complicating the picture. > I still don't fully understand why bNodeSocketValueMenu has to keep a pointer to the bNodeTree. It feels like it could just have some std::shared_ptr<NodeEnumItems> at run-time, that contains the values directly. Using runtime implicit sharing data now, works great.
Lukas Tönne added 1 commit 2023-12-18 17:38:31 +01:00
e0a158e5db Removed the `NODE_ENUM_DEFINITION_CHANGED` flag and tag ntree directly.
This removes the need to propagate the enum definition flag to the
node tree that owns it in every tree update. Downside is that every
place where API functions are call also has to make sure to trigger a
node tree update.
Lukas Tönne added 1 commit 2023-12-18 18:44:45 +01:00
b0e76d5317 Clearer API for updating the nodes modifier after tree changes.
The `MOD_nodes_update_interface` updates ID properties as well as
updating bakes and tagging for depsgraph recalc. When menu enum items
change the properties needed to be updated for correct menus, but not
recalculate the modifier. Since enum changes are not covered by the
`interface_changed` flag, the update was running more often, with a
flag to indicate if the modifier should be recalculated.

To make the API clearer, the `MOD_nodes_update_interface` has been split
into two functions:
- MOD_nodes_update_properties: rebuilds the ID properties
- MOD_nodes_update_properties_and_recalc: rebuild ID properties and then
    tag for modifier evaluation.
Lukas Tönne added 2 commits 2023-12-18 18:50:58 +01:00
Jacques Lucke reviewed 2023-12-19 01:05:16 +01:00
@ -486,6 +490,7 @@ class NodeTreeMainUpdater {
this->make_node_previews_dirty(ntree);
this->propagate_runtime_flags(ntree);
this->propagate_enum_definitions(ntree);
Member

I think it's important that this can detect whether the enum definitions on the interface changed (by just comparing the old and new state). This is also done below in a few cases: result.interface_changed = true;.

This is important because it allows more selectively updating parent node groups and the modifier. Setting interface_changed to true will also tag the group nodes that invoke the current group as changed (see add_node_tag(pair.first, pair.second, NTREE_CHANGED_NODE_PROPERTY);).

I think it's important that this can detect whether the enum definitions on the interface changed (by just comparing the old and new state). This is also done below in a few cases: `result.interface_changed = true;`. This is important because it allows more selectively updating parent node groups and the modifier. Setting `interface_changed` to true will also tag the group nodes that invoke the current group as changed (see `add_node_tag(pair.first, pair.second, NTREE_CHANGED_NODE_PROPERTY);`).
Author
Member

I think i need to add another update flag NTREE_CHANGED_ENUM to keep track of which nodes are actually affected by enum changes. RNA would also set this more specifically, instead of the generic NTREE_CHANGED_NODE_PROPERTY. Otherwise it wouldn't be possible to tell genuine enum pointer conflicts apart from pointers that simply haven't been updated yet and get replaced. Other parts of the updater seem to do the same, e.g. BKE_ntree_update_tag_node_internal_link is called both from the API as well as the update_internal_links_in_node propagation function.

I think i need to add another update flag `NTREE_CHANGED_ENUM` to keep track of which nodes are actually affected by enum changes. RNA would also set this more specifically, instead of the generic `NTREE_CHANGED_NODE_PROPERTY`. Otherwise it wouldn't be possible to tell genuine enum pointer conflicts apart from pointers that simply haven't been updated yet and get replaced. Other parts of the updater seem to do the same, e.g. `BKE_ntree_update_tag_node_internal_link` is called both from the API as well as the `update_internal_links_in_node` propagation function.
Author
Member

I didn't need to add another update flag after all. I just make sure the enum items of the menu switch are only replaced when the node changes, and then compare the new node tree interface pointers at the end.

I didn't need to add another update flag after all. I just make sure the enum items of the menu switch are only replaced when the node changes, and then compare the new node tree interface pointers at the end.
LukasTonne marked this conversation as resolved
@ -801,6 +806,186 @@ class NodeTreeMainUpdater {
}
}
void reset_enum_ptr(bNodeSocketValueMenu &dst)
Member

Minor note, since we are in a class here, it's possible to order the functions so that the higher level functions are at the top and they call the functions below. That's done in the rest of the update code here.

Minor note, since we are in a `class` here, it's possible to order the functions so that the higher level functions are at the top and they call the functions below. That's done in the rest of the update code here.
LukasTonne marked this conversation as resolved
@ -20,3 +20,3 @@
* the values.
*/
void MOD_nodes_update_interface(Object *object, NodesModifierData *nmd);
void MOD_nodes_update_properties(NodesModifierData *nmd);
Member

What do you think about calling these MOD_nodes_update_interface_ui and the other one just MOD_nodes_update_interface? It should be clear that the former only affects the UI and can't affect the evaluated result. I think to be able to detect which of these is the case, we really need to compare the old and new enum items exposed in the group interface.

It might also just not be necessary to differentiate these two cases for now, if we just avoid calling the old MOD_nodes_update_interface if nothing has actually changed.

What do you think about calling these `MOD_nodes_update_interface_ui` and the other one just `MOD_nodes_update_interface`? It should be clear that the former only affects the UI and can't affect the evaluated result. I think to be able to detect which of these is the case, we really need to compare the old and new enum items exposed in the group interface. It might also just not be necessary to differentiate these two cases for now, if we just avoid calling the old `MOD_nodes_update_interface` if nothing has actually changed.
Author
Member

If any enum changes are included in the interface_changed flag then we may not need to distinguish these two cases after all. It would mean the interface gets fully updated and the modifier is evaluated after any change, including things like renaming an enum item that wouldn't require a full re-evaluation. If that's ok i'm happy to revert this part to keep it simple and just stick with one MOD_nodes_update_interface function.

If any enum changes are included in the `interface_changed` flag then we may not need to distinguish these two cases after all. It would mean the interface gets fully updated and the modifier is evaluated after any change, including things like renaming an enum item that wouldn't require a full re-evaluation. If that's ok i'm happy to revert this part to keep it simple and just stick with one `MOD_nodes_update_interface` function.
LukasTonne marked this conversation as resolved
Lukas Tönne added 2 commits 2023-12-19 13:35:39 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
58cd018064
Better enum propagation with change detection for limited updates.
Now the propagation method keeps track of which sockets actually have
changes to their enum definitions. This allows detecting when the node
group interface changes and when node groups or modifiers need to
subsequently update as well.
Lukas Tönne added 2 commits 2023-12-19 17:50:04 +01:00
bd34307b25 Revert modifier update functions for ID properties.
Modifier property updates should now only be triggered when actual
changes happen, making the distinction unnecessary.
Hans Goudey requested review from Hans Goudey 2023-12-19 20:05:58 +01:00
Hans Goudey requested changes 2023-12-19 21:11:58 +01:00
Hans Goudey left a comment
Member
  • Field inference isn't quite right here:
    image

My main question is about the socket color. White is already taken by the collection socket. I'd propose a darkish gray to mimic the look of the drop-down button in the default theme. Not sure everyone loves that idea, but it's the only one I've heard, haha

- [ ] Field inference isn't quite right here: ![image](/attachments/5416f628-384f-4f0b-95ff-a930c4540760) My main question is about the socket color. White is already taken by the collection socket. I'd propose a darkish gray to mimic the look of the drop-down button in the default theme. Not sure everyone loves that idea, but it's the only one I've heard, haha
@ -0,0 +16,4 @@
/**
* Runtime copy of #NodeEnumItem for use in #RuntimeNodeEnumItems.
*/
struct RuntimeNodeEnumItem {
Member

This file should go in the blender::bke namespace

This file should go in the `blender::bke` namespace
Author
Member

I tried that, problem is we can't reference types in a namespace from DNA ...

I tried that, problem is we can't reference types in a namespace from DNA ...
Member

You can, it's done a fair amount actually. See MeshRuntimeHandle for an example.

You can, it's done a fair amount actually. See `MeshRuntimeHandle` for an example.
Author
Member

Ah forgot about that trick.

Ah forgot about that trick.
LukasTonne marked this conversation as resolved
@ -73,6 +73,8 @@ PrimitiveBakeItem::~PrimitiveBakeItem()
StringBakeItem::StringBakeItem(std::string value) : value_(std::move(value)) {}
EnumBakeItem::EnumBakeItem(const int32_t value) : value_(std::move(value)) {}
Member

Might as well define the constructor in the header, it's so trivial. No need for a std::move on an integer also

Might as well define the constructor in the header, it's so trivial. No need for a `std::move` on an integer also
Author
Member

Removed enum baking, not relevant any more.

Removed enum baking, not relevant any more.
LukasTonne marked this conversation as resolved
@ -922,0 +935,4 @@
} bNodeSocketValueMenu;
/* Flags for #bNodeSocketValueMenu. */
typedef enum NodeSocketValueMenuFlag {
Member

This seems like runtime data, since it's found by doing static analysis on the tree. Could it be stored outside of DNA?

This seems like runtime data, since it's found by doing static analysis on the tree. Could it be stored outside of DNA?
Author
Member

Yes this is a runtime flag. Moving the flag into RuntimeNodeEnumItems doesn't work though, because when there is a conflict the enum_items pointer is set to null. The items themselves are fine, the updater just can't decide which of them to assign to the socket. So it's the socket itself that should be marked as "conflicted".

There isn't a good place to put this flag in the generic bNodeSocketRuntime, so rather than adding yet another complicated runtime pointer thingy in bNodeSocketValueMenu just for a flag, i'm going to rename it to runtime_flag and move the flag enum definition into BKE_node_enum.hh.

Yes this is a runtime flag. Moving the flag into `RuntimeNodeEnumItems` doesn't work though, because when there is a conflict the `enum_items` pointer is set to null. The items themselves are fine, the updater just can't decide which of them to assign to the socket. So it's the socket itself that should be marked as "conflicted". There isn't a good place to put this flag in the generic `bNodeSocketRuntime`, so rather than adding yet another complicated runtime pointer thingy in `bNodeSocketValueMenu` just for a flag, i'm going to rename it to `runtime_flag` and move the flag enum definition into `BKE_node_enum.hh`.
LukasTonne marked this conversation as resolved
@ -124,0 +125,4 @@
const EnumPropertyItem *RNA_node_enum_definition_itemf(const RuntimeNodeEnumItems &enum_items,
bool *r_free);
const EnumPropertyItem *RNA_node_socket_enum_itemf(bContext *C,
Member

Unused definition

Unused definition
LukasTonne marked this conversation as resolved
@ -0,0 +43,4 @@
break;
case SOCK_RGBA:
b.add_input<decl::Color>(name, identifier)
.default_value({0.8f, 0.8f, 0.8f, 1.0f})
Member

No need to specify the default, it's already the default for the default

No need to specify the default, it's already the default for the default
LukasTonne marked this conversation as resolved
@ -0,0 +46,4 @@
.default_value({0.8f, 0.8f, 0.8f, 1.0f})
.supports_field();
break;
case SOCK_SHADER:
Member

Seems possible to write this code generically for all types without the switch statement, like how it's done in the index switch node. Same with the outputs.

Seems possible to write this code generically for all types without the switch statement, like how it's done in the index switch node. Same with the outputs.
LukasTonne marked this conversation as resolved
@ -0,0 +53,4 @@
b.add_input<decl::Bool>(name, identifier).default_value(false).supports_field();
break;
case SOCK_INT:
b.add_input<decl::Int>(name, identifier).min(-100000).max(100000).supports_field();
Member

Any particular reason to define this range? Seems unnecessary

Any particular reason to define this range? Seems unnecessary
LukasTonne marked this conversation as resolved
@ -0,0 +160,4 @@
/* Allow the node group interface to define the socket order. */
b.use_custom_socket_order();
const bool fields_type = ELEM(data_type,
Member

There's a socket_type_supports_fields function for this already

There's a `socket_type_supports_fields` function for this already
LukasTonne marked this conversation as resolved
@ -0,0 +260,4 @@
else {
/* No sensible way to connect inputs currently:
* Switch socket connection will always create a conflicting enum ref.
* Case input sockets don't existing without an actual enum definition.
Member

Broken English here, and I don't really get the limitation currently

Broken English here, and I don't really get the limitation currently
LukasTonne marked this conversation as resolved
@ -0,0 +265,4 @@
}
}
/* Multifunction which evaluates the switch input for each enum item and partially fills the output
Member

use doxygen syntax

use doxygen syntax
LukasTonne marked this conversation as resolved
@ -0,0 +267,4 @@
/* Multifunction which evaluates the switch input for each enum item and partially fills the output
* array with values from the input array where the identifier matches. */
template<typename T> class MenuSwitchFn : public mf::MultiFunction {
Member

This class shouldn't need to be templated, it's fairly simple to handle different types generically inside.
Also, I think the implementation could look exactly the same as the index switch node, except with a different callback in the IndexMask::from_groups lambda. Currently it's looping over all items for every enum item, which is a bit bad in the worst case

This class shouldn't need to be templated, it's fairly simple to handle different types generically inside. Also, I think the implementation could look exactly the same as the index switch node, except with a different callback in the `IndexMask::from_groups` lambda. Currently it's looping over all items for every enum item, which is a bit bad in the worst case
Author
Member

Did IndexMask construction change? I implemented this months ago, and IIRC back then constructing a full index mask for each enum case was basically doing the same thing, looping over the full range to construct a mask, only to apply it once and discard.

The difference to the Index Switch case is that the index node only has to construct one mask and can used the index itself to find the right input array to copy from for each output index. The Menu Switch has to do a search for a matching enum identifier for each menu element if we want to only do one loop (some hash table might make that faster but probably comes with some constant cost as well). My current solution avoids this by handling each enum case separately and writing only those output elements that match the current case. That way it only has to check for equality instead of doing a linear search for each element.

Did IndexMask construction change? I implemented this months ago, and IIRC back then constructing a full index mask for each enum case was basically doing the same thing, looping over the full range to construct a mask, only to apply it once and discard. The difference to the `Index Switch` case is that the index node only has to construct one mask and can used the index itself to find the right input array to copy from for each output index. The Menu Switch has to do a search for a matching enum _identifier_ for each menu element if we want to only do one loop (some hash table might make that faster but probably comes with some constant cost as well). My current solution avoids this by handling each enum case separately and writing only those output elements that match the current case. That way it only has to check for equality instead of doing a linear search for each element.
Author
Member

Ok, IndexMask::from_groups looks like it could work here. I originally swapped the inner and outer loops to be able to devirtualize the input arrays one at a time. The implementation in Index Switch seems to just access the input GVArrays by index, which i thought devirtualization was supposed to avoid? There isn't a devirtualization for GVArrays. I'm ok with that, if anyone feels like it they can optimize this later.

Ok, `IndexMask::from_groups` looks like it could work here. I originally swapped the inner and outer loops to be able to devirtualize the input arrays one at a time. The implementation in `Index Switch` seems to just access the input GVArrays by index, which i thought devirtualization was supposed to avoid? There isn't a devirtualization for GVArrays. I'm ok with that, if anyone feels like it they can optimize this later.
Member

Think we can relatively easily optimize IndexMask::from_groups later.

The implementation in Index Switch seems to just access the input GVArrays by index, which i thought devirtualization was supposed to avoid?

The index switch node calls materialize_to_uninitialized for each GVArray-- it doesn't access each element by index itself. Can't get much better than that in terms of devirtualization.

Think we can relatively easily optimize `IndexMask::from_groups` later. >The implementation in Index Switch seems to just access the input GVArrays by index, which i thought devirtualization was supposed to avoid? The index switch node calls `materialize_to_uninitialized` for each `GVArray`-- it doesn't access each element by index itself. Can't get much better than that in terms of devirtualization.
LukasTonne marked this conversation as resolved
@ -0,0 +317,4 @@
bool can_be_field_ = false;
const NodeEnumDefinition &enum_def_;
const CPPType *cpp_type_;
std::unique_ptr<MultiFunction> multi_function_;
Member

Any particular reason to store the multifunction here rather creating locally and passing ownership in FieldOperation::Create?

Any particular reason to store the multifunction here rather creating locally and passing ownership in `FieldOperation::Create`?
Author
Member

Not really. I wasn't sure about the lifetime of the lazy function and whether execute_impl would be called more than once. Constructing during the execute call is fine.

Not really. I wasn't sure about the lifetime of the lazy function and whether `execute_impl` would be called more than once. Constructing during the execute call is fine.
LukasTonne marked this conversation as resolved
@ -0,0 +380,4 @@
/* No guarantee that the switch input matches any enum,
* set default outputs to ensure valid state. */
if (!found_match) {
cpp_type_->value_initialize(output_ptr);
Member

Probably more semantically helpful to use set_default_remaining_node_outputs even if the result is the same

Probably more semantically helpful to use `set_default_remaining_node_outputs` even if the result is the same
Author
Member

There is a bug in both the Index Switch and Menu Switch nodes here: If you remove all items the set_default_remaining_node_outputs call will crash, because the lf_index_by_bsocket for the output is never defined (-1). This stuff is hard to follow but i think lazy_function_interface_from_node must be called at some point, according to the generic lazy function types.

There is a bug in both the Index Switch and Menu Switch nodes here: If you remove all items the `set_default_remaining_node_outputs` call will crash, because the `lf_index_by_bsocket` for the output is never defined (-1). This stuff is hard to follow but i think `lazy_function_interface_from_node` must be called at some point, according to the generic lazy function types.
Author
Member

Passing the graph info to the constructor now, same as get_bake_lazy_function, and then insert the socket mapping in the constructor. Would be nice to have some kind of correctness check in debug to avoid having to figure this out every time. Maybe not all nodes need the indices and should check only when accessing the index.

Passing the graph info to the constructor now, same as `get_bake_lazy_function`, and then insert the socket mapping in the constructor. Would be nice to have some kind of correctness check in debug to avoid having to figure this out every time. Maybe not all nodes need the indices and should check only when accessing the index.
Author
Member

Reported separately for the Index Switch node #116885

Reported separately for the Index Switch node #116885
LukasTonne marked this conversation as resolved
@ -0,0 +387,4 @@
void execute_field(Field<int> condition, lf::Params &params) const
{
if (!condition.node().depends_on_input()) {
Member

Pretty sure this check is redundant with the earlier is_context_dependent_field check.

Pretty sure this check is redundant with the earlier `is_context_dependent_field` check.
LukasTonne marked this conversation as resolved
@ -0,0 +500,4 @@
SOCK_GEOMETRY,
SOCK_OBJECT,
SOCK_COLLECTION,
SOCK_TEXTURE,
Member

SOCK_TEXTURE can be removed, it's not supported anywhere

`SOCK_TEXTURE` can be removed, it's not supported anywhere
LukasTonne marked this conversation as resolved
@ -484,0 +513,4 @@
bool Menu::can_connect(const bNodeSocket &socket) const
{
return sockets_can_connect(*this, socket) && socket.type == SOCK_MENU;
Member

Any chance this could get a smarter check so that it only connects to the same enum type? Maybe that's out of scope for now, but it would be really nice.

Any chance this could get a smarter check so that it only connects to the same enum type? Maybe that's out of scope for now, but it would be really nice.
Author
Member

It might work under certain circumstances, but it has limitations and changes workflow somewhat:

  1. Inputs only allow one connection, so checking for matching connections is pointless. The existing menu is replaced anyway.
  2. Outputs allow multiple connections, but users may want to replace the existing menu. Prohibiting connections would force users to first remove the existing connection, which could become annoying.
  3. Node group outputs don't know about the concrete type, this is currently an issue with type resolution more generally. A node group will not propagate the menu from outputs to inputs. We'd need some kind of internal placeholder for enum definitions to make this work. Each output then would still be treated as a separate enum and we still can't infer conflicts, since the concrete enum definitions may be compatible after all.

I suggest we keep this as a possible later feature.

It might work under certain circumstances, but it has limitations and changes workflow somewhat: 1. Inputs only allow one connection, so checking for matching connections is pointless. The existing menu is replaced anyway. 2. Outputs allow multiple connections, but users may want to replace the existing menu. Prohibiting connections would force users to first remove the existing connection, which could become annoying. 3. Node group outputs don't know about the concrete type, this is currently an issue with type resolution more generally. A node group will not propagate the menu from outputs to inputs. We'd need some kind of internal placeholder for enum definitions to make this work. Each output then would still be treated as a separate enum and we still can't infer conflicts, since the concrete enum definitions may be compatible after all. I suggest we keep this as a possible later feature.
Lukas Tönne added 3 commits 2023-12-20 09:14:02 +01:00
Lukas Tönne added 1 commit 2023-12-21 10:31:15 +01:00
Author
Member

I removed the menu socket from bake items. It already gets rejected by Bake nodes and simulation zones due to the SimulationItemsAccessor::supports_socket_type list, so this should be safe. We can consider adding bake support separately later on.

I removed the menu socket from bake items. It already gets rejected by Bake nodes and simulation zones due to the `SimulationItemsAccessor::supports_socket_type` list, so this should be safe. We can consider adding bake support separately later on.
Lukas Tönne added 3 commits 2023-12-21 11:23:34 +01:00
Lukas Tönne added 2 commits 2023-12-21 13:29:15 +01:00
Lukas Tönne added 3 commits 2023-12-21 13:55:04 +01:00
Lukas Tönne added 1 commit 2023-12-21 16:58:12 +01:00
Lukas Tönne added 2 commits 2023-12-22 11:04:22 +01:00
Lukas Tönne added 1 commit 2023-12-22 11:09:16 +01:00
Lukas Tönne added 1 commit 2023-12-24 15:17:47 +01:00
Iliya Katushenock reviewed 2023-12-24 16:21:52 +01:00
Iliya Katushenock left a comment
Member

It's interesting the fact that right now multifunctions can support data block data type, maybe i missed something.

~~It's interesting the fact that right now multifunctions can support data block data type, maybe i missed something.~~
@ -804,0 +892,4 @@
if (input->is_available() && input->type == SOCK_MENU) {
for (const bNodeSocket *output : node->output_sockets()) {
this->update_socket_enum_definition(
*input->default_value_typed<bNodeSocketValueMenu>(),

Shouldn't type check for output socket be here too?

Shouldn't type check for output socket be here too?
Author
Member

Yes, good catch.

Yes, good catch.
LukasTonne marked this conversation as resolved
@ -0,0 +256,4 @@
const NodeMenuSwitch &storage = node_storage(node);
const eNodeSocketDatatype data_type = eNodeSocketDatatype(storage.data_type);
can_be_field_ = ELEM(
data_type, SOCK_FLOAT, SOCK_INT, SOCK_BOOLEAN, SOCK_VECTOR, SOCK_RGBA, SOCK_ROTATION);

socket_type_supports_fields(data_type)?

`socket_type_supports_fields(data_type)`?
LukasTonne marked this conversation as resolved
Lukas Tönne added 1 commit 2024-01-03 11:48:13 +01:00
Lukas Tönne added 1 commit 2024-01-04 10:48:32 +01:00
Lukas Tönne reviewed 2024-01-04 10:55:14 +01:00
@ -0,0 +313,4 @@
}
/* No guarantee that the switch input matches any enum,
* set default outputs to ensure valid state. */
set_default_remaining_node_outputs(params, node_);
Author
Member

@JacquesLucke There is a failing assert in SocketValueVariant::convert_to_single when using set_default_remaining_node_outputs on field outputs. The SocketValueVariant default value is created with a None kind, which is not supported. This fails during GeoTreeLogger::log_value.

@JacquesLucke There is a failing assert in `SocketValueVariant::convert_to_single` when using `set_default_remaining_node_outputs` on field outputs. The `SocketValueVariant` default value is created with a `None` kind, which is not supported. This fails during `GeoTreeLogger::log_value`.
LukasTonne marked this conversation as resolved
Lukas Tönne added 1 commit 2024-01-04 11:12:24 +01:00
Lukas Tönne added 2 commits 2024-01-04 11:18:35 +01:00
Author
Member

Field inference isn't quite right here:
image

Seems to work fine now. Did this only happen for rotation socket type? IIRC the field support wasn't consistent in the initial version.

My main question is about the socket color. White is already taken by the collection socket. I'd propose a darkish gray to mimic the look of the drop-down button in the default theme. Not sure everyone loves that idea, but it's the only one I've heard, haha

Main issue with dark colors is that links become hard to see. I'll leave it for discussion next week.
image

> Field inference isn't quite right here: > ![image](/attachments/5416f628-384f-4f0b-95ff-a930c4540760) Seems to work fine now. Did this only happen for rotation socket type? IIRC the field support wasn't consistent in the initial version. > My main question is about the socket color. White is already taken by the collection socket. I'd propose a darkish gray to mimic the look of the drop-down button in the default theme. Not sure everyone loves that idea, but it's the only one I've heard, haha Main issue with dark colors is that links become hard to see. I'll leave it for discussion next week. ![image](/attachments/9446a050-b932-43f6-80e2-a226e8324340)
Lukas Tönne added 1 commit 2024-01-08 11:26:29 +01:00
Lukas Tönne added 1 commit 2024-01-08 12:09:37 +01:00
Lukas Tönne requested review from Hans Goudey 2024-01-08 12:34:18 +01:00
Lukas Tönne requested review from Jacques Lucke 2024-01-08 12:34:19 +01:00
Hans Goudey changed title from Menu Switch Node to Geometry Nodes: Menu Switch Node 2024-01-15 16:43:44 +01:00
Lukas Tönne added 1 commit 2024-01-24 14:57:01 +01:00
Author
Member

Some color tests.
None of these are very convincing, but imo there just aren't any good colors left, we just have too many sockets. So unless and until we redesign the socket colors i would like us to just pick a color and be done with it.
My socket-color-experiment branch has theme colors to play around with socket colors more easily, if anyone feels the need to do that.

  • Dark-ish gray sockets with a brighter outline:
    Quite noisy and hard to read.
    image
  • Dusty Mint
    image
  • Dark blue
    Screenshot_20240124_152554
  • Pale yellow
    Screenshot_20240124_152646
Some color tests. None of these are very convincing, but imo there just aren't any good colors left, we just have too many sockets. So unless and until we redesign the socket colors i would like us to just pick a color and be done with it. My [socket-color-experiment branch](https://projects.blender.org/LukasTonne/blender/src/branch/socket-color-experiment) has theme colors to play around with socket colors more easily, if anyone feels the need to do that. - Dark-ish gray sockets with a brighter outline: Quite noisy and hard to read. ![image](/attachments/6fcbc8ea-9fa5-42b5-a022-e27318b356c8) - Dusty Mint ![image](/attachments/3e4cf178-ca9e-4fbf-9802-01bc83ccce25) - Dark blue ![Screenshot_20240124_152554](/attachments/9b3e6f25-deb7-4fe7-8e97-9ecbc622a510) - Pale yellow ![Screenshot_20240124_152646](/attachments/d7e35baf-2232-4125-aaaf-7b4bee763471)
Lukas Tönne added 1 commit 2024-01-24 15:40:42 +01:00
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
13065b1f17
Use 0.40 gray for the menu socket.
Author
Member

UI meeting seems to have settled on 0.40 for the socket color:
Screenshot_20240124_153957

UI meeting seems to have settled on 0.40 for the socket color: ![Screenshot_20240124_153957](/attachments/7e136592-8f16-4f69-a06b-4b885c7617c3)
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2024-01-24 17:51:19 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
e875af63e6
Format fix.
Jacques Lucke approved these changes 2024-01-25 15:19:57 +01:00
Jacques Lucke left a comment
Member

Got a bunch of smaller comments, but overall looks nice and works in my test. If the mentioned things are fixed, I don't need to have another look.

  • Socket inspection shouldn't show the internal identifier.
  • Index Switch node does not support menu sockets currently.
  • Update screenshots in PR description.
Got a bunch of smaller comments, but overall looks nice and works in my test. If the mentioned things are fixed, I don't need to have another look. * [x] Socket inspection shouldn't show the internal identifier. * [x] Index Switch node does not support menu sockets currently. * [x] Update screenshots in PR description.
@ -1826,6 +1865,8 @@ static void socket_id_user_increment(bNodeSocket *sock)
case SOCK_ROTATION:
case SOCK_INT:
case SOCK_STRING:
/* Note: Enum socket node tree is a weak reference and not user-counted. */
Member

Outdated comment, same below.

Outdated comment, same below.
LukasTonne marked this conversation as resolved
@ -0,0 +54,4 @@
if (!this->items().contains_ptr(&item)) {
return false;
}
const int remove_index = &item - this->items().begin();
Member

Maybe you want to use the utility in DNA_array_utils.hh. Same below.

Maybe you want to use the utility in` DNA_array_utils.hh`. Same below.
LukasTonne marked this conversation as resolved
@ -0,0 +75,4 @@
return;
}
this->items_num = 0;
MEM_SAFE_FREE(this->items_array);
Member

Looks like this does not free everything in the items. Can just use the utility in DNA_array_utils.hh.

Looks like this does not free everything in the items. Can just use the utility in `DNA_array_utils.hh`.
LukasTonne marked this conversation as resolved
@ -78,6 +79,7 @@ template<> void socket_data_id_user_increment(bNodeSocketValueMaterial &data)
{
id_us_plus(reinterpret_cast<ID *>(data.value));
}
/* Note: bNodeSocketValueMenu does not do user counting on the node tree reference. */
Member

This comment and the one below don't seem to be necessary anymore.

This comment and the one below don't seem to be necessary anymore.
LukasTonne marked this conversation as resolved
@ -484,6 +485,9 @@ class NodeTreeMainUpdater {
this->make_node_previews_dirty(ntree);
this->propagate_runtime_flags(ntree);
if (this->propagate_enum_definitions(ntree)) {
Member

Since this is geometry nodes only for now, it can also be moved one line down.

Since this is geometry nodes only for now, it can also be moved one line down.
LukasTonne marked this conversation as resolved
@ -507,3 +507,3 @@
const eNodeSocketDatatype socket_type = eNodeSocketDatatype(typeinfo->type);
/* The property should be created in #MOD_nodes_update_interface with the correct type. */
/* The property should be created in #MOD_nodes_update_properties with the correct type. */
Member

Outdated change.

Outdated change.
LukasTonne marked this conversation as resolved
@ -995,2 +998,4 @@
break;
}
case SOCK_MENU:
uiItemL(sub, "MENU SOCKET TODO", ICON_NONE);
Member

Change to something like RPT_("Unsupported Menu Socket")

Change to something like `RPT_("Unsupported Menu Socket")`
LukasTonne marked this conversation as resolved
@ -7941,7 +7941,6 @@ static void rna_def_modifier_grease_pencil_tint(BlenderRNA *brna)
RNA_def_property_update(prop, 0, "rna_Modifier_update");
}
Member

Unrelated change.

Unrelated change.
Author
Member

Doesn't show up in the changes any more, guess is outdated.

Doesn't show up in the changes any more, guess is outdated.
LukasTonne marked this conversation as resolved
@ -68,7 +68,6 @@ void update_input_properties_from_node_tree(const bNodeTree &tree,
const IDProperty *old_properties,
bool use_bool_for_use_attribute,
IDProperty &properties);
Member

unrelated change

unrelated change
LukasTonne marked this conversation as resolved
@ -373,6 +373,7 @@ DefNode(GeometryNode, GEO_NODE_IS_VIEWPORT, 0, "IS_VIEWPORT", IsViewport, "Is Vi
DefNode(GeometryNode, GEO_NODE_JOIN_GEOMETRY, 0, "JOIN_GEOMETRY", JoinGeometry, "Join Geometry", "Merge separately generated geometries into a single one")
DefNode(GeometryNode, GEO_NODE_MATERIAL_SELECTION, 0, "MATERIAL_SELECTION", MaterialSelection, "Material Selection", "Provide a selection of faces that use the specified material")
DefNode(GeometryNode, GEO_NODE_MERGE_BY_DISTANCE, 0, "MERGE_BY_DISTANCE", MergeByDistance, "Merge by Distance", "Merge vertices or points within a given distance")
DefNode(GeometryNode, GEO_NODE_MENU_SWITCH, def_geo_menu_switch, "MENU_SWITCH", MenuSwitch, "Menu Switch", "Select from multiple inputs by name")
Member

Should be moved one line up.

Should be moved one line up.
LukasTonne marked this conversation as resolved
@ -0,0 +44,4 @@
SOCK_COLLECTION,
SOCK_MATERIAL,
SOCK_IMAGE,
SOCK_MENU);
Member

In my simple test, the menu switch didn't work with menu sockets yet (shows red link). If that's too difficult to fix right now, just remove it from the list here.

In my simple test, the menu switch didn't work with menu sockets yet (shows red link). If that's too difficult to fix right now, just remove it from the list here.
Author
Member

Yeah, can be made to work, but i really don't want to spend more time on this PR.

Yeah, can be made to work, but i really don't want to spend more time on this PR.
LukasTonne marked this conversation as resolved
@ -0,0 +57,4 @@
const eNodeSocketDatatype data_type = eNodeSocketDatatype(storage.data_type);
const bool supports_fields = socket_type_supports_fields(data_type);
auto &menu = b.add_input<decl::Menu>("Menu").default_value(false);
Member

Bit confusing to have a default_value of false. Probably shouldn't have a default value at all.

Bit confusing to have a `default_value` of `false`. Probably shouldn't have a default value at all.
LukasTonne marked this conversation as resolved
@ -0,0 +63,4 @@
}
for (const NodeEnumItem &enum_item : storage.enum_definition.items()) {
const std::string identifier = "Item" + std::to_string(enum_item.identifier);
Member

Use "Item_", just to be a little bit more consistent with other such lists (e.g. in NOD_zone_socket_items.hh).

Use `"Item_"`, just to be a little bit more consistent with other such lists (e.g. in `NOD_zone_socket_items.hh`).
LukasTonne marked this conversation as resolved
Lukas Tönne added 2 commits 2024-01-25 15:55:28 +01:00
Lukas Tönne added 1 commit 2024-01-25 16:01:58 +01:00
Lukas Tönne added 1 commit 2024-01-25 16:12:18 +01:00
Hans Goudey approved these changes 2024-01-25 21:32:44 +01:00
Hans Goudey left a comment
Member

Other than the comments above from Jacques and a couple inline comments, this looks good to me.

One small thing: create_inspection_string_for_generic_value shows the socket inspection value for menu sockets with (String). Nice that is shows the name instead of the integer value, but it could be nice to show (Menu) instead.

Other than the comments above from Jacques and a couple inline comments, this looks good to me. One small thing: `create_inspection_string_for_generic_value` shows the socket inspection value for menu sockets with `(String)`. Nice that is shows the name instead of the integer value, but it could be nice to show `(Menu)` instead.
@ -0,0 +35,4 @@
* Shared immutable list of enum items.
* These are owned by a node and can be referenced by node sockets.
*/
struct RuntimeNodeEnumItems : blender::ImplicitSharingMixin {
Member

No need for blender:: here

No need for `blender::` here
LukasTonne marked this conversation as resolved
@ -3919,0 +3965,4 @@
{
bNodeTree *ntree = reinterpret_cast<bNodeTree *>(ptr->owner_id);
const NodeEnumItem *item = static_cast<NodeEnumItem *>(ptr->data);
bNode *node = find_node_by_enum_item(ntree, item);
Member

Unused variable (in release builds anyway)

Unused variable (in release builds anyway)
LukasTonne marked this conversation as resolved
Lukas Tönne added 13 commits 2024-01-26 11:51:34 +01:00
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne merged commit 5ad49f4142 into main 2024-01-26 12:40:14 +01:00
Lukas Tönne deleted branch nodes-menu-switch 2024-01-26 12:40:16 +01:00
First-time contributor

what go you guys think about adding a switch node that allows you to switch between menus but in reverse (than what the switch node (set the menu, and not switch menu node) could do now)?

right now the switch node could take from two menus on the left and pick one to continue right. but what if we could have multiple menus from the right and pass one of them to the left (the selection will have to be a constant). this way me could change the input menus of the nodegroup dynamically.

what go you guys think about adding a switch node that allows you to switch between menus but in reverse (than what the switch node (set the menu, and not switch menu node) could do now)? right now the switch node could take from two menus on the **left** and pick one **to** continue **right**. but what if we could have multiple menus from the **right** and pass one of them **to** the **left** (the selection will have to be a constant). this way me could change the input menus of the nodegroup dynamically.
Author
Member

@shmuel If the selection is a constant it won't be a dynamic change (based on runtime values), but rather just a change of socket connections. Sort of like a static socket switch. Menu propagation should then "just work", it's not really about menu sockets specifically. That would be a more general feature, and imo not really worth spending time on. It would be much like swapping socket connections, i sometimes use a reroute node for cases where i want to try out different node setups:
Screenshot_20240201_110043

@shmuel If the selection is a constant it won't be a dynamic change (based on runtime values), but rather just a change of socket connections. Sort of like a static socket switch. Menu propagation should then "just work", it's not really about menu sockets specifically. That would be a more general feature, and imo not really worth spending time on. It would be much like swapping socket connections, i sometimes use a reroute node for cases where i want to try out different node setups: ![Screenshot_20240201_110043](/attachments/29931472-106d-42cc-9090-1a27158ce67a)
First-time contributor

I was going to use the Menu Switch today but realized that this doesn't work.
blender_240210-162126.png

But after some thinking and guessing I found out that this works, which I guess is fine for this example.
blender_240210-162848.png

But if you have a bigger tree and want to use the same menu option for multiple things you'll get long noodles over the place which can get way to messy for anyone with OCD if you want to use the same menu for several things.
blender_240210-163505.png

What I want to do is this instead but it doesn't work, the menu in the modifier goes blank.
blender_240210-163523.png

I guess I can store the integer and use a Named Attribute to bring it back to get rid of the long noodle, but it all feels like a quirky work around for something that most people expect to work like in the first example. I understand that the enum is unique for each Menu Switch node, but from a users point of view the is quite confusing since they both have inputs called A and B so it "should" work, and the fix ain't directly straight forward. Though I guess you already know this, but just wanted to share it anyways :)

I was going to use the Menu Switch today but realized that this doesn't work. ![blender_240210-162126.png](/attachments/73a1e97e-9d0d-42b1-84ac-a4bd099e2c48) But after some thinking and guessing I found out that this works, which I guess is fine for this example. ![blender_240210-162848.png](/attachments/3caff647-df44-4358-878e-6c5a0f9039a1) But if you have a bigger tree and want to use the same menu option for multiple things you'll get long noodles over the place which can get way to messy for anyone with OCD if you want to use the same menu for several things. ![blender_240210-163505.png](/attachments/68a54d40-36fc-48a0-80af-dccf55566dae) What I want to do is this instead but it doesn't work, the menu in the modifier goes blank. ![blender_240210-163523.png](/attachments/2b715641-520b-4893-a906-b5c21bc77453) I guess I can store the integer and use a Named Attribute to bring it back to get rid of the long noodle, but it all feels like a quirky work around for something that most people expect to work like in the first example. I understand that the enum is unique for each Menu Switch node, but from a users point of view the is quite confusing since they both have inputs called A and B so it "should" work, and the fix ain't directly straight forward. Though I guess you already know this, but just wanted to share it anyways :)

@Gurra this is not the place for users to asking the help. First thing that you facing with its current limitation: only one Menu input can be connected to Menu output. See: #117643

@Gurra this is not the place for users to asking the help. First thing that you facing with its current limitation: only one Menu input can be connected to Menu output. See: #117643
Member

Note, it's currently not planned to be able to intersect menu definitions from different nodes. I explicitly don't want to merge menus based on their names because that breaks very quickly when renaming things (and we want to be able to rename things without breaking node setups).

The way to deal with this is to deduplicate the menu definition. So instead of having two Menu Switch nodes, you should just put one into a node group and reuse that group. There is one limitation still, which is that groups currently don't allow for dynamic socket types. However, it is planned to change that eventually.

In your current situation, you can but the Menu Switch node into a group which outputs an index and then use the Index Switch node in the group you have currently. That should look pretty much exactly like the node tree you want to achieve.

Once we support more dynamic types on group nodes, this workaround with the Index Switch node will not be necessary anymore.

Note, it's currently not planned to be able to intersect menu definitions from different nodes. I explicitly don't want to merge menus based on their names because that breaks very quickly when renaming things (and we want to be able to rename things without breaking node setups). The way to deal with this is to deduplicate the menu definition. So instead of having two Menu Switch nodes, you should just put one into a node group and reuse that group. There is one limitation still, which is that groups currently don't allow for dynamic socket types. However, it is planned to change that eventually. In your current situation, you can but the Menu Switch node into a group which outputs an index and then use the `Index Switch` node in the group you have currently. That should look pretty much exactly like the node tree you want to achieve. Once we support more dynamic types on group nodes, this workaround with the `Index Switch` node will not be necessary anymore.
First-time contributor

@mod_moder It wasn't a question, it was feedback from a users point of view.

@JacquesLucke Thanks for your answer and for the tip :)

@mod_moder It wasn't a question, it was feedback from a users point of view. @JacquesLucke Thanks for your answer and for the tip :)
First-time contributor

Hey. Could you guys take a look at my proposal on Right-Click Select please?

Hey. Could you guys take a look at [my proposal](https://blender.community/c/rightclickselect/Bn4W/) on Right-Click Select please?
Member

@PedroRyb We decided against something like Menu ID data blocks. I explained how we want to handle this case in my previous comment in this thread.

@PedroRyb We decided against something like Menu ID data blocks. I explained how we want to handle this case in my previous comment in this thread.
First-time contributor

Access to Menu Index can still be retrieved through grouping
image

Access to Menu Index can still be retrieved through grouping ![image](/attachments/f5d61e3f-6365-4659-bdd2-601ddeaaa878)
First-time contributor

I think what menu socket looks like now is that it's design against the 'node group' design. Users use group socket input because they need to connect to more than one node inside a group and reuse it everywhere. If only one node is supported, that is no meaning to set a node socket inside a group.

I think what menu socket looks like now is that it's design against the 'node group' design. Users use group socket input because they need to connect to more than one node inside a group and reuse it everywhere. If only one node is supported, that is no meaning to set a node socket inside a group.
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
11 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: blender/blender#113445
No description provided.