Geometry Nodes: support attaching gizmos to input values #112677

Merged
Jacques Lucke merged 390 commits from JacquesLucke/blender:geometry-nodes-gizmos into main 2024-07-10 16:19:06 +02:00
Member

This adds support for attaching gizmos for input values. The goal is to make it easier for users to set input values intuitively in the 3D viewport.

We went through multiple different possible designs until we settled on the one implemented here. We picked it for it's flexibility and ease of use when using geometry node assets. The core principle in the design is that gizmos are attached to existing input values instead of being the input value themselves. This actually fits the existing concept of gizmos in Blender well, but may be a bit unintutitive in a node setup at first. The attachment is done using links in the node editor.

The most basic usage of the node is to link a Value node to the new Linear Gizmo node. This attaches the gizmo to the input value and allows you to change it from the 3D view. The attachment is indicated by the gizmo icon in the sockets which are controlled by a gizmo as well as the back-link (notice the double link) when the gizmo is active.
image

The core principle makes it straight forward to control the same node setup from the 3D view with gizmos, or by manually changing input values, or by driving the input values procedurally.
image

If the input value is controlled indirectly by other inputs, it's often possible to automatically propagate the gizmo to the actual input.
image

Backpropagation does not work for all nodes, although more nodes can be supported over time.

This patch adds the first three gizmo nodes which cover common use cases:

  • Linear Gizmo: Creates a gizmo that controls a float or integer value using a linear movement of e.g. an arrow in the 3D viewport.
  • Dial Gizmo: Creates a circular gizmo in the 3D viewport that can be rotated to change the attached angle input.
  • Transform Gizmo: Creates a simple gizmo for location, rotation and scale.

In the future, more built-in gizmos and potentially the ability for custom gizmos could be added.

All gizmo nodes have a Transform geometry output. Using it is optional but it is recommended when the gizmo is used to control inputs that affect a geometry. When it is used, Blender will automatically transform the gizmos together with the geometry that they control. To achieve this, the output should be merged with the generated geometry using the Join Geometry node. The data contained in Transform output is not visible geometry, but just internal information that helps Blender to give a better user experience when using gizmos.

image

The gizmo nodes have a multi-input socket. This allows controlling multiple values with the same gizmo.

Only a small set of gizmo shapes is supported initially. It might be extended in the future but one goal is to give the gizmos used by different node group assets a familiar look and feel. A similar constraint exists for colors. Currently, one can choose from a fixed set of colors which can be modified in the theme settings.

The set of visible gizmos is determined by a multiple factors because it's not really feasible to show all possible gizmos at all times. To see any of the geometry nodes gizmos, the "Active Modifier" option has to be enabled in the "Viewport Gizmos" popover. Then all gizmos are drawn for which at least one of the following is true:

  • The gizmo controls an input of the active modifier of the active object.
  • The gizmo controls a value in a selected node in an open node editor.
  • The gizmo controls a pinned value in an open node editor. Pinning works by clicking the gizmo icon next to the value.

image

Potential future improvements:

  • More gizmo nodes.
  • More inverse implementations of nodes.
  • More fine grained control over which gizmos are shown.
  • Support propagating gizmos back to attributes.
  • Support propagating gizmos through drivers.
  • Add screen space and 2D gizmos to transform gizmo.
  • Support holding down the alt key to ignore soft limits.
  • Add gizmos to some built-in nodes.

Links:


Some more impressions:
image
image

This adds support for attaching gizmos for input values. The goal is to make it easier for users to set input values intuitively in the 3D viewport. We went through multiple different possible designs until we settled on the one implemented here. We picked it for it's flexibility and ease of use when using geometry node assets. The core principle in the design is that **gizmos are attached to existing input values instead of being the input value themselves**. This actually fits the existing concept of gizmos in Blender well, but may be a bit unintutitive in a node setup at first. The attachment is done using links in the node editor. The most basic usage of the node is to link a Value node to the new Linear Gizmo node. This attaches the gizmo to the input value and allows you to change it from the 3D view. The attachment is indicated by the gizmo icon in the sockets which are controlled by a gizmo as well as the back-link (notice the double link) when the gizmo is active. ![image](/attachments/2009752d-0bb2-439c-96ec-19507608084f) The core principle makes it straight forward to control the same node setup from the 3D view with gizmos, or by manually changing input values, or by driving the input values procedurally. ![image](/attachments/3a8474c3-09e4-4184-9605-2bb417065984) If the input value is controlled indirectly by other inputs, it's often possible to **automatically propagate** the gizmo to the actual input. ![image](/attachments/3c506c48-8413-4bbd-93e9-ebaf767968d9) Backpropagation does not work for all nodes, although more nodes can be supported over time. This patch adds the first three gizmo nodes which cover common use cases: * **Linear Gizmo**: Creates a gizmo that controls a float or integer value using a linear movement of e.g. an arrow in the 3D viewport. * **Dial Gizmo**: Creates a circular gizmo in the 3D viewport that can be rotated to change the attached angle input. * **Transform Gizmo**: Creates a simple gizmo for location, rotation and scale. In the future, more built-in gizmos and potentially the ability for custom gizmos could be added. All gizmo nodes have a **Transform** geometry output. Using it is optional but it is recommended when the gizmo is used to control inputs that affect a geometry. When it is used, Blender will automatically transform the gizmos together with the geometry that they control. To achieve this, the output should be merged with the generated geometry using the *Join Geometry* node. The data contained in *Transform* output is not visible geometry, but just internal information that helps Blender to give a better user experience when using gizmos. ![image](/attachments/bc777d62-1ae3-47dc-afbd-0e31b86b8989) The gizmo nodes have a multi-input socket. This allows **controlling multiple values** with the same gizmo. Only a small set of **gizmo shapes** is supported initially. It might be extended in the future but one goal is to give the gizmos used by different node group assets a familiar look and feel. A similar constraint exists for **colors**. Currently, one can choose from a fixed set of colors which can be modified in the theme settings. The set of **visible gizmos** is determined by a multiple factors because it's not really feasible to show all possible gizmos at all times. To see any of the geometry nodes gizmos, the "Active Modifier" option has to be enabled in the "Viewport Gizmos" popover. Then all gizmos are drawn for which at least one of the following is true: * The gizmo controls an input of the active modifier of the active object. * The gizmo controls a value in a selected node in an open node editor. * The gizmo controls a pinned value in an open node editor. Pinning works by clicking the gizmo icon next to the value. ![image](/attachments/cdb8196e-ee03-499b-99d9-aba60ca73391) Potential future improvements: * More gizmo nodes. * More inverse implementations of nodes. * More fine grained control over which gizmos are shown. * Support propagating gizmos back to attributes. * Support propagating gizmos through drivers. * Add screen space and 2D gizmos to transform gizmo. * Support holding down the `alt` key to ignore soft limits. * Add gizmos to some built-in nodes. Links: * Workshop 2023: https://code.blender.org/2023/11/geometry-nodes-workshop-november-2023/#gizmos * Workshop 2024: https://devtalk.blender.org/t/2024-05-13-geometry-nodes-workshop-notes/34760#gizmos-3 * Feedback Thread: https://devtalk.blender.org/t/geometry-nodes-gizmos-feedback/32073 ---- Some more impressions: ![image](/attachments/3c6275f2-1000-4539-9e99-10473f88b97a) ![image](/attachments/83ce7680-dc35-43c1-8292-b93f6d535a4a)
Jacques Lucke added 11 commits 2023-09-21 17:09:42 +02:00
Jacques Lucke added 2 commits 2023-09-21 19:46:41 +02:00
Jacques Lucke added 2 commits 2023-09-21 20:21:12 +02:00
Jacques Lucke added 7 commits 2023-09-22 11:53:21 +02:00
Jacques Lucke added 5 commits 2023-09-22 14:29:28 +02:00
Jacques Lucke added 1 commit 2023-09-22 16:06:26 +02:00
Jacques Lucke added 10 commits 2023-09-24 20:53:59 +02:00
Jacques Lucke added 1 commit 2023-09-25 14:54:56 +02:00
Jacques Lucke added 1 commit 2023-09-26 12:22:45 +02:00
Jacques Lucke added 1 commit 2023-09-26 12:38:36 +02:00
Jacques Lucke added 3 commits 2023-09-27 15:12:51 +02:00
Jacques Lucke added 1 commit 2023-10-02 16:23:55 +02:00
Jacques Lucke added 2 commits 2023-10-03 14:15:39 +02:00
Jacques Lucke added 5 commits 2023-10-04 20:32:17 +02:00
Jacques Lucke added 3 commits 2023-10-08 17:57:33 +02:00
Jacques Lucke added 1 commit 2023-10-08 19:03:14 +02:00
Jacques Lucke added 3 commits 2023-10-08 19:41:36 +02:00
Jacques Lucke added 2 commits 2023-10-22 12:45:46 +02:00
Jacques Lucke added 1 commit 2023-10-22 13:07:14 +02:00
Jacques Lucke added 1 commit 2023-11-02 13:36:25 +01:00
Merge branch 'main' into geometry-nodes-gizmos
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
91c41c8bd1
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/PR112677) when ready.
Jacques Lucke added 1 commit 2023-11-14 12:29:20 +01:00
Jacques Lucke added 1 commit 2023-11-14 12:36:03 +01:00
Jacques Lucke added 23 commits 2023-11-14 19:11:59 +01:00
Jacques Lucke added 3 commits 2023-11-14 19:19:12 +01:00
Jacques Lucke added 4 commits 2023-11-14 20:26:32 +01:00
Jacques Lucke added 1 commit 2023-11-14 20:56:45 +01:00
Jacques Lucke added 4 commits 2023-11-14 22:54:04 +01: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/PR112677) when ready.
Jacques Lucke added 3 commits 2023-11-15 16:00:53 +01:00
Jacques Lucke added 3 commits 2023-11-15 16:23:56 +01: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/PR112677) when ready.
Jacques Lucke added 4 commits 2023-11-16 17:36:03 +01:00
Jacques Lucke added 1 commit 2023-11-16 17:53:30 +01:00
fix vector node layout
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
e8a8298f6d
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/PR112677) when ready.
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/PR112677) when ready.
Jacques Lucke added 3 commits 2023-11-18 12:52:01 +01:00
Jacques Lucke added 1 commit 2023-11-20 14:48:44 +01:00
Merge branch 'main' into geometry-nodes-gizmos
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
29540a2f8d
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/PR112677) when ready.
Jacques Lucke added 7 commits 2023-11-21 13:53:56 +01:00
Jacques Lucke added 2 commits 2023-11-21 14:14:14 +01:00
Jacques Lucke added 22 commits 2023-11-21 22:01:26 +01:00
Jacques Lucke added 4 commits 2023-11-21 22:22:21 +01:00
Jacques Lucke requested review from Hans Goudey 2023-11-21 22:24:34 +01:00
Hans Goudey requested changes 2023-11-22 03:01:34 +01:00
Hans Goudey left a comment
Member
  • Somehow by setting the direction to zero and then some other value, I can create many gizmos from a single gizmo node. Even deleting the node or setting another active object doesn't remove them! They still move and control the value, but they don't reset to their previous position after interaction.
    image
  • Scaling the object makes gizmos smaller and larger. Feels weird when everything else is sized in screen space
    image

Remaining design topics IMO:

  • Node naming
  • Scaling of gizmos
    • From discussion in chat: a screen space option with a factor to scale from the default, and a "object space" option.
  • Node tools
    • I think it would be good to have an idea of how these would be used for "active node tools" before we commit it, just to be safe. I think once you have an active tool selected that corresponds to a node tool, the gizmo nodes would be evaluated and displayed, and you could use them to start the operation. But I think you'd have a better idea of the feasibility of something like that.

Overall I found the code pretty nice. My one larger concern is the growing extent to which we're adding specific code in places that are just general for the entire node system. I didn't get to the gizmo propagation code yet.

- [x] Somehow by setting the direction to zero and then some other value, I can create many gizmos from a single gizmo node. Even deleting the node or setting another active object doesn't remove them! They still move and control the value, but they don't reset to their previous position after interaction. ![image](/attachments/23bfa01b-1a69-44f5-ae34-66b66c3a965b) - [x] Scaling the object makes gizmos smaller and larger. Feels weird when everything else is sized in screen space ![image](/attachments/90d0ea63-0c12-493c-ada6-6c2c822ca06b) Remaining design topics IMO: - [x] Node naming - [x] Scaling of gizmos - From discussion in chat: a screen space option with a factor to scale from the default, and a "object space" option. - [x] Node tools - I think it would be good to have an idea of how these would be used for "active node tools" before we commit it, just to be safe. I think once you have an active tool selected that corresponds to a node tool, the gizmo nodes would be evaluated and displayed, and you could use them to start the operation. But I think you'd have a better idea of the feasibility of something like that. Overall I found the code pretty nice. My one larger concern is the growing extent to which we're adding specific code in places that are just general for the entire node system. I didn't get to the gizmo propagation code yet.
@ -680,6 +691,7 @@ class NODE_MT_geometry_node_add_all(Menu):
layout.menu("NODE_MT_category_GEO_TEXTURE")
layout.menu("NODE_MT_category_GEO_UTILITIES")
layout.separator()
layout.menu("NODE_MT_category_GEO_GIZMO")
Member

Might as well hide this for tools

Might as well hide this for tools
JacquesLucke marked this conversation as resolved
@ -14,3 +16,4 @@
#include "BLI_bounds_types.hh"
#include "BLI_function_ref.hh"
#include "BLI_map.hh"
#include "BLI_math_matrix_types.hh"
Member

BKE_geometry_set.hh is a fairly common header, it would be nice to avoid the need to include these two headers here. Putting them in a separate class / header like CurvesEditHints and GreasePencilEditHints would make sense I think.

`BKE_geometry_set.hh` is a fairly common header, it would be nice to avoid the need to include these two headers here. Putting them in a separate class / header like `CurvesEditHints` and `GreasePencilEditHints` would make sense I think.
JacquesLucke marked this conversation as resolved
@ -133,6 +136,7 @@ class bNodeTreeRuntime : NonCopyable, NonMovable {
/** Information about usage of anonymous attributes within the group. */
std::unique_ptr<anonymous_attribute_inferencing::AnonymousAttributeInferencingResult>
anonymous_attribute_inferencing;
std::unique_ptr<nodes::gizmos::GizmoPropagationResult> gizmo_inferencing;
Member

I don't really expect anything to change in this PR necessarily, but I find it really ugly how we keep adding these geometry nodes specific things into the base node tree runtime class. I think it adds confusion about the responsibility of different code. It would be better to generalize this a bit. But maybe that depends on our end goal for what node tree types are supposed to mean. Just deserves some active thought IMO.

I don't really expect anything to change in this PR necessarily, but I find it really ugly how we keep adding these geometry nodes specific things into the base node tree runtime class. I think it adds confusion about the responsibility of different code. It would be better to generalize this a bit. But maybe that depends on our end goal for what node tree types are supposed to mean. Just deserves some active thought IMO.
Author
Member

Hm, I don't really have a problem with it in this specific case. I haven't found a case where it actually hurts us (at least in run-time data). We could move the geometry nodes specific stuff to a separate struct, but it's not clear to me whether that makes the code better or just more verbose.

Hm, I don't really have a problem with it in this specific case. I haven't found a case where it actually hurts us (at least in run-time data). We could move the geometry nodes specific stuff to a separate struct, but it's not clear to me whether that makes the code better or just more verbose.
JacquesLucke marked this conversation as resolved
@ -199,0 +204,4 @@
* This is set in #update_gizmo_propagation and is stored here so that it can be quickly accessed
* during socket drawing.
*/
bool has_gizmo = false;
Member

Storing this specific data in general structs is a serious anti-pattern IMO. 99% of the time this isn't needed, and it isn't related to anything else here.

Maybe it would work to store something in GizmoPropagationResult instead. Maybe a BitVector or a Set<int>

Storing this specific data in general structs is a serious anti-pattern IMO. 99% of the time this isn't needed, and it isn't related to anything else here. Maybe it would work to store something in `GizmoPropagationResult` instead. Maybe a `BitVector` or a `Set<int>`
Author
Member

Last time we tried something like that with location, it didn't work out. Not sure if that situation has really changed. If it did, I'm fine with moving this data to a separate array, but I'm not willing to risk that as part of this patch. We can try that separately again.

Last time we tried something like that with `location`, it didn't work out. Not sure if that situation has really changed. If it did, I'm fine with moving this data to a separate array, but I'm not willing to risk that as part of this patch. We can try that separately again.
JacquesLucke marked this conversation as resolved
@ -97,1 +97,3 @@
uiItemR(layout, &sockptr, "default_value", DEFAULT_FLAGS, "", ICON_NONE);
uiLayout *row = uiLayoutRow(layout, true);
uiItemR(row, &sockptr, "default_value", DEFAULT_FLAGS, "", ICON_NONE);
if (output->runtime->has_gizmo) {
Member

I guess this is covered by my other comments, but I find myself skeptical of the gizmo checks creeping in this far, running even in shader nodes, for example. It just seems like a hack. Some alternatives I could think of:

  • Use input sockets for these "input nodes" so this can be done automatically
  • Use a different "Value" input node for geometry nodes so this doesn't end up in every node system
  • Generalize a "extra button" callback that can be per node tree type

For generic socket drawing it's probably worth generalizing this more as well

I guess this is covered by my other comments, but I find myself skeptical of the gizmo checks creeping in this far, running even in shader nodes, for example. It just seems like a hack. Some alternatives I could think of: - Use input sockets for these "input nodes" so this can be done automatically - Use a different "Value" input node for geometry nodes so this doesn't end up in every node system - Generalize a "extra button" callback that can be per node tree type For generic socket drawing it's probably worth generalizing this more as well
Author
Member

I think on a conceptual level, an "extra button" callback for nodes and sockets would make sense here. However, it seems like this would not actually give us the control we want. For a better looking ui it's probably better to define how it's drawn in one place instead of two, especially because the ui elements belong closely together.

I see this somewhat similar to property decoration for creating keyframes.

With this additional callback, it wouldn't be so clear whether you should pass in layout or row, both are constraining in different ways.

I think on a conceptual level, an "extra button" callback for nodes and sockets would make sense here. However, it seems like this would not actually give us the control we want. For a better looking ui it's probably better to define how it's drawn in one place instead of two, especially because the ui elements belong closely together. I see this somewhat similar to property decoration for creating keyframes. With this additional callback, it wouldn't be so clear whether you should pass in `layout` or `row`, both are constraining in different ways.
JacquesLucke marked this conversation as resolved
@ -0,0 +76,4 @@
if (prop_type == PROP_FLOAT) {
return RNA_property_float_get_index(&this->owner, this->property, *this->index);
}
else if (prop_type == PROP_INT) {
Member

Else after return

Else after return
JacquesLucke marked this conversation as resolved
@ -0,0 +137,4 @@
RNA_property_update(C, &this->owner, this->property);
if (Object *object = CTX_data_active_object(C)) {
/* The recalc is necessary so that the gizmo positions are updated even if they don't affect
* the final output currently. */
Member

Do you have an idea for how this could be fixed? Maybe just tagging the region for redraw could work?

Do you have an idea for how this could be fixed? Maybe just tagging the region for redraw could work?
Author
Member

Removed it for now. Couldn't reproduce the purpose anymore, but not sure if I really fixed it already or just don't know how to reproduce the issue anymore. If we'll need it again, it can also use DEG_id_tag_update_for_side_effect_request.

Removed it for now. Couldn't reproduce the purpose anymore, but not sure if I really fixed it already or just don't know how to reproduce the issue anymore. If we'll need it again, it can also use `DEG_id_tag_update_for_side_effect_request`.
JacquesLucke marked this conversation as resolved
@ -0,0 +158,4 @@
const bNodeSocket &second_input_socket = node.input_socket(1);
const float second_value =
tree_log.find_primitive_socket_value<float>(second_input_socket).value_or(0.0f);
switch (mode) {
Member

Looks so nice without the brackets :D

      switch (mode) {
        case NODE_MATH_MULTIPLY:
          return safe_divide(target_factor, second_value);
        case NODE_MATH_DIVIDE:
          return target_factor * second_value;
        case NODE_MATH_RADIANS:
          return RAD2DEG(target_factor);
        case NODE_MATH_DEGREES:
          return DEG2RAD(target_factor);
        default:
          return std::nullopt;
      }
Looks so nice without the brackets :D ``` switch (mode) { case NODE_MATH_MULTIPLY: return safe_divide(target_factor, second_value); case NODE_MATH_DIVIDE: return target_factor * second_value; case NODE_MATH_RADIANS: return RAD2DEG(target_factor); case NODE_MATH_DEGREES: return DEG2RAD(target_factor); default: return std::nullopt; } ```
JacquesLucke marked this conversation as resolved
@ -253,1 +253,4 @@
}
if (geometry.get_component<GeometryComponentEditData>()) {
auto &component = geometry.get_component_for_write<GeometryComponentEditData>();
for (float4x4 &m : component.gizmo_transforms_.values()) {
Member

Just curious, do you imagine that other nodes would end up being able to deform the gizmo transforms?

Just curious, do you imagine that other nodes would end up being able to deform the gizmo transforms?
Author
Member

Note sure about the existing nodes, but I could imagine that we could add nodes that allow deforming the transforms. It's quite similar to what we talked about for crazy space for curves.

Note sure about the existing nodes, but I could imagine that we could add nodes that allow deforming the transforms. It's quite similar to what we talked about for crazy space for curves.
JacquesLucke marked this conversation as resolved
Jacques Lucke added 2 commits 2023-11-22 15:47:47 +01:00
Jacques Lucke added 1 commit 2023-11-24 10:18:34 +01:00
Jacques Lucke added 2 commits 2023-11-26 14:16:32 +01: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/PR112677) when ready.
Jacques Lucke added 2 commits 2023-11-28 10:28:42 +01:00
Member

I do after multiple considerations feel like there should be some (subtle) difference in drawing for the links that propagate the gizmo control backwards in addition to the icons on the sockets that do so.

When working on an example setup I found multiple instances where it gets confusing quite quickly how the gizmo control is flowing in the graph. The icons do can in theory communicate the complete information, but practically they are not sufficient since, when working on a node-tree, you don't always have all nodes in your view at all times, nor should you have to, to understand the information presented.

Case where the icons right now are not sufficient:
image
Is X and Y or either one used for the gizmo control?

Case where it's ambiguous which connection controls the group input:
image
Is one connection a gizmo or both? Which one is it?

And this might look like a stupid example, but having to scan all the connected nodes between on or potentially multiple reroutes that don't even have an icon themselves is a lot of overhead:
image

And again, I realize that only showing a part of the node-tree is not looking at all the information it provides, but understanding the dataflow should ideally not rely on seeing all the context. That's a big readability issue imo, that we are already fighting with the evaluation of fields, where you need to scan a lot of the node-tree to understand what is happening, rather than having all that context locally visible.
In a practical sense, looking at all the connected nodes first, which might be quite far away is quite inconvenient when you need this information to understand the context of the gizmo icon in the node.

One question that might alleviate the issue of presenting potentially irrelevant information and making the node-tree more noisy that it might need to be:
Would it be possible to only draw the links differently whenever the gizmos are actually drawn? That way, in most of the cases where the gizmo back propagation is something you don't care about the node-tree wouldn't look any different then now. The icons would still be there. But then it would be more like showing the flow when it matters. Just an idea.

I do after multiple considerations feel like there should be some (subtle) difference in drawing for the links that propagate the gizmo control backwards in addition to the icons on the sockets that do so. When working on an example setup I found multiple instances where it gets confusing quite quickly how the gizmo control is flowing in the graph. The icons ~~do~~ can in theory communicate the complete information, but practically they are not sufficient since, when working on a node-tree, you don't always have all nodes in your view at all times, nor should you have to, to understand the information presented. Case where the icons right now are not sufficient: ![image](/attachments/f9258605-d044-4f61-9ea5-e0665d75541b) Is X and Y or either one used for the gizmo control? Case where it's ambiguous which connection controls the group input: ![image](/attachments/5d29c272-d92d-49e3-a88e-25d831fa5926) Is one connection a gizmo or both? Which one is it? And this might look like a stupid example, but having to scan all the connected nodes between on or potentially multiple reroutes that don't even have an icon themselves is a lot of overhead: ![image](/attachments/d0b71fe9-afb6-4d22-a79f-f751d7ddd412) And again, I realize that only showing a part of the node-tree is not looking at all the information it provides, but understanding the dataflow should ideally not rely on seeing all the context. That's a big readability issue imo, that we are already fighting with the evaluation of fields, where you need to scan a lot of the node-tree to understand what is happening, rather than having all that context locally visible. In a practical sense, looking at all the connected nodes first, which might be quite far away is quite inconvenient when you need this information to understand the context of the gizmo icon in the node. One question that might alleviate the issue of presenting potentially irrelevant information and making the node-tree more noisy that it might need to be: Would it be possible to only draw the links differently whenever the gizmos are actually drawn? That way, in most of the cases where the gizmo back propagation is something you don't care about the node-tree wouldn't look any different then now. The icons would still be there. But then it would be more like showing the flow when it matters. Just an idea.
Author
Member

I wonder, how would knowing which links in your examples propagate a gizmo affect what you do? Knowing where a link goes helps of course, but that doesn't seem unique to gizmos at a first glance.

Would it be possible to only draw the links differently whenever the gizmos are actually drawn? That way, in most of the cases where the gizmo back propagation is something you don't care about the node-tree wouldn't look any different then now.

Seems possible, but I wonder if that is good enough. It doesn't seem to help in the example you showed if the gizmo isn't visible already.

I wonder, how would knowing which links in your examples propagate a gizmo affect what you do? Knowing where a link goes helps of course, but that doesn't seem unique to gizmos at a first glance. > Would it be possible to only draw the links differently whenever the gizmos are actually drawn? That way, in most of the cases where the gizmo back propagation is something you don't care about the node-tree wouldn't look any different then now. Seems possible, but I wonder if that is good enough. It doesn't seem to help in the example you showed if the gizmo isn't visible already.
Member

I wonder, how would knowing which links in your examples propagate a gizmo affect what you do? Knowing where a link goes helps of course, but that doesn't seem unique to gizmos at a first glance.

I mean, the fact that you have to understand how the control of a gizmo is propagated back is not in question, is it? It's pretty important when setting up how the node-tree is being controlled to understand these connections. It's pretty important when connecting the a node link to see that this also means it will be controlled by a gizmo, potentially a second or third gizmo if you are connecting the value in multiple places. It's not so much changing my actions in how I connect the nodes, but more so giving me clear insight into the implications of my actions.
As an example, if I have a node in my node-tree that uses gizmos for the inputs and I connect something to one of these inputs, the gizmo is still usable to control the value that this link propagates the information to. This should be clearly visible from the node that shows this connection imo.
image
Scanning every connected node with your eyes to find the string of connected nodes with the icon is not a valid replacement for making this clearly readable imo.

Would it be possible to only draw the links differently whenever the gizmos are actually drawn? That way, in most of the cases where the gizmo back propagation is something you don't care about the node-tree wouldn't look any different then now.

Seems possible, but I wonder if that is good enough. It doesn't seem to help in the example you showed if the gizmo isn't visible already.

Exactly, my thought process was that in the cases that the gizmo isn't visible, you usually do not care about the propagation. Changing the drawing of the connected links that propagate the gizmo information when you select a node and makes its gizmos visible would also highlight that the gizmos that are now showing up will propagate changes in your node-tree.

> I wonder, how would knowing which links in your examples propagate a gizmo affect what you do? Knowing where a link goes helps of course, but that doesn't seem unique to gizmos at a first glance. I mean, the fact that you have to understand how the control of a gizmo is propagated back is not in question, is it? It's pretty important when setting up how the node-tree is being controlled to understand these connections. It's pretty important when connecting the a node link to see that this also means it will be controlled by a gizmo, potentially a second or third gizmo if you are connecting the value in multiple places. It's not so much changing my actions in how I connect the nodes, but more so giving me clear insight into the implications of my actions. As an example, if I have a node in my node-tree that uses gizmos for the inputs and I connect something to one of these inputs, the gizmo is still usable to control the value that this link propagates the information to. This should be clearly visible from the node that shows this connection imo. ![image](/attachments/2f70835a-e02c-46fc-bff1-26db050a2adf) Scanning every connected node with your eyes to find the string of connected nodes with the icon is not a valid replacement for making this clearly readable imo. > > > Would it be possible to only draw the links differently whenever the gizmos are actually drawn? That way, in most of the cases where the gizmo back propagation is something you don't care about the node-tree wouldn't look any different then now. > > Seems possible, but I wonder if that is good enough. It doesn't seem to help in the example you showed if the gizmo isn't visible already. Exactly, my thought process was that in the cases that the gizmo isn't visible, you usually do not care about the propagation. Changing the drawing of the connected links that propagate the gizmo information when you select a node and makes its gizmos visible would also highlight that the gizmos that are now showing up will propagate changes in your node-tree.
Jacques Lucke added 6 commits 2023-11-29 15:10:54 +01:00
Author
Member

As an example, if I have a node in my node-tree that uses gizmos for the inputs and I connect something to one of these inputs, the gizmo is still usable to control the value that this link propagates the information to. This should be clearly visible from the node that shows this connection imo.

That part could also be indicated by graying out the gizmo icon when the gizmo is "broken" because it's controlled in a way that does not support a gizmo I think.

image

Exactly, my thought process was that in the cases that the gizmo isn't visible, you usually do not care about the propagation. Changing the drawing of the connected links that propagate the gizmo information when you select a node and makes its gizmos visible would also highlight that the gizmos that are now showing up will propagate changes in your node-tree.

Hm ok, I can give this a try.

> As an example, if I have a node in my node-tree that uses gizmos for the inputs and I connect something to one of these inputs, the gizmo is still usable to control the value that this link propagates the information to. This should be clearly visible from the node that shows this connection imo. That part could also be indicated by graying out the gizmo icon when the gizmo is "broken" because it's controlled in a way that does not support a gizmo I think. ![image](/attachments/8a97a012-8763-4842-afc0-e5b0f2f2f7be) > Exactly, my thought process was that in the cases that the gizmo isn't visible, you usually do not care about the propagation. Changing the drawing of the connected links that propagate the gizmo information when you select a node and makes its gizmos visible would also highlight that the gizmos that are now showing up will propagate changes in your node-tree. Hm ok, I can give this a try.
Member

As an example, if I have a node in my node-tree that uses gizmos for the inputs and I connect something to one of these inputs, the gizmo is still usable to control the value that this link propagates the information to. This should be clearly visible from the node that shows this connection imo.

That part could also be indicated by graying out the gizmo icon when the gizmo is "broken" because it's controlled in a way that does not support a gizmo I think.

That's a good idea, but it's not actually what I meant. I just meant the totally valid case that I've shown in the screenshot. I think it's a lot more clear that the gizmos will have an effect on other values in your node-tree that the active node is connected to when the links indicate the flow of that control.
Another example would be wanting to manually control the value precisely, that you just set by eye in the viewport. Finding the actual value that has been modified is a lot easier if you can visually track the connections.

> > As an example, if I have a node in my node-tree that uses gizmos for the inputs and I connect something to one of these inputs, the gizmo is still usable to control the value that this link propagates the information to. This should be clearly visible from the node that shows this connection imo. > > That part could also be indicated by graying out the gizmo icon when the gizmo is "broken" because it's controlled in a way that does not support a gizmo I think. That's a good idea, but it's not actually what I meant. I just meant the totally valid case that I've shown in the screenshot. I think it's a lot more clear that the gizmos will have an effect on other values in your node-tree that the active node is connected to when the links indicate the flow of that control. Another example would be wanting to manually control the value precisely, that you just set by eye in the viewport. Finding the actual value that has been modified is a lot easier if you can visually track the connections.
Jacques Lucke added 1 commit 2023-12-16 14:06:56 +01:00
Jacques Lucke added 2 commits 2023-12-19 16:12:58 +01:00
Jacques Lucke added 1 commit 2023-12-19 21:40:05 +01:00
Jacques Lucke added 7 commits 2023-12-19 23:28:11 +01:00
Jacques Lucke added 1 commit 2023-12-22 22:45:34 +01:00
Jacques Lucke added 2 commits 2023-12-22 23:10:15 +01:00
Jacques Lucke changed title from WIP: Geometry Nodes: add support for gizmos to WIP: Geometry Nodes: attaching attaching gizmos for input values 2023-12-22 23:18:46 +01:00
Jacques Lucke changed title from WIP: Geometry Nodes: attaching attaching gizmos for input values to WIP: Geometry Nodes: support attaching gizmos for input values 2023-12-22 23:21:35 +01:00
Jacques Lucke changed title from WIP: Geometry Nodes: support attaching gizmos for input values to WIP: Geometry Nodes: support attaching gizmos to input values 2023-12-22 23:21:46 +01:00
Jacques Lucke added 3 commits 2023-12-23 00:13:46 +01:00
Jacques Lucke added 1 commit 2023-12-23 00:27:38 +01:00
Author
Member

I think it would be good to have an idea of how these would be used for "active node tools" before we commit it, just to be safe. I think once you have an active tool selected that corresponds to a node tool, the gizmo nodes would be evaluated and displayed, and you could use them to start the operation. But I think you'd have a better idea of the feasibility of something like that.

That seems feasible. Will require a bit of refactoring, but probably not too much. Evaluating just the gizmo nodes that control operators inputs, without computing everything else is possible. Not sure if it will be required to disable that behavior in some cases since it has the potential to be slow.

> I think it would be good to have an idea of how these would be used for "active node tools" before we commit it, just to be safe. I think once you have an active tool selected that corresponds to a node tool, the gizmo nodes would be evaluated and displayed, and you could use them to start the operation. But I think you'd have a better idea of the feasibility of something like that. That seems feasible. Will require a bit of refactoring, but probably not too much. Evaluating just the gizmo nodes that control operators inputs, without computing everything else is possible. Not sure if it will be required to disable that behavior in some cases since it has the potential to be slow.
Jacques Lucke added 3 commits 2023-12-23 01:09:12 +01:00
Could be something I fixed already, but maybe the problem will also show up again later.
I noticed that this could use `DEG_id_tag_update_for_side_effect_request` nowadays if required.
move gizmo edit hints to a separate header file
Some checks failed
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
1e7ddf59ec
Author
Member

@blender-bot build

@blender-bot build
Jacques Lucke changed title from WIP: Geometry Nodes: support attaching gizmos to input values to Geometry Nodes: support attaching gizmos to input values 2023-12-23 01:24:15 +01:00
Jacques Lucke requested review from Hans Goudey 2023-12-23 01:24:16 +01: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/PR112677) when ready.
Jacques Lucke added 3 commits 2023-12-23 12:41:14 +01:00
more explicitly handle muted nodes
Some checks failed
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
c1af9be9bb
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/PR112677) when ready.
Jacques Lucke added 1 commit 2023-12-23 12:58:55 +01:00
Jacques Lucke added 2 commits 2024-01-12 13:54:57 +01:00
Jacques Lucke added 2 commits 2024-01-24 10:49:43 +01:00
don't tag gizmos for update all the time
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
b6afe3e13b
Author
Member

@blender-bot build

@blender-bot build
Jacques Lucke added 2 commits 2024-04-10 15:44:42 +02:00
fixes after merge
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
588be215da
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/PR112677) when ready.
Jacques Lucke added 2 commits 2024-05-05 20:43:34 +02:00
Jacques Lucke added 2 commits 2024-06-07 16:20:49 +02:00
Jacques Lucke added 67 commits 2024-06-13 22:46:44 +02:00
Jacques Lucke added 6 commits 2024-06-16 15:11:21 +02:00
Jacques Lucke added 3 commits 2024-06-16 16:08:38 +02:00
support object space dial gizmo
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
8e7f3b80c4
Member

Only blender organization members with write access can start builds. See documentation for details.

Only blender organization members with write access can start builds. See [documentation](https://projects.blender.org/infrastructure/blender-bot/src/branch/main/README.md) for details.
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/PR112677) when ready.
Jacques Lucke added 4 commits 2024-06-17 19:15:30 +02:00
make format
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
19cd6b5a10
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/PR112677) when ready.
Hans Goudey reviewed 2024-06-17 23:47:36 +02:00
Hans Goudey left a comment
Member

I still have to read the inverse evaluation code properly. Just checking in on this branch again. It still works nicely from a UI perspective (though I won't comment on the link drawing :P)

I still have to read the inverse evaluation code properly. Just checking in on this branch again. It still works nicely from a UI perspective (though I won't comment on the link drawing :P)
@ -354,6 +361,9 @@ struct bNodeType {
/** Get extra information that is drawn next to the node. */
NodeExtraInfoFunction get_extra_info;
NodeInverseElemEvalFunction eval_inverse_elem;
Member

Would be nice to have some info about the responsibility of these callbacks here. Or a reference to some comments elsewhere.

Would be nice to have some info about the responsibility of these callbacks here. Or a reference to some comments elsewhere.
@ -208,6 +212,8 @@ class bNodeSocketRuntime : NonCopyable, NonMovable {
*/
short total_inputs = 0;
bool has_gizmo = false;
Member

I know I mentioned this before, but I still think putting geometry nodes specific things in the general bNode and bNodeSocket structs is problematic. Nowadays we don't reorder nodes based on selection, so can this move to a large boolean array or some set maybe? Same with the other.

I know I mentioned this before, but I still think putting geometry nodes specific things in the general `bNode` and `bNodeSocket` structs is problematic. Nowadays we don't reorder nodes based on selection, so can this move to a large boolean array or some set maybe? Same with the other.
@ -0,0 +23,4 @@
b.add_input<decl::Bool>("Screen Space")
.default_value(true)
.description(
"If true, true gizmo is displayed in screen space. Otherwise it's in object space");
Member

Property descriptions for booleans should describe what happens when the value is true. So anything like "If true", "When enabled", or "when checked" should be avoided.

Property descriptions for booleans should describe what happens when the value is true. So anything like "If true", "When enabled", or "when checked" should be avoided.
@ -467,6 +467,9 @@ static void gizmo_tweak_finish(bContext *C, wmOperator *op, const bool cancel, b
wm_gizmomap_modal_set(mtweak->gzmap, C, mtweak->gz_modal, nullptr, false);
}
}
if (mtweak->gz_modal->flag & WM_GIZMO_NEEDS_UNDO) {
Member

Just a thought, do you think an alternative here is putting the gizmo "apply" code into an operator, more like existing gizmos like transform? Then the operator could do its own undo push. My guess is that would be a more "standard" use of Blender's architecture.

Just a thought, do you think an alternative here is putting the gizmo "apply" code into an operator, more like existing gizmos like transform? Then the operator could do its own undo push. My guess is that would be a more "standard" use of Blender's architecture.
Jacques Lucke added 1 commit 2024-06-18 13:30:03 +02:00
Jacques Lucke added 9 commits 2024-06-18 16:47:22 +02:00
Jacques Lucke added 1 commit 2024-06-18 18:01:05 +02:00
Jacques Lucke added 2 commits 2024-06-19 11:01:55 +02:00
Jacques Lucke added 4 commits 2024-06-20 10:17:49 +02:00
Jacques Lucke added 2 commits 2024-06-20 10:35:19 +02:00
Jacques Lucke added 7 commits 2024-06-20 19:38:19 +02:00
Jacques Lucke added 1 commit 2024-06-20 19:56:54 +02:00
Jacques Lucke added 2 commits 2024-06-24 12:58:10 +02:00
Jacques Lucke added 2 commits 2024-06-27 11:06:01 +02:00
Member

Just briefly checked the latest state again:

  • I think the undo step is not working properly (?) When I make a change in the nodes and use gizmos multiple times it undoes all those things in one step
  • When a part of the node-tree is not evaluated/ not connected to the output, its gizmos can still be drawn and interacted with. But when using the transform output of the gizmos, that means that the transformation is not done according to the rest of the nodes. This is mainly a problem with the viewer node, especially combined with gizmo pinning. I think it might just be best to not draw these gizmos in the first place, since the geometry they are related to is also not drawn. I'm not actually sure this can be done reliably, but I guess it could just be done specifically when the transform output is used, since then the evaluation is clear. What do you think?
  • The primary + secondary link drawing actually works reasonably well for me visually. The thickness of the outline and the primary link should still be matched with the regular ones. I didn't do that properly in the mockup.
  • The aliasing is indeed quite noticeable when zooming out, unfortunately
  • There is an issue with terminating the back-propagation of the transform gizmo for individual components of it. This might be a restriction of how this works, but just thought I'd point it out, since this works when all components are invalid.
    image
Just briefly checked the latest state again: - I think the undo step is not working properly (?) When I make a change in the nodes and use gizmos multiple times it undoes all those things in one step - When a part of the node-tree is not evaluated/ not connected to the output, its gizmos can still be drawn and interacted with. But when using the transform output of the gizmos, that means that the transformation is not done according to the rest of the nodes. This is mainly a problem with the viewer node, especially combined with gizmo pinning. I think it might just be best to not draw these gizmos in the first place, since the geometry they are related to is also not drawn. I'm not actually sure this can be done reliably, but I guess it could just be done specifically when the transform output is used, since then the evaluation is clear. What do you think? - The primary + secondary link drawing actually works reasonably well for me visually. The thickness of the outline and the primary link should still be matched with the regular ones. I didn't do that properly in the mockup. - The aliasing is indeed quite noticeable when zooming out, unfortunately - There is an issue with terminating the back-propagation of the transform gizmo for individual components of it. This might be a restriction of how this works, but just thought I'd point it out, since this works when all components are invalid. ![image](/attachments/4c5f0719-304c-4417-9816-e958e790d194)
150 KiB
Jacques Lucke added 6 commits 2024-06-27 19:22:45 +02:00
Jacques Lucke added 12 commits 2024-06-27 22:00:32 +02:00
Jacques Lucke added 2 commits 2024-06-27 22:30:37 +02:00
Jacques Lucke added 3 commits 2024-06-28 11:45:27 +02:00
Jacques Lucke added 1 commit 2024-06-28 11:59:49 +02:00
Jacques Lucke added 1 commit 2024-06-28 12:01:16 +02:00
Jacques Lucke added 1 commit 2024-06-28 12:30:20 +02:00
Jacques Lucke added 1 commit 2024-06-28 12:52:32 +02:00
Jacques Lucke added 3 commits 2024-06-28 13:35:10 +02:00
Jacques Lucke added 4 commits 2024-06-28 16:49:17 +02:00
fix socket sidebar drawing with pin icon
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
7f6ae73651
Author
Member

I think I your feedback. Some new things:

  • The transform gizmo node can now automatically disable e.g. all translation gizmos if the translation is not editable.
  • The new link drawing is now only done for active gizmos.
  • Removed support for propagating through drivers for now. Can be added back later if necessary.
  • If the transform output is used, only show the gizmos if the transforms have been propagated to the visible geometry.
I think I your feedback. Some new things: * The transform gizmo node can now automatically disable e.g. all translation gizmos if the translation is not editable. * The new link drawing is now only done for active gizmos. * Removed support for propagating through drivers for now. Can be added back later if necessary. * If the transform output is used, only show the gizmos if the transforms have been propagated to the visible geometry.
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/PR112677) when ready.
Member

The drawing looks much nicer now!

Thanks for implementing the context dependant link drawing depending on the gizmo visibility. I'm quite unsure about that. I think I'll have to do some more practical testing to figure out what I would prefer. I do think it can work, but there might also be some issues left with controlling better what gizmos are actually shown. (E.g. a way to pin all gizmos of a node-group in one click)

image
I also feel like this case would warrant showing the gizmo for the Offset input, when the node-group is active, even though it's being propagated to the vector input node. Right now it only shows when selecting the vector input node.
But I'm not sure about that, at least it would make sense to me to display the gizmo link connected to that node imo.
(Actually, I found a bug in the case of the screenshot: while the link drawing works reliably when selecting the vector node, the actual gizmo itself only works after re-evaluation of the node-tree, it seems. Specifically when making the node-group active first and then the vector node.)

But I think this is part of what I need to experience a bit more in practice and it would be good to get some more opinions on what to show when as well.

Removed support for propagating through drivers for now. Can be added back later if necessary.

Do you want to create a new task for that?

The drawing looks much nicer now! Thanks for implementing the context dependant link drawing depending on the gizmo visibility. I'm quite unsure about that. I think I'll have to do some more practical testing to figure out what I would prefer. I do think it can work, but there might also be some issues left with controlling better what gizmos are actually shown. (E.g. a way to pin all gizmos of a node-group in one click) ![image](/attachments/35862bb6-1e7e-448a-b7e5-bd55215f850c) I also feel like this case would warrant showing the gizmo for the `Offset` input, when the node-group is active, even though it's being propagated to the vector input node. Right now it only shows when selecting the vector input node. But I'm not sure about that, at least it would make sense to me to display the gizmo link connected to that node imo. (Actually, I found a bug in the case of the screenshot: while the link drawing works reliably when selecting the vector node, the actual gizmo itself only works after re-evaluation of the node-tree, it seems. Specifically when making the node-group active first and then the vector node.) But I think this is part of what I need to experience a bit more in practice and it would be good to get some more opinions on what to show when as well. > Removed support for propagating through drivers for now. Can be added back later if necessary. Do you want to create a new task for that?
Author
Member

Thanks for implementing the context dependant link drawing depending on the gizmo visibility. I'm quite unsure about that.

I quite like it currently. I'm just not entirely sure if we can strictly couple it to the visibility in the viewport(s). E.g. sometimes gizmos are not shown there because they are disabled in the overlay settings, or because their transform is not propagated to the final geometry among other reasons. Taking that into account in node editor drawing is probably possible, but not entirely straight forward.

Actually, I found a bug in the case of the screenshot: while the link drawing works reliably when selecting the vector node, the actual gizmo itself only works after re-evaluation of the node-tree, it seems. Specifically when making the node-group active first and then the vector node.

Please provide a .blend file that has the bug. I can't reproduce it right now.

Do you want to create a new task for that?

I'll add it as a follow up task to the PR description. We can create a task for it a little later when we are ready here.

> Thanks for implementing the context dependant link drawing depending on the gizmo visibility. I'm quite unsure about that. I quite like it currently. I'm just not entirely sure if we can strictly couple it to the visibility in the viewport(s). E.g. sometimes gizmos are not shown there because they are disabled in the overlay settings, or because their transform is not propagated to the final geometry among other reasons. Taking that into account in node editor drawing is probably possible, but not entirely straight forward. > Actually, I found a bug in the case of the screenshot: while the link drawing works reliably when selecting the vector node, the actual gizmo itself only works after re-evaluation of the node-tree, it seems. Specifically when making the node-group active first and then the vector node. Please provide a .blend file that has the bug. I can't reproduce it right now. > Do you want to create a new task for that? I'll add it as a follow up task to the PR description. We can create a task for it a little later when we are ready here.
Jacques Lucke added 2 commits 2024-07-01 08:56:04 +02:00
Member

Please provide a .blend file that has the bug. I can't reproduce it right now.

procedural_modeling-gizmo_test.blend

  • click on vector node
  • click on node-group
  • click on vector node again
> Please provide a .blend file that has the bug. I can't reproduce it right now. [procedural_modeling-gizmo_test.blend](/attachments/89186548-7765-4404-92a4-49b310916276) - click on vector node - click on node-group - click on vector node again
Jacques Lucke added 1 commit 2024-07-01 13:47:21 +02:00
Jacques Lucke added 1 commit 2024-07-01 14:29:00 +02:00
Jacques Lucke added 3 commits 2024-07-01 18:15:06 +02:00
Jacques Lucke added 5 commits 2024-07-02 11:48:20 +02:00
Jacques Lucke added 4 commits 2024-07-02 16:16:30 +02:00
Jacques Lucke added 2 commits 2024-07-02 19:51:56 +02:00
Jacques Lucke added 1 commit 2024-07-02 21:04:31 +02:00
Jacques Lucke added 1 commit 2024-07-02 21:47:29 +02:00
Jacques Lucke added 10 commits 2024-07-03 11:26:57 +02:00
cleanup
Some checks reported errors
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
79c086462c
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/PR112677) when ready.
Jacques Lucke added 1 commit 2024-07-03 11:31:35 +02:00
Jacques Lucke added 1 commit 2024-07-03 11:34:14 +02:00
fix
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
cb81a04d8f
Jacques Lucke requested review from Simon Thommes 2024-07-03 11:49:00 +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/PR112677) when ready.
Jacques Lucke added 2 commits 2024-07-03 13:56:18 +02:00
Iliya Katushenock reviewed 2024-07-03 14:00:37 +02:00
Iliya Katushenock left a comment
Member

For the record, it seems this gizmo implementation is separate enough from geometry nodes evaluation and only relation is to know which nodes should be evaluated by following gizmos logic.
So, probably this will be easy to support gizmo in other backends like composition or texture nodes. By easy i mean that there is no need to rewrite 100% of the code just to evaluate values.
Next one point is that this is not just another one suga for funny video with using gizmo for control resolution of the circular staircase in the house. This is the step in direction to replace python (and not only) operators by node tools with now gizmo settings.

For the record, it seems this gizmo implementation is separate enough from geometry nodes evaluation and only relation is to know which nodes should be evaluated by following gizmos logic. So, probably this will be easy to support gizmo in other backends like composition or texture nodes. By _easy_ i mean that there is no need to rewrite 100% of the code just to evaluate values. Next one point is that this is not just another one suga for funny video with using gizmo for control resolution of the circular staircase in the house. This is the step in direction to replace python (and not only) operators by node tools with now gizmo settings.
Jacques Lucke added 1 commit 2024-07-03 16:10:41 +02:00
Hans Goudey approved these changes 2024-07-04 18:55:44 +02:00
Hans Goudey left a comment
Member

This looks quite good to me. I'm happy with the different levels of abstraction and how easy it was to understand the code. I didn't read deeply enough to fully understand all the details but I'm was satisfied that anyone could as necessary.

One thing that would have been helpful in my engineering visualization files where I was testing adding gizmos is a circle drawing mode for the linear gizmo:
image

It would also be very helpful to click on the gizmos to enter values directly. At that point I think I could give these files to people not familiar with Blender and they would be able to do compelling things. But that's a more general gizmo feature I guess.

I find the link drawing quite nice and I like the context dependent drawing as well, since it mirrors what you see in the 3D view. Overall, big +1, I think this is ready to land!

This looks quite good to me. I'm happy with the different levels of abstraction and how easy it was to understand the code. I didn't read deeply enough to fully understand all the details but I'm was satisfied that anyone could as necessary. One thing that would have been helpful in my engineering visualization files where I was testing adding gizmos is a circle drawing mode for the linear gizmo: ![image](/attachments/4af730e8-f40b-4178-b7e2-691926631b94) It would also be very helpful to click on the gizmos to enter values directly. At that point I think I could give these files to people not familiar with Blender and they would be able to do compelling things. But that's a more general gizmo feature I guess. I find the link drawing quite nice and I like the context dependent drawing as well, since it mirrors what you see in the 3D view. Overall, big +1, I think this is ready to land!
184 KiB
@ -0,0 +19,4 @@
* the path of group nodes to get from the geometry nodes modifier to the group containing the
* gizmo node).
*/
struct GeoNodesGizmoID {
Member

How about NodeGizmoID? Since there's nothing really specific to geometry nodes in this struct

How about `NodeGizmoID`? Since there's nothing really specific to geometry nodes in this struct
JacquesLucke marked this conversation as resolved
@ -1307,0 +1324,4 @@
FN_NODE_INPUT_INT,
FN_NODE_INPUT_BOOL,
FN_NODE_INPUT_ROTATION,
NODE_GROUP_INPUT))
Member

Could you extract this node type ELEM check to a separate function?

Could you extract this node type ELEM check to a separate function?
JacquesLucke marked this conversation as resolved
@ -4165,0 +4175,4 @@
snode.edittree->ensure_topology_cache();
/* Compute compute context hash for the current node tree path. */
ComputeContextHash current_compute_context_hash;
Member

What do you think about this?

  const ComputeContextHash current_compute_context_hash = [&]() {
    ComputeContextBuilder compute_context_builder;
    compute_context_builder.push<bke::ModifierComputeContext>(
        object_and_modifier->nmd->modifier.name);
    if (!ed::space_node::push_compute_context_for_tree_path(snode, compute_context_builder)) {
      return ComputeContextHash{};
    }
    return compute_context_builder.current()->hash();
  }();
What do you think about this? ```cpp const ComputeContextHash current_compute_context_hash = [&]() { ComputeContextBuilder compute_context_builder; compute_context_builder.push<bke::ModifierComputeContext>( object_and_modifier->nmd->modifier.name); if (!ed::space_node::push_compute_context_for_tree_path(snode, compute_context_builder)) { return ComputeContextHash{}; } return compute_context_builder.current()->hash(); }(); ```
Author
Member

Note that this is not semantically the same, because the failure case is not properly handled yet. Will use your code with added error handling.

Note that this is not semantically the same, because the failure case is not properly handled yet. Will use your code with added error handling.

In the end this is not constant variable, lambda now meaningless\

In the end this is not constant variable, lambda now meaningless\
JacquesLucke marked this conversation as resolved
@ -27,8 +27,10 @@ struct NodesModifierRuntime {
/**
* Contains logged information from the last evaluation.
* This can be used to help the user to debug a node tree.
* This is a shared pointer, because we might want to keep it around in some cases after the
Member

This is a shared pointer, because -> This is a shared pointer because