Behavior seems great to me even with different combination of pinned contexts!

Found a couple of crashes and bugs:

• Adding output with either dragging a connection to the group output or bpy.ops.node.tree_socket_add(in_out='OUT') in the output UIList
• Outputs don't seem to get properly exposed in the node interface. E.g. create output that has position as the default attribute. It will work as a proper output, but not be shown in the node interface.

Looks quite good to me! Also nice how this works for point clouds.

Thanks for the change, looks good to me now!

Patch looks good to me for 99% of cases! The regular behavior with points, edges and polugons looks good.

This looks good!
There is a crash with the example file, but that is also happening in master, so likely due to the curve resample node:
T88603

It's working well for me from a user perspective.
Apart from the fact that curves should be drawn after they are made real I couldn't find any issues!

I checked the patch with 2 different production files from Sprite Fright using geometry nodes in multiple different ways. All seems to work well!

Don't really have a way to test the functionality as we don't have a way to pass around textures with other nodes yet, but looks good!

Apart from the issues outlined, this is looking good to me now from a user perspective!

Apart from the related UI issue of type changing that we have to address separately this looks look to go for me now, will be nice to have this in master!

@Falk David (filedescriptor) great, thanks! Very niche issue, but it's driving me a bit insane :D

Works great for the propagation of materials for instances that are made real!

@Philipp Oeser (lichtwerk) Thanks for looking into it! For now I worked around the issue by fully recreating the object using geometry nodes to transfer the geometry data.

Working great in general!

Thanks for adding this!

In D10959#280653, @Tomas (Rusculleda) wrote:

I don't know if it's been mentioned already, but all the animation editors (except timeline) have a selected only option. In those cases it's in the header, and it's an icon

Maybe this could use the same?

I'm honestly not a big fan of combining the Selected Only option with the filters under one toggle.
To me the selection works on a separate level as it's strongly and globally connected to the viewport and should be a separate toggle from the filters that are local per editor.

In D10931#279833, @Jacques Lucke (JacquesLucke) wrote:

I think after changing the active modifier the context should also update to the state of that new active modifier.

That is doable, but I think I'd rather do it as a bug fix instead, just to keep this patch a little bit simpler.

Very nice!
I think the design of the filters themselves works super well.

I think the general behaviour as it is now is nice!
Small request though:
I think after changing the active modifier the context should also update to the state of that new active modifier.

Okay, thanks for the responses!

Thanks for implementing this! Absolutely fantastic node to have :)

In D10875#278493, @Charlie Jolly (charlie) wrote:

• It wasn't obvious that I needed to change the spreadsheet evaluation state to get this to work.
  • Link the evaluation state dynamically so if a node is explicitly toggled the spreadsheet automatically reflects this.

• It is not possible to uncheck a toggled node.
  • Make the toggle work as a true toggle so it can be unchecked. When untoggled, the evaluation state revert backs to Final.

In D10875#278155, @Jacques Lucke (JacquesLucke) wrote:

Thanks for making this change, I much prefer the way this works.
I do think that it has to be more clearly readable which node is being inspected. The icon alone does not grab a lot of attention and it is quite crucial which node is inspected, especially once this is connected to a viewport inspection.
So I would propose adding something more striking that identifies the inspected node in the UI.
An idea that comes to mind would be a thick border of multiple pixels around the node.

In T86499#1141246, @Fabian Schempp (fabian_schempp) wrote:

I think when we get the data set sidebar for the spreadsheet we may need some additional horizontal space.
So maybe we use a 50/50 split between Spreadsheet and 3DView?

You've done a great job of bringing the patch this far and I think it would be great if you could bring it all the way into master as well. But, of course, you're not obligated to do that and I can understand that when there are regularly more requests made, that it can just be too much.

Okay great, thanks :)

Thanks for addressing my feedback!
I'm pretty happy with what we have with this node now!

For meshes it looks all good to me now!

Thanks for addressing my feedback!

Works great!

In D10506#277104, @Hans Goudey (HooglyBoogly) wrote:

One thing I've been doing in mockups is naming the fields "Mesh" or "Volume" or "Curve" instead of "Geometry". We haven't "officially" agreed on that, but I think it's a nice solution to this.

I would still prefer it if we had implicit conversion, honestly. Then the type should not matter. But for when we explicitly have only one possible input component that is being used that sounds like a good idea!

I would actually do this the same way as with the surface distribution node, calling it Density Attribute.

IMO the "Attribute" at the end is a bit too long, and I don't really think it's necessary, it goes without saying that a string field refers to an attribute in geometry nodes.

I agree, but having two inputs both called Density doesn't sound like a good solution to me either. We could call the attribute input Mask. I remember that we decided against that, but I can't see a great option right now.

Hopefully in the future we can find some way to abstract this that makes it less painful.

Agreed :D

Thanks for the response!

I think the behavior of being able to control the density with both inputs to a similar capacity without capping it is fine. But I do agree that the Naming should change. I think just using Density is totally fine even though it is more vague. Density Factor is a bit misleading when no attribute is used.

I forgot to mention one thing:
The default spacing of (0.3,0.3,0.3) seems a bit arbitrary to me. I would just put it at (1.0,1.0,1.0) that is equivalent to the default density of 1 and also means less points by default which is good when dealing with large volumes.

Just chatted about this with Hans, Jacques and Pablo and there was a consensus that we should probably rename the Grid attribute to Density.

Just checked the patch for the first time and it looks great!
I do have some concerns from a usability standpoint, regarding the context of this node.

In T86499#1139018, @Kenzie (kenziemac130) wrote:

Should the workspace be named something different then "Geometry Nodes"? Maybe "Procedural Modeling" or something similar? It seems the other categories describe the workflow rather than the system or a the name of a single editor.

Actually... maybe the outliner should be more prominent, as we support linking collections/objects.
So maybe the same proportions as Layout would be good here:

I was thinking of this for the workspace:

I haven't been following the full conversation but from what I see the main open question was the placement of the selected node.
Something that I would actually expect instead of the current behavior is that the node is not directly placed but instead grabbed by the cursor just like when adding a node.
That adds an additional click but I think it would come intuitively and might be a nice way of solving the question of placement by putting it directly into the hands of the user.

In D10459#276380, @Charlie Jolly (charlie) wrote:

@Simon Thommes (simonthommes) this is really good feedback. I agree with pretty much everything you've said.

Great, I'm glad!

Okay, I tested the patch and have a bunch of impressions.
First of all: I think it's super useful to have alternative random distributions, especially the symmetrical ones around a center directly inside of this node!

@Charlie Jolly (charlie) Is this patch ready to test? If so could you merge it with current master? It didn't build for me when I tried it just now.

In D10420#274940, @Some User (someuser) wrote:

hm, it make sense to make Boundbox output as a part of 'Object/Collection Info' Node?

The way this is split up into details totally makes sense to me.
In terms of needed mapping methods I have a couple of thoughts:

• How many options are necessary is quite difficult to say. Unfortunately, which option works best, can heavily rely on the specific case and topology.
• At least there should probably also be a 'Projected' option
• By far the biggest issue is the transfer on the Corner domain, with for example UV seams. Even the current operator/modifier works far from ideal in this case.
  • Having some method that is a bit smarter when handling split corners would be nice in general...

Thanks for the response!

Just tested the patch and I think it's a good idea!
A few things though:

• The operator settings should be exposed as tool settings instead

• When the sampler is activated in another tool by holding the modifier key it should already indicate that in the cursor display.
• These same changes should also be added in parallel to the paint mode of the image editor

@Charlie Jolly (charlie) thanks for looking into it that looks good now! I didn't really have an example file, sorry. Just seemed wrong to me when looking at it.

In D10459#273483, @Charlie Jolly (charlie) wrote:

Add Triangular distribution

Left to right:
Uniform, Midpoint, Gaussian, Triangular

I made a demo file for this to show some of it's potential with the current status of geometry nodes:

Muting logic works perfectly now, from what I've been testing! Nice job!

Looks good to me!
At some point when we have an attribute data type more exposed it might make sense to drop the the enum and do it automatic instead but that's besides this patch.

Tested again, seems to be working great for the most part! The tagging works great and both up- and downstream muting usually give the expected results.
But there seems to be two issues left that I could find.

1. That is when muting two wires that lead to the same reroute node upstream. (Same for doing the same in two separate muting steps.) If all outgoing wires are muted/unmuted that should also propagate this state upstream. Otherwise that results in this kind of behaviour:

2. Unmuting should probably always propagate upstream. Otherwise it can result in something like this:

Thank you for the adjustments! Looks good to me now from a user standpoint. I'll leave the final sign-off to code review by a developer.

I mean if this change is going to be redundant soon it doesn't really matter, @Dalai Felinto (dfelinto) has a point.
But once we have the navigation in the side panel, I think it would still make sense to hide the ones that have no data by design, or gray them out. So I think if this will be easier already addressing it in the current version and doesn't create unnecessary work, it could still be done now.

I agree that this node should be in Utilities.
Other than that, it should work the opposite way than it does right now. I'm am not sure why this was changed but in the original design it was shown that True should return B, not A.