- User Since
- Nov 29 2018, 10:03 PM (133 w, 6 d)
Fri, Jun 18
Behavior seems great to me even with different combination of pinned contexts!
Wed, Jun 16
Tue, Jun 15
Thu, Jun 10
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.
Tue, Jun 8
Looks quite good to me! Also nice how this works for point clouds.
Fri, Jun 4
Wed, Jun 2
Tue, Jun 1
Thanks for the change, looks good to me now!
Wed, May 26
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:
May 23 2021
May 21 2021
May 17 2021
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!
May 14 2021
I checked the patch with 2 different production files from Sprite Fright using geometry nodes in multiple different ways. All seems to work well!
May 12 2021
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!
May 10 2021
Apart from the issues outlined, this is looking good to me now from a user perspective!
May 7 2021
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!
May 6 2021
May 5 2021
@Falk David (filedescriptor) great, thanks! Very niche issue, but it's driving me a bit insane :D
May 4 2021
Works great for the propagation of materials for instances that are made real!
May 3 2021
@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.
Apr 29 2021
Apr 26 2021
Apr 22 2021
Working great in general!
Thanks for adding this!
Apr 19 2021
Apr 16 2021
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.
Apr 14 2021
Apr 13 2021
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!
Apr 12 2021
Thanks for implementing this! Absolutely fantastic node to have :)
Apr 9 2021
Apr 8 2021
Apr 7 2021
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.
Maybe @Pablo Vazquez (pablovazquez) has a good idea for that.
Apr 6 2021
Apr 2 2021
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!
Apr 1 2021
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.
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.
Mar 31 2021
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.
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:
Mar 30 2021
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.
Great, I'm glad!
Mar 29 2021
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.
Mar 26 2021
Mar 25 2021
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!
Mar 23 2021
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.
Mar 22 2021
Mar 18 2021
Mar 17 2021
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!
Mar 16 2021
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.
Mar 15 2021
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.
- 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:
- Unmuting should probably always propagate upstream. Otherwise it can result in something like this:
Mar 12 2021
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.