Geometry Nodes: Menu Switch Node #113445
No reviewers
Labels
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 project
No Assignees
11 Participants
Notifications
Due Date
No due date set.
Depends on
#114362 ID properties: Support enum values with items
blender/blender
Reference: blender/blender#113445
Loading…
Reference in New Issue
No description provided.
Delete Branch "LukasTonne/blender:nodes-menu-switch"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
Menu Switch node selecting between geometries
Enum definition in the node editor side bar
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.
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.
@blender-bot package
Package build started. Download here when ready.
I think the boolean should also expose the boolean value in the socket, right now I have to use a Boolean node:
Real example (Layer selector for my tests):
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":
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:
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".
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".
Fixed. It was indeed order-dependent because one group input could reset the enum from a previous group input.
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.
@blender-bot package
Package build started. Download here when ready.
Here are my reasons for not using a different socket shapes or link drawing:
@blender-bot package
Package build started. Download here when ready.
Hi Lukas, nice work. Another round of bugs/findings:
- [ ] 4. Drawing glitch sometimes:reported separately #114461I'm also not in love with calling the socket "Switch":
Maybe we could call it "Menu" ?
@ -103,4 +103,17 @@ class StringBakeItem : public BakeItem {
}
};
class EnumBakeItem : public BakeItem {
Can just use the PrimitiveBakeItem for this, it just stores an int.
@ -993,2 +996,4 @@
break;
}
case SOCK_ENUM:
uiItemL(sub, "ENUM SOCKET TODO", ICON_NONE);
Implement menu dropdown for material views.
The node that shows up in the link search here is the regular boolean
Switch
node, not theMenu 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 newMenu
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.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.
(note: field inferencing for the output socket seems broken, looking into it)
I can't repro this. Maybe it only happened for the square sockets? Could be hardware- or driver-dependent.
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.
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.
@blender-bot package
Package build started. Download here when ready.
This appears to be a bug in RNA:item.identifier == nullptr
indicates the end of the items list (as opposed toidentifier[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.
@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.
WIP: Implementation of Menu Switch nodeto Implementation of Menu Switch nodeWe 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:
NodeDeclarationBuilder::add_input
andadd_output
.IndexSwitchItemsAccessor
? It feels like this way you could avoid a lot of the boilerplate.bNodeSocketValueMenu
has to keep a pointer to thebNodeTree
. It feels like it could just have somestd::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
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).
Removed the flag and now API calls tag the node tree directly.
@ -372,1 +392,3 @@
}
if (md->type == eModifierType_Nodes) {
MOD_nodes_update_interface(
object, (NodesModifierData *)md, /*recalc=*/result.interface_changed);
Why would we ever want to update the modifier if
interface_changed
is false? If an exposed enum changed,interface_changed
should be true.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 theMOD_nodes_update_interface
now gets called more often, potentially increasing overhead. However, only part of whatMOD_nodes_update_interface
does is necessary for enum updates, so therecalc
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.I made two separate update functions out of
MOD_nodes_update_interface
now, let me know if that looks ok:b0e76d5317
@ -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>(),
Please use
this->
for method calls in this file.@ -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 */
Wrong comment.
@ -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();
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?
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)
@ -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
What does "every time", "enum item changes" and "full interface update" mean here exactly?
Reverted by changes to
MOD_nodes_update_interface
.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 astd::unique_ptr
with a custom deleter. Have to check with core devs.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.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 toAnonymousAttributeID
). 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 newRuntimeNodeEnumItems
(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 thebNodeSocketValueMenu::items
pointer is set to null.The "conventional node storage callbacks" would just have to increase/decrease the user count of the
items
.Implementation of Menu Switch nodeto Menu Switch NodeI'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.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.
Using runtime implicit sharing data now, works great.
@ -486,6 +490,7 @@ class NodeTreeMainUpdater {
this->make_node_previews_dirty(ntree);
this->propagate_runtime_flags(ntree);
this->propagate_enum_definitions(ntree);
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 (seeadd_node_tag(pair.first, pair.second, NTREE_CHANGED_NODE_PROPERTY);
).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 genericNTREE_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 theupdate_internal_links_in_node
propagation function.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.
@ -801,6 +806,186 @@ class NodeTreeMainUpdater {
}
}
void reset_enum_ptr(bNodeSocketValueMenu &dst)
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.@ -20,3 +20,3 @@
* the values.
*/
void MOD_nodes_update_interface(Object *object, NodesModifierData *nmd);
void MOD_nodes_update_properties(NodesModifierData *nmd);
What do you think about calling these
MOD_nodes_update_interface_ui
and the other one justMOD_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.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 oneMOD_nodes_update_interface
function.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 {
This file should go in the
blender::bke
namespaceI tried that, problem is we can't reference types in a namespace from DNA ...
You can, it's done a fair amount actually. See
MeshRuntimeHandle
for an example.Ah forgot about that trick.
@ -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)) {}
Might as well define the constructor in the header, it's so trivial. No need for a
std::move
on an integer alsoRemoved enum baking, not relevant any more.
@ -922,0 +935,4 @@
} bNodeSocketValueMenu;
/* Flags for #bNodeSocketValueMenu. */
typedef enum NodeSocketValueMenuFlag {
This seems like runtime data, since it's found by doing static analysis on the tree. Could it be stored outside of DNA?
Yes this is a runtime flag. Moving the flag into
RuntimeNodeEnumItems
doesn't work though, because when there is a conflict theenum_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 inbNodeSocketValueMenu
just for a flag, i'm going to rename it toruntime_flag
and move the flag enum definition intoBKE_node_enum.hh
.@ -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,
Unused definition
@ -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})
No need to specify the default, it's already the default for the default
@ -0,0 +46,4 @@
.default_value({0.8f, 0.8f, 0.8f, 1.0f})
.supports_field();
break;
case SOCK_SHADER:
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.
@ -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();
Any particular reason to define this range? Seems unnecessary
@ -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,
There's a
socket_type_supports_fields
function for this already@ -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.
Broken English here, and I don't really get the limitation currently
@ -0,0 +265,4 @@
}
}
/* Multifunction which evaluates the switch input for each enum item and partially fills the output
use doxygen syntax
@ -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 {
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 caseDid 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.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 inIndex 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.Think we can relatively easily optimize
IndexMask::from_groups
later.The index switch node calls
materialize_to_uninitialized
for eachGVArray
-- it doesn't access each element by index itself. Can't get much better than that in terms of devirtualization.@ -0,0 +317,4 @@
bool can_be_field_ = false;
const NodeEnumDefinition &enum_def_;
const CPPType *cpp_type_;
std::unique_ptr<MultiFunction> multi_function_;
Any particular reason to store the multifunction here rather creating locally and passing ownership in
FieldOperation::Create
?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.@ -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);
Probably more semantically helpful to use
set_default_remaining_node_outputs
even if the result is the sameThere 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 thelf_index_by_bsocket
for the output is never defined (-1). This stuff is hard to follow but i thinklazy_function_interface_from_node
must be called at some point, according to the generic lazy function types.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.Reported separately for the Index Switch node #116885
@ -0,0 +387,4 @@
void execute_field(Field<int> condition, lf::Params ¶ms) const
{
if (!condition.node().depends_on_input()) {
Pretty sure this check is redundant with the earlier
is_context_dependent_field
check.@ -0,0 +500,4 @@
SOCK_GEOMETRY,
SOCK_OBJECT,
SOCK_COLLECTION,
SOCK_TEXTURE,
SOCK_TEXTURE
can be removed, it's not supported anywhere@ -484,0 +513,4 @@
bool Menu::can_connect(const bNodeSocket &socket) const
{
return sockets_can_connect(*this, socket) && socket.type == SOCK_MENU;
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.
It might work under certain circumstances, but it has limitations and changes workflow somewhat:
I suggest we keep this as a possible later feature.
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.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?
Yes, good catch.
@ -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)
?@ -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_);
@JacquesLucke There is a failing assert in
SocketValueVariant::convert_to_single
when usingset_default_remaining_node_outputs
on field outputs. TheSocketValueVariant
default value is created with aNone
kind, which is not supported. This fails duringGeoTreeLogger::log_value
.Lukas Tönne referenced this pull request2024-01-04 11:34:59 +01:00
Seems to work fine now. Did this only happen for rotation socket type? IIRC the field support wasn't consistent in the initial version.
Main issue with dark colors is that links become hard to see. I'll leave it for discussion next week.
Menu Switch Nodeto Geometry Nodes: Menu Switch NodeSome 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.
Quite noisy and hard to read.
UI meeting seems to have settled on 0.40 for the socket color:
@blender-bot build
@blender-bot build
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.
@ -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. */
Outdated comment, same below.
@ -0,0 +54,4 @@
if (!this->items().contains_ptr(&item)) {
return false;
}
const int remove_index = &item - this->items().begin();
Maybe you want to use the utility in
DNA_array_utils.hh
. Same below.@ -0,0 +75,4 @@
return;
}
this->items_num = 0;
MEM_SAFE_FREE(this->items_array);
Looks like this does not free everything in the items. Can just use the utility in
DNA_array_utils.hh
.@ -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. */
This comment and the one below don't seem to be necessary anymore.
@ -484,6 +485,9 @@ class NodeTreeMainUpdater {
this->make_node_previews_dirty(ntree);
this->propagate_runtime_flags(ntree);
if (this->propagate_enum_definitions(ntree)) {
Since this is geometry nodes only for now, it can also be moved one line down.
@ -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. */
Outdated change.
@ -995,2 +998,4 @@
break;
}
case SOCK_MENU:
uiItemL(sub, "MENU SOCKET TODO", ICON_NONE);
Change to something like
RPT_("Unsupported Menu Socket")
@ -7941,7 +7941,6 @@ static void rna_def_modifier_grease_pencil_tint(BlenderRNA *brna)
RNA_def_property_update(prop, 0, "rna_Modifier_update");
}
Unrelated change.
Doesn't show up in the changes any more, guess is outdated.
@ -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);
unrelated change
@ -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")
Should be moved one line up.
@ -0,0 +44,4 @@
SOCK_COLLECTION,
SOCK_MATERIAL,
SOCK_IMAGE,
SOCK_MENU);
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.
Yeah, can be made to work, but i really don't want to spend more time on this PR.
@ -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);
Bit confusing to have a
default_value
offalse
. Probably shouldn't have a default value at all.@ -0,0 +63,4 @@
}
for (const NodeEnumItem &enum_item : storage.enum_definition.items()) {
const std::string identifier = "Item" + std::to_string(enum_item.identifier);
Use
"Item_"
, just to be a little bit more consistent with other such lists (e.g. inNOD_zone_socket_items.hh
).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 {
No need for
blender::
here@ -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);
Unused variable (in release builds anyway)
@blender-bot build
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.
@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:
I was going to use the Menu Switch today but realized that this doesn't work.
But after some thinking and guessing I found out that this works, which I guess is fine for this example.
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.
What I want to do is this instead but it doesn't work, the menu in the modifier goes blank.
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
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.@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 :)
Hey. Could you guys take a look at my proposal on Right-Click Select please?
@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.
Access to Menu Index can still be retrieved through grouping
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.