Geometry Nodes: add Inspection Index to Repeat Zone #112818

Jacques Lucke · 2023-09-24T22:19:28+02:00

Jacques Lucke commented

2023-09-24 22:19:28 +02:00

Previously, it was only possible to inspect the data from the first iteration. That applied to both, the viewer node as well as socket inspection. Now, there is a new Inspection Index setting in the zone properties. It specifies which iteration should be used by the inspection features.

In theory we could support features like counting the index from the end, but that can be done separately as well, as it likely requires more UI.

Previously, it was only possible to inspect the data from the first iteration. That applied to both, the viewer node as well as socket inspection. Now, there is a new `Inspection Index` setting in the zone properties. It specifies which iteration should be used by the inspection features. In theory we could support features like counting the index from the end, but that can be done separately as well, as it likely requires more UI.

👍 1

Jacques Lucke added 3 commits 2023-09-24 22:19:44 +02:00

8578f4e28f initial viewer iteration

431f876864 support keeping viewer active when changing iteration

fa78335266 improve comments

Jacques Lucke requested review from Simon Thommes 2023-09-24 22:20:29 +02:00

Jacques Lucke requested review from Hans Goudey 2023-09-24 22:20:29 +02:00

Simon Thommes requested changes 2023-09-25 12:24:01 +02:00

Simon Thommes left a comment

This simple example is not working for me, it seems:

Intuitively, the iteration should start counting at 1, rather than 0 imo. It should read like 1st iteration up to 10th iteration. 0 should be out of range.

This simple example is not working for me, it seems: ![image](/attachments/19e868d8-86cc-421d-bc90-9635b9f36106) Intuitively, the iteration should start counting at 1, rather than 0 imo. It should read like 1st iteration up to 10th iteration. 0 should be out of range.

image.png

107 KiB

Simon Thommes commented

2023-09-25 12:27:13 +02:00

Btw, I think the way of exposing this in the UI as a setting of the repeat in/output is far from ideal. I don't have a good solution to that right now, but it would be a shame if it just stayed there and wouldn't be exposed in a more visible/accessible way, more closely related to the viewer node at some point later. Are there plans to do this in the future and this is just an initial step to get the feature in?

Jacques Lucke commented

2023-09-25 12:59:10 +02:00

Making the iteration start at 1 could work, but I'm not sure if this will become more of a problem later. In the foreach zone we likely will start at index zero, because that's also the index that is passed into the loop.

Showing the setting in the viewer node as well can work. It's not entirely clear how that would work with multiple nested repeat zones though. The iteration still likely has to be stored in the zone for socket inspection.

Making the iteration start at 1 could work, but I'm not sure if this will become more of a problem later. In the foreach zone we likely will start at index zero, because that's also the index that is passed into the loop. Showing the setting in the viewer node as well can work. It's not entirely clear how that would work with multiple nested repeat zones though. The iteration still likely has to be stored in the zone for socket inspection.

Simon Thommes commented

2023-09-25 14:43:35 +02:00

Making the iteration start at 1 could work, but I'm not sure if this will become more of a problem later. In the foreach zone we likely will start at index zero, because that's also the index that is passed into the loop.

That setting in the for each loop would not be called iteration though, right? More something like Element Index. An index seems more natural to begin with 0. To me that's different intuitively.

Showing the setting in the viewer node as well can work. It's not entirely clear how that would work with multiple nested repeat zones though. The iteration still likely has to be stored in the zone for socket inspection.

I would definitely not store this in the viewer node. It's just a matter of exposing this in the UI in a useful way. But tbh, I see the usecases for this feature relatively limited, as it still runs through the rest of the zone on each iteration, so I'd usually just want to inspect the first one or how the result changes with more iterations. So just adding it simply like this for now it probably fine.

> Making the iteration start at 1 could work, but I'm not sure if this will become more of a problem later. In the foreach zone we likely will start at index zero, because that's also the index that is passed into the loop. That setting in the for each loop would not be called iteration though, right? More something like Element Index. An index seems more natural to begin with 0. To me that's different intuitively. > Showing the setting in the viewer node as well can work. It's not entirely clear how that would work with multiple nested repeat zones though. The iteration still likely has to be stored in the zone for socket inspection. I would definitely not store this in the viewer node. It's just a matter of exposing this in the UI in a useful way. But tbh, I see the usecases for this feature relatively limited, as it still runs through the rest of the zone on each iteration, so I'd usually just want to inspect the first one or how the result changes with more iterations. So just adding it simply like this for now it probably fine.

Hans Goudey commented

2023-09-25 20:41:43 +02:00

Thinking about this more, it seems like it would be nice if iteration index was defined in the viewer node or elsewhere in the viewer path, rather than in the original node tree. Conceptually that separates the "viewing" from the actual execution instructions. It's also consistent with the choice of domain and type.

To handle nested repeat zones, I guess it would have to be adjustable somewhere in the viewer path UI. Maybe the complexity means it isn't so possible though?

EDIT: Oops, didn't reload the page to read the existing discussion before commenting..

Thinking about this more, it seems like it would be nice if iteration index was defined in the viewer node or elsewhere in the viewer path, rather than in the original node tree. Conceptually that separates the "viewing" from the actual execution instructions. It's also consistent with the choice of domain and type. To handle nested repeat zones, I guess it would have to be adjustable somewhere in the viewer path UI. Maybe the complexity means it isn't so possible though? EDIT: Oops, didn't reload the page to read the existing discussion before commenting..

Jacques Lucke commented

2023-09-25 20:48:21 +02:00

Note that the iteration index is already stored in the viewer path (and therefore is also pinned if you pin a viewer). The iteration in the zone is used when creating the viewer path when a viewer is activated, and for socket inspection.

Jacques Lucke commented

2023-09-26 09:51:09 +02:00

That setting in the for each loop would not be called iteration though, right? More something like Element Index. An index seems more natural to begin with 0. To me that's different intuitively.

I wonder if this starting at 0 would be more ok if the property is called Debug/Viewer Index. I still find starting to count at 1 fairly disputable and unexpected in any system that is remotely similar to programming.

> That setting in the for each loop would not be called iteration though, right? More something like Element Index. An index seems more natural to begin with 0. To me that's different intuitively. I wonder if this starting at 0 would be more ok if the property is called `Debug/Viewer Index`. I still find starting to count at 1 fairly disputable and unexpected in any system that is remotely similar to programming.

Simon Thommes commented

2023-09-26 11:49:35 +02:00

I wonder if this starting at 0 would be more ok if the property is called Debug/Viewer Index. I still find starting to count at 1 fairly disputable and unexpected in any system that is remotely similar to programming.

It should be called something with index in that case then imo. That's fine then. But just Iteration implies a natural count to me.

When you guys are talking about storing it in the viewer path, what does that imply? Just want to point out that storing this information in the viewer node itself would mean that switching between different repeat zones would be problematic.
But I'm guessing what you're discussing it more of an implementation detail rather than that.

I wonder if this starting at 0 would be more ok if the property is called `Debug/Viewer Index`. I still find starting to count at 1 fairly disputable and unexpected in any system that is remotely similar to programming. It should be called something with index in that case then imo. That's fine then. But just `Iteration` implies a natural count to me. When you guys are talking about storing it in the viewer path, what does that imply? Just want to point out that storing this information in the viewer node itself would mean that switching between different repeat zones would be problematic. But I'm guessing what you're discussing it more of an implementation detail rather than that.

Jacques Lucke commented

2023-09-26 12:15:55 +02:00

When you guys are talking about storing it in the viewer path, what does that imply?

Storing it in the viewer path mainly implies that it is also part of what is pinned e.g. in the spreadsheet.

Just want to point out that storing this information in the viewer node itself would mean that switching between different repeat zones would be problematic.

I don't think that just storing it in the viewer node is a proper solution.

> When you guys are talking about storing it in the viewer path, what does that imply? Storing it in the viewer path mainly implies that it is also part of what is pinned e.g. in the spreadsheet. > Just want to point out that storing this information in the viewer node itself would mean that switching between different repeat zones would be problematic. I don't think that just storing it in the viewer node is a proper solution.

👍 1

Jacques Lucke added 2 commits 2023-09-26 14:40:50 +02:00

d09d59e7ef Merge branch 'main' into repeat-viewer-iteration

71e6f6778d rename to inspection index

Jacques Lucke changed title from ~~Geometry Nodes: add Viewer Iteration to Repeat Zone~~ to Geometry Nodes: add Inspection Index to Repeat Zone

2023-09-26 14:42:58 +02:00

Jacques Lucke requested review from Simon Thommes 2023-09-26 14:43:39 +02:00

Hans Goudey approved these changes 2023-09-26 15:42:52 +02:00

source/blender/blenkernel/BKE_viewer_path.h

						
				@ -35,10 +35,16 @@ struct IDRemapper;

				extern "C" {

				#endif

				enum ViewerPathEqualFlag {

Hans Goudey commented

2023-09-26 15:40:59 +02:00

An enum class would be much prettier here ;)

An `enum class` would be much prettier here ;)

Simon Thommes approved these changes 2023-09-26 15:59:19 +02:00

Simon Thommes left a comment

false comment

Simon Thommes requested changes 2023-09-26 15:59:47 +02:00

Simon Thommes left a comment

I think it would be good if there was some sort of feedback, when the Inspection index is >= the iteration number rather than just showing nothing for the preview. I think there should be a warning where you set the inspection index that it is out of range and imo the evaluation should just clamp it to the maximum, so you just see the data of highest applicable iteration.

Jacques Lucke added 2 commits 2023-09-26 23:17:18 +02:00

2e494d6102 Merge branch 'main' into repeat-viewer-iteration

b3c2e13bfc show warning when inspection index is out of range

Jacques Lucke commented

2023-09-26 23:20:05 +02:00

I added a warning now when the inspection index is higher than the number or iterations.

Doing the clamping sounds reasonable, but is unfortunately not super straight forward to do right now. I think I'd rather do that separately. The fundamental problem is that with the clamping the viewer path that is setup does not match exactly the viewer path that will be displayed (the iteration index will differ if it's clamped at run-time). We can certainly make this work, but it requires a bit more refactoring.

I added a warning now when the inspection index is higher than the number or iterations. Doing the clamping sounds reasonable, but is unfortunately not super straight forward to do right now. I think I'd rather do that separately. The fundamental problem is that with the clamping the viewer path that is setup does not match exactly the viewer path that will be displayed (the iteration index will differ if it's clamped at run-time). We can certainly make this work, but it requires a bit more refactoring.

Jacques Lucke requested review from Simon Thommes 2023-09-26 23:20:13 +02:00

Jacques Lucke merged commit c8cc169d6f into main

2023-09-27 11:09:46 +02:00

Jacques Lucke referenced this issue from a commit

2023-09-27 11:09:47 +02:00

Geometry Nodes: add Inspection Index to Repeat Zone

Jacques Lucke deleted branch repeat-viewer-iteration

2023-09-27 11:09:48 +02:00

Nate Rupsis referenced this issue from a commit

2023-09-28 20:19:25 +02:00

Geometry Nodes: add Inspection Index to Repeat Zone

Colin Basnett referenced this issue from a commit