⚙ D10623 Geometry Nodes: Add domain and data type to attribute search

Hans Goudey (HooglyBoogly) requested review of this revision.Mar 5 2021, 2:12 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B13271: Diff 34781.Mar 5 2021, 2:12 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Mar 5 2021, 2:14 AM

Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.

Hans Goudey (HooglyBoogly) added tasks: T85280: Attribute search (dropdown) for polishing, T85881: User interface improvements for attribute search.Mar 5 2021, 2:16 AM

Hans Goudey (HooglyBoogly) mentioned this in T85881: User interface improvements for attribute search.Mar 5 2021, 3:43 AM

@Hans Goudey (HooglyBoogly) What about (socket color)[category] -> name [Domain]? Did you get to try that?

(just reading the reasoning in the other thread I will comment there instead)

Florian Kamenz (TimBrown) awarded a token.Mar 5 2021, 3:56 PM

We talked about this and agreed on:

Wait until spreadsheet implements access to the categories to be able to easily use it here.
Expose more built-in attributes that work in different domains other than point.
Then have both options for the team to test
- The original design
- The variation where the domain is presented in the left side.

Tomas (Rusculleda) awarded a token.Mar 5 2021, 6:49 PM

Since there's also some uncertainty / disagreement about design for exposing the "Category", I think it also makes some time to let that sit before moving forward here.

Anyway, there are bugs with the current patch, so planning changes here anyway.

Hans Goudey (HooglyBoogly) mentioned this in T86611: Support Attribute Search In Material Nodes.Mar 16 2021, 2:41 AM

Rebase on top of current master, use a set for attribute data

Harbormaster completed remote builds in B13650: Diff 35458.Mar 22 2021, 2:54 AM

Charlie Jolly (charlie) mentioned this in T86749: Geometry Node Community Additions.Mar 27 2021, 1:42 AM

Merge branch 'master' into geometry-nodes-attribute-search-data
Remove commented code
Enable the exec function (search won't work at the moment)

Harbormaster completed remote builds in B13957: Diff 36004.Apr 8 2021, 10:25 PM

I have the necessary changes to make this work right here: P2060 I'll try to get feedback from other UI developers before going further with that though

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 8 2021, 10:27 PM

Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).

Remove unecessary butstore stuff
Use solution to search button problem from Julian
Small comment cleanup

Harbormaster completed remote builds in B14022: Diff 36108.Apr 12 2021, 10:53 PM

Hans Goudey (HooglyBoogly) retitled this revision from Geometry Nodes: Add domain and data type to attribute search (WIP) to Geometry Nodes: Add domain and data type to attribute search.Apr 12 2021, 10:54 PM

I don't know why, but somehow this breaks the normal node search. It always inserts an Attribute Clamp node for me now.

source/blender/editors/space_node/node_geometry_attribute_search.cc
67	`nullptr`, same below.

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 13 2021, 11:08 AM

This revision now requires changes to proceed.Apr 13 2021, 11:08 AM

Fix NULL -> nullptr
Use a different method for making search work, no more dupalloc

Harbormaster completed remote builds in B14038: Diff 36128.Apr 13 2021, 3:38 PM

Julian Eisel (Severin) requested changes to this revision.Apr 13 2021, 3:45 PM

Julian Eisel (Severin) added a subscriber: Julian Eisel (Severin).

Julian Eisel (Severin) added inline comments.

source/blender/editors/interface/interface_templates.c
314	bool and pointer seem mixed up here?
source/blender/editors/space_node/node_geometry_attribute_search.cc
149	Just use `MAX_NAME`.
176–178	When UI code frees this later it will use regular `MEM_freeN()`, so this won't call the destructor. I'd suggest either also calling `MEM_mallocN()` here directly, or clearly documenting that `AttributeSearchData` must not have a destructor! The third possibility is bringing back the `MEM_dupallocN()`.

This revision now requires changes to proceed.Apr 13 2021, 3:45 PM

Review updates from Julian

Harbormaster completed remote builds in B14039: Diff 36129.Apr 13 2021, 3:57 PM

Hans Goudey (HooglyBoogly) added inline comments.Apr 13 2021, 3:59 PM

source/blender/editors/interface/interface_templates.c
314	Good catch.
source/blender/editors/space_node/node_geometry_attribute_search.cc
176–178	Good points. I think I'll go with the documentation option, since that way: I can still construct the data with the const references to node tree and node that can't be changed afterwards It doesn't require the inefficiency of `MEM_dupallocN`

Add dummy available attribute info to store temporary button string

This isn't the prettiest thing, but it solves a significant problem, since the exec callback
needs information for when you type in a name of a new attribute or an attribute that the
search doesn't know about. I tried approaching this another way, by giving a callback to
the exec function with the current string from the button, but I think that's far more hacky.
For the records, that attempt is here:

P2066 UI code hack for D10623 part 2

1	diff --git a/source/blender/editors/include/UI_interface.h b/source/blender/editors/include/UI_interface.h
2	index 7a189139358..2300b4429a6 100644
3	--- a/source/blender/editors/include/UI_interface.h
4	+++ b/source/blender/editors/include/UI_interface.h
5	@@ -507,6 +507,10 @@ typedef void (uiButSearchUpdateFn)(const struct bContext C,
6	const char *str,
7	uiSearchItems *items,
8	const bool is_first);
9	+typedef void (uiButSearchExecFn)(struct bContext C,
10	+ void *arg1,
11	+ void *arg2,
12	+ const char *current_button_str);
13	typedef void (uiButSearchArgFreeFn)(void arg);
14	typedef bool (uiButSearchContextMenuFn)(struct bContext C,
15	void *arg,
16	@@ -1602,7 +1606,7 @@ void UI_but_func_search_set(uiBut *but,
17	void *arg,
18	const bool free_arg,
19	uiButSearchArgFreeFn search_arg_free_fn,
20	- uiButHandleFunc search_exec_fn,
21	+ uiButSearchExecFn search_exec_fn,
22	void *active);
23	void UI_but_func_search_set_context_menu(uiBut *but, uiButSearchContextMenuFn context_menu_fn);
24	void UI_but_func_search_set_tooltip(uiBut *but, uiButSearchTooltipFn tooltip_fn);
25	diff --git a/source/blender/editors/interface/interface.c b/source/blender/editors/interface/interface.c
26	index 26136f8d905..e0709538441 100644
27	--- a/source/blender/editors/interface/interface.c
28	+++ b/source/blender/editors/interface/interface.c
29	@@ -6606,9 +6606,10 @@ uiBut uiDefSearchBut(uiBlock block,
30	* should be freed when the button is destroyed.
31	* \param search_arg_free_fn: When non-null, use this function to free \a arg.
32	* \param search_exec_fn: Function that executes the action, gets \a arg as the first argument.
33	- * The second argument as the active item-pointer
34	- * \param active: When non-null, this item-pointer item will be visible and selected,
35	- * otherwise the first item will be selected.
36	+ * The second argument as the active item-pointer, and the third argument is the string currently
37	+ * displayed in the button, for situations where there is no corresponding item.
38	+ * \param active: When non-null, this item-pointer item will be visible and selected, otherwise the
39	+ * first item will be selected.
40	*/
41	void UI_but_func_search_set(uiBut *but,
42	uiButSearchCreateFn search_create_fn,
43	@@ -6616,7 +6617,7 @@ void UI_but_func_search_set(uiBut *but,
44	void *arg,
45	const bool free_arg,
46	uiButSearchArgFreeFn search_arg_free_fn,
47	- uiButHandleFunc search_exec_fn,
48	+ uiButSearchExecFn search_exec_fn,
49	void *active)
50	{
51	uiButSearch search_but = (uiButSearch )but;
52	@@ -6638,6 +6639,8 @@ void UI_but_func_search_set(uiBut *but,
53	search_but->items_update_fn = search_update_fn;
54	search_but->item_active = active;
55
56	+ search_but->exec_fn = search_exec_fn;
57	+
58	search_but->arg = arg;
59	search_but->arg_free_fn = search_arg_free_fn;
60
61	@@ -6658,8 +6661,9 @@ void UI_but_func_search_set(uiBut *but,
62	}
63	}
64
65	- /* search buttons show red-alert if item doesn't exist, not for menus */
66	- if (0 == (but->block->flag & UI_BLOCK_LOOP)) {
67	+ /* search buttons show red-alert if item doesn't exist, not for menus. Don't do this for
68	+ * buttons where any result is valid anyway, since any string will be valid anyway. */
69	+ if (0 == (but->block->flag & UI_BLOCK_LOOP) && !search_but->results_are_suggestions) {
70	/* skip empty buttons, not all buttons need input, we only show invalid */
71	if (but->drawstr[0]) {
72	ui_but_search_refresh(search_but);
73	@@ -6756,7 +6760,10 @@ static void operator_enum_search_update_fn(const struct bContext *C,
74	}
75	}
76
77	-static void operator_enum_search_exec_fn(struct bContext UNUSED(C), void but, void *arg2)
78	+static void operator_enum_search_exec_fn(struct bContext *UNUSED(C),
79	+ void *but,
80	+ void *arg2,
81	+ const char *UNUSED(current_button_str))
82	{
83	wmOperatorType ot = ((uiBut )but)->optype;
84	PointerRNA opptr = UI_but_operator_ptr_get(but); / Will create it if needed! */
85	diff --git a/source/blender/editors/interface/interface_handlers.c b/source/blender/editors/interface/interface_handlers.c
86	index a5a5a69728e..26b2576e4a6 100644
87	--- a/source/blender/editors/interface/interface_handlers.c
88	+++ b/source/blender/editors/interface/interface_handlers.c
89	@@ -451,7 +451,9 @@ typedef struct uiAfterFunc {
90	PropertyRNA *rnaprop;
91
92	void *search_arg;
93	+ uiButSearchExecFn search_exec_fn;
94	uiButSearchArgFreeFn search_arg_free_fn;
95	+ const char *current_button_str;
96
97	bContextStore *context;
98
99	@@ -803,9 +805,11 @@ static void ui_apply_but_func(bContext C, uiBut but)
100	if (but->type == UI_BTYPE_SEARCH_MENU) {
101	uiButSearch search_but = (uiButSearch )but;
102	after->search_arg_free_fn = search_but->arg_free_fn;
103	+ after->search_exec_fn = search_but->exec_fn;
104	after->search_arg = search_but->arg;
105	search_but->arg_free_fn = NULL;
106	search_but->arg = NULL;
107	+ after->current_button_str = but->drawstr;
108	}
109
110	if (but->context) {
111	@@ -953,9 +957,14 @@ static void ui_apply_but_funcs_after(bContext *C)
112	if (after.func) {
113	after.func(C, after.func_arg1, after.func_arg2);
114	}
115	- if (after.funcN) {
116	- after.funcN(C, after.func_argN, after.func_arg2);
117	+ // if (after.funcN) {
118	+ // after.funcN(C, after.func_argN, after.func_arg2);
119	+ // }
120	+
121	+ if (after.search_exec_fn) {
122	+ after.search_exec_fn(C, after.func_argN, after.func_arg2, after.current_button_str);
123	}
124	+
125	if (after.func_argN) {
126	MEM_freeN(after.func_argN);
127	}
128	diff --git a/source/blender/editors/interface/interface_intern.h b/source/blender/editors/interface/interface_intern.h
129	index 4c96512b4f3..ddf17696717 100644
130	--- a/source/blender/editors/interface/interface_intern.h
131	+++ b/source/blender/editors/interface/interface_intern.h
132	@@ -308,6 +308,7 @@ typedef struct uiButSearch {
133
134	uiButSearchCreateFn popup_create_fn;
135	uiButSearchUpdateFn items_update_fn;
136	+ uiButSearchExecFn exec_fn;
137	void *item_active;
138
139	void *arg;
140	diff --git a/source/blender/editors/interface/interface_template_search_menu.c b/source/blender/editors/interface/interface_template_search_menu.c
141	index 91ad6619889..ee9751f4550 100644
142	--- a/source/blender/editors/interface/interface_template_search_menu.c
143	+++ b/source/blender/editors/interface/interface_template_search_menu.c
144	@@ -934,7 +934,10 @@ static void menu_search_arg_free_fn(void *data_v)
145	MEM_freeN(data);
146	}
147
148	-static void menu_search_exec_fn(bContext C, void UNUSED(arg1), void *arg2)
149	+static void menu_search_exec_fn(bContext *C,
150	+ void *UNUSED(arg1),
151	+ void *arg2,
152	+ const char *UNUSED(current_button_str))
153	{
154	struct MenuSearch_Item *item = arg2;
155	if (item == NULL) {
156	diff --git a/source/blender/editors/interface/interface_template_search_operator.c b/source/blender/editors/interface/interface_template_search_operator.c
157	index 2b765a1a2f5..0b840a75a36 100644
158	--- a/source/blender/editors/interface/interface_template_search_operator.c
159	+++ b/source/blender/editors/interface/interface_template_search_operator.c
160	@@ -47,7 +47,10 @@
161	/** \name Operator Search Template Implementation
162	* \{ */
163
164	-static void operator_search_exec_fn(bContext C, void UNUSED(arg1), void *arg2)
165	+static void operator_search_exec_fn(bContext *C,
166	+ void *UNUSED(arg1),
167	+ void *arg2,
168	+ const char *UNUSED(current_button_str))
169	{
170	wmOperatorType *ot = arg2;
171
172	diff --git a/source/blender/editors/interface/interface_templates.c b/source/blender/editors/interface/interface_templates.c
173	index 760fbe75688..6f6d8f36eee 100644
174	--- a/source/blender/editors/interface/interface_templates.c
175	+++ b/source/blender/editors/interface/interface_templates.c
176	@@ -233,7 +233,7 @@ static uiBlock template_common_search_menu(const bContext C,
177	ARegion *region,
178	uiButSearchUpdateFn search_update_fn,
179	void *search_arg,
180	- uiButHandleFunc search_exec_fn,
181	+ uiButSearchExecFn search_exec_fn,
182	void *active_item,
183	uiButSearchTooltipFn item_tooltip_fn,
184	const int preview_rows,
185	@@ -345,7 +345,10 @@ typedef struct TemplateID {
186	} TemplateID;
187
188	/* Search browse menu, assign */
189	-static void template_ID_set_property_exec_fn(bContext C, void arg_template, void *item)
190	+static void template_ID_set_property_exec_fn(bContext *C,
191	+ void *arg_template,
192	+ void *item,
193	+ const char *UNUSED(current_button_str))
194	{
195	TemplateID template_ui = (TemplateID )arg_template;
196
197	@@ -1654,7 +1657,10 @@ typedef struct TemplateSearch {
198	int preview_rows, preview_cols;
199	} TemplateSearch;
200
201	-static void template_search_exec_fn(bContext C, void arg_template, void *item)
202	+static void template_search_exec_fn(bContext *C,
203	+ void *arg_template,
204	+ void *item,
205	+ const char *UNUSED(current_button_str))
206	{
207	TemplateSearch *template_search = arg_template;
208	uiRNACollectionSearch *coll_search = &template_search->search_data;
209	diff --git a/source/blender/editors/space_node/node_geometry_attribute_search.cc b/source/blender/editors/space_node/node_geometry_attribute_search.cc
210	index 145c1659ac0..7422333bc72 100644
211	--- a/source/blender/editors/space_node/node_geometry_attribute_search.cc
212	+++ b/source/blender/editors/space_node/node_geometry_attribute_search.cc
213	@@ -100,14 +100,14 @@ static void attribute_search_update_fn(
214	/* Note that the attribute domain and data type are dummies, since
215	* #AvailableAttributeInfo equality is only based on the string. */
216	if (!attribute_hints.contains(AvailableAttributeInfo{str, ATTR_DOMAIN_AUTO, CD_PROP_BOOL})) {
217	- UI_search_item_add(items, str, (void *)str, ICON_ADD, 0, 0);
218	+ UI_search_item_add(items, str, nullptr, ICON_ADD, 0, 0);
219	}
220	}
221
222	if (str[0] == '\0' && !is_first) {
223	/* Allow clearing the text field when the string is empty, but not on the first pass,
224	* or opening an attribute field for the first time would show this search item. */
225	- UI_search_item_add(items, str, (void *)str, ICON_X, 0, 0);
226	+ UI_search_item_add(items, str, nullptr, ICON_X, 0, 0);
227	}
228
229	/* Don't filter when the menu is first opened, but still run the search
230	@@ -133,14 +133,22 @@ static void attribute_search_update_fn(
231	BLI_string_search_free(search);
232	}
233
234	-static void attribute_search_exec_fn(bContext UNUSED(C), void data_v, void *item_v)
235	+static void attribute_search_exec_fn(bContext *UNUSED(C),
236	+ void *data_v,
237	+ void *item_v,
238	+ const char *current_button_str)
239	{
240	AttributeSearchData data = static_cast<AttributeSearchData >(data_v);
241	AvailableAttributeInfo item = static_cast<AvailableAttributeInfo >(item_v);
242
243	bNodeSocket &socket = data->socket;
244	bNodeSocketValueString value = static_cast<bNodeSocketValueString >(socket.default_value);
245	- BLI_strncpy(value->value, item->name.c_str(), MAX_NAME);
246	+ if (item) {
247	+ BLI_strncpy(value->value, item->name.c_str(), MAX_NAME);
248	+ }
249	+ else {
250	+ BLI_strncpy(value->value, current_button_str, MAX_NAME);
251	+ }
252	}
253
254	void node_geometry_add_attribute_search_button(const bNodeTree *node_tree,
255	diff --git a/source/blender/editors/space_node/node_select.c b/source/blender/editors/space_node/node_select.c
256	index de63aa07acb..0260a4af7cb 100644
257	--- a/source/blender/editors/space_node/node_select.c
258	+++ b/source/blender/editors/space_node/node_select.c
259	@@ -1220,7 +1220,10 @@ static void node_find_update_fn(const struct bContext *C,
260	BLI_string_search_free(search);
261	}
262
263	-static void node_find_exec_fn(struct bContext C, void UNUSED(arg1), void *arg2)
264	+static void node_find_exec_fn(struct bContext *C,
265	+ void *UNUSED(arg1),
266	+ void *arg2,
267	+ const char *UNUSED(current_button_str))
268	{
269	SpaceNode *snode = CTX_wm_space_node(C);
270	bNode *active = arg2;
271	diff --git a/source/blender/editors/space_outliner/outliner_tools.c b/source/blender/editors/space_outliner/outliner_tools.c
272	index c7b46cd640b..600f7ba2fb5 100644
273	--- a/source/blender/editors/space_outliner/outliner_tools.c
274	+++ b/source/blender/editors/space_outliner/outliner_tools.c
275	@@ -568,7 +568,10 @@ static void merged_element_search_update_fn(const bContext *UNUSED(C),
276	}
277
278	/* Activate an element from the merged element search menu */
279	-static void merged_element_search_exec_fn(struct bContext C, void UNUSED(arg1), void *element)
280	+static void merged_element_search_exec_fn(struct bContext *C,
281	+ void *UNUSED(arg1),
282	+ void *element,
283	+ const char *UNUSED(current_button_str))
284	{
285	SpaceOutliner *space_outliner = CTX_wm_space_outliner(C);
286	TreeElement te = (TreeElement )element;

Harbormaster completed remote builds in B14045: Diff 36137.Apr 13 2021, 6:40 PM

Julian Eisel (Severin) accepted this revision.Apr 13 2021, 8:00 PM

Jacques Lucke (JacquesLucke) accepted this revision.Apr 14 2021, 9:24 AM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/BKE_node_ui_storage.hh
88	Use the `blender::get_default_hash` function.
99	Right now this does not handle the case when the same attribute exists on multiple domains (that is possible when there are instances). One simple solution for now would be to always display the domain with the most information, because that is the attribute that would remain after making it real. Note that this does not always work once we have curves, but it's good enough for now. Alternatively, you could store multiple domains and data types in `AvailableAttributeInfo`. We might have to do that eventually, but it's not really super necessary. same_attribute_on_different_domains.blend792 KBDownload
source/blender/editors/space_node/node_geometry_attribute_search.cc
50	You could make this more explicit with a static assert that checks for `std::is_trivially_destructible_v`.

This revision is now accepted and ready to land.Apr 14 2021, 9:24 AM

@Hans Goudey (HooglyBoogly) I find it quite hard to read the datatype of the selected attribute:

Maybe use the same color as the domain so it shows white when highlighted?
(note to patch, remember once the patch is in to add an image of it in the release notes)

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Apr 14 2021, 6:01 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/BKE_node_ui_storage.hh
99	Thanks. IIRC the reason to use a set here was to make sure each item could be discrete. (mostly writing that as a reminder to myself) Another option would be to just add multiple items to the set for different attributes. I think that's probably nicer if we ever use the attribute hint information to automatically set data type and domain enums. I know Dalai mentioned that for the attribute processor. I'd like to deal with that as a separate change, I hope that's okay with you.
source/blender/editors/space_node/node_geometry_attribute_search.cc
50	Good idea!

Closed by commit rB71eaf872c2db: Geometry Nodes: Add domain and data type to attribute search (authored by Hans Goudey (HooglyBoogly)). · Explain WhyApr 14 2021, 6:12 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) marked an inline comment as done.

Hans Goudey (HooglyBoogly) added a commit: rB71eaf872c2db: Geometry Nodes: Add domain and data type to attribute search.

Damian (damian) awarded a token.Apr 14 2021, 6:36 PM

What about Icons?
It would also be a little more consistent with other search boxes.

I tried icons with an earlier version of this patch:

There are a few issues with it:

We refer to domains by name in other places, so it's not as clear what the meaning is with only icons
We don't have a cohesive set of good looking icons for attribute domains, so it looks wonky
I'm not sure about the consistency argument, we have plenty of search items without icons
We have plenty of space in the search box for the name of the domain instead, it also mirrors the data type text design

I do sort of like the icons, but not enough to overcome those issues IMO.

Erik Abrahamsson (erik85) mentioned this in T87567: Crash when typing in attribute to a node that doesn't have a linkage in Geometry Node..Apr 16 2021, 8:10 PM

Geometry Nodes: Add domain and data type to attribute search
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 36137

source/blender/blenkernel/BKE_node_ui_storage.hh

source/blender/blenkernel/intern/node_ui_storage.cc

source/blender/editors/include/UI_interface.h

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_layout.c

source/blender/editors/interface/interface_template_search_menu.c

source/blender/editors/interface/interface_template_search_operator.c

source/blender/editors/interface/interface_templates.c

source/blender/editors/space_node/node_geometry_attribute_search.cc

source/blender/editors/space_node/node_select.c

source/blender/editors/space_outliner/outliner_tools.c

Geometry Nodes: Add domain and data type to attribute searchClosedPublicActions