Details

Reviewers

Maniphest Tasks

T92825: Refactor node editor add menu and search

Commits

rB837144b4577f: Nodes: Use plain menus for geometry nodes add menu

Summary

At the cost of slightly more boilerplate code, we can avoid the NodeItem
and NodeCategory abstractions used to build the node add menu.
This makes the menus more flexible and more obvious, which will
make them easier to extend with assets.

The identifiers for the new menu types are inconsistent with regular
class naming for backwards compatibility with the old "category"
menu naming.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Sep 14 2022, 9:14 PM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) added a task: T92825: Refactor node editor add menu and search.

Fixes for translation

Cleanup, fixes

Iliya Katueshenock (Moder) awarded a token.Sep 19 2022, 7:36 PM

node_add_type() should also be able to accept custom settings for each entry.

In D15973#435563, @Miguel Porces (cmporces) wrote:

node_add_type() should also be able to accept custom settings for each entry.

It can, it just works a bit differently. See the code for the "Mix Color" entry:

props = node_add_menu.add_node_type(layout, "ShaderNodeMix", label=iface_("Mix Color"))
ops = props.settings.add()
ops.name = "data_type"
ops.value = "'RGBA'"

Hans Goudey (HooglyBoogly) retitled this revision from Nodes: Use plain menus for geometry nodes add menu (WIP) to Nodes: Use plain menus for geometry nodes add menu.Sep 20 2022, 5:31 AM

Jacques Lucke (JacquesLucke) requested changes to this revision.Sep 20 2022, 11:53 AM

Jacques Lucke (JacquesLucke) added inline comments.

release/scripts/startup/bl_ui/node_add_menu.py
10	Enforce that `label` is passed as a keyword (instead of positional argument) like so: `def add_node_type(layout, node_type, *, label=None):`
20	This doesn't look very recursive to me.
39	Could gather a list of all node groups first and only add them to the layout afterwards. This way the `added_separate` thing shouldn't be necessary.
46	Missing `.`
release/scripts/startup/bl_ui/node_add_menu_geometry.py
10	Do you happen to know if this menu idname is the same as before? Would be good to keep it consistent to avoid breaking some addons maybe.
326	This does not work. `node_add_menu` is not a top level module. Traceback (most recent call last): File "/home/jacques/blender/build_release/bin/3.4/scripts/startup/bl_ui/node_add_menu_geometry.py", line 325, in draw from node_add_menu import draw_node_group_add_menu ModuleNotFoundError: No module named 'node_add_menu' Traceback (most recent call last): File "/home/jacques/blender/build_release/bin/3.4/scripts/startup/bl_ui/node_add_menu_geometry.py", line 325, in draw from node_add_menu import draw_node_group_add_menu ModuleNotFoundError: No module named 'node_add_menu' Since `node_add_menu` is imported already, use `node_add_menu.draw_node_group_add_menu`.

This revision now requires changes to proceed.Sep 20 2022, 11:53 AM

Hans Goudey (HooglyBoogly) marked 4 inline comments as done.Sep 20 2022, 5:58 PM

Hans Goudey (HooglyBoogly) added inline comments.

release/scripts/startup/bl_ui/node_add_menu.py
20	Oops, forgot to add that part I suppose, thanks for catching that. Using a stack seemed easier than actual recursion, but I think the name is fine still.
39	Good idea
release/scripts/startup/bl_ui/node_add_menu_geometry.py
10	Pretty sure they were `NODE_MT_category_GEO_ATTRIBUTE`, `NODE_MT_category_GEO_GEOMETRY` etc. Given those inconsistent names, I'd think it's better to just use the regular naming convention consistently. Plus, the end goal here is adding node assets to the menus, which means there's less need to extend the menus in the first place. Don't have a strong opinion on this one though.

Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Sep 20 2022, 5:58 PM

Changes based on review, some cleanup

Jacques Lucke (JacquesLucke) requested changes to this revision.Sep 21 2022, 1:42 PM

Jacques Lucke (JacquesLucke) added inline comments.

release/scripts/startup/bl_ui/node_add_menu.py
21	typo (`recursion`)
29	Why does this make sense?
60	If I'm not mistaken, this logic is wrong. We want to know which groups contain the current group and not which groups does the current group contain. Maybe just reused the existing code for now,
64	Didn't even know you could use multiple `if` like that (would have concatenated the conditions with `and`.
68	I wonder if we should just gray out node groups that can't be added to keep the menu more consistent.
70	You checked for these things above already.
release/scripts/startup/bl_ui/node_add_menu_geometry.py
10	Since the name doesn't really matter for much for us, I think it would be better to not change it now. We could still change it in the future potentially when we do have the asset system but I see no strong reason for breaking addons here. Not only the name of the submenus should stay the same, but also the name of the entire add menu.

This revision now requires changes to proceed.Sep 21 2022, 1:42 PM

Hans Goudey (HooglyBoogly) marked 8 inline comments as done.Sep 22 2022, 6:58 PM

Hans Goudey (HooglyBoogly) added inline comments.

release/scripts/startup/bl_ui/node_add_menu.py
60	You're right, I was thinking about this wrong. I'll use the old quadratic code.
64	Found it on stack overflow. I'll change to the more expected way :)
68	If we did that I think it would be nice to do it with an operator poll message so we could have a poll message. So maybe we can leave it for later.
release/scripts/startup/bl_ui/node_add_menu_geometry.py
10	Okay, it was a bit painful, but it but I moved back to the old category names. The way the menu is drawn with `menu_contents`, I can't use the old add menu identifier since it is still used.