Geometry Nodes: allow naming geometry sets #114910

Jacques Lucke · 2023-11-15T12:12:07+01:00

Jacques Lucke commented

2023-11-15 12:12:07 +01:00

This adds a new name member to the GeometrySet class. This name can be set with the new Set Geometry Name node. Currently, the name is only used in the spreadsheet when displaying instances.

The main purpose of this name is to help debugging in instance trees. However, in the future it may also be used when exporting instance trees or when creating separate objects from them.

Note, the name is not expected to be unique, it is fully in user control.

Naming geometries is necessary to make the spreadsheet more useful for instances, because currently the user has no information for which geometry is used by each instance.

We also want to use this name to improve the integration with grease pencil where sometimes layers become instances with the same name.

This adds a new `name` member to the `GeometrySet` class. This name can be set with the new `Set Geometry Name` node. Currently, the name is only used in the spreadsheet when displaying instances. The main purpose of this name is to help debugging in instance trees. However, in the future it may also be used when exporting instance trees or when creating separate objects from them. Note, the name is not expected to be unique, it is fully in user control. ![image](/attachments/2615a02d-257a-49e9-b3ad-3408379e986a) Naming geometries is necessary to make the spreadsheet more useful for instances, because currently the user has no information for which geometry is used by each instance. We also want to use this name to improve the [integration with grease pencil](https://code.blender.org/2023/11/geometry-nodes-workshop-november-2023/#grease-pencil-integration) where sometimes layers become instances with the same name.

image.png

229 KiB

👀 1 ❤️ 3

Jacques Lucke added 1 commit 2023-11-15 12:12:21 +01:00

initial geometry names d7b20a5a75

Jacques Lucke requested review from Lukas Tönne 2023-11-15 12:12:56 +01:00

Jacques Lucke requested review from Falk David 2023-11-15 12:12:56 +01:00

Jacques Lucke requested review from Simon Thommes 2023-11-15 12:12:57 +01:00

Jacques Lucke requested review from Hans Goudey 2023-11-15 12:12:57 +01:00

Hans Goudey approved these changes 2023-11-15 14:07:12 +01:00

Hans Goudey left a comment

Code-wise this looks good.

source/blender/blenkernel/BKE_geometry_set.hh Outdated

						
				@ -137,2 +137,4 @@

				 public:

				  /**

				   * This is a user defined name for this geometry. It is not expected to be unique. It's main

Hans Goudey commented

2023-11-15 14:05:17 +01:00

This is a -> A
It's main -> Its main

- `This is a` -> `A` - `It's main` -> `Its main`

JacquesLucke marked this conversation as resolved

Jacques Lucke added 1 commit 2023-11-15 16:31:53 +01:00

typos ad8569401c

Falk David approved these changes 2023-11-16 10:57:47 +01:00

Jacques Lucke added 1 commit 2023-11-24 10:12:06 +01:00

Merge branch 'main' into geometry-names 6daa6683d3

Simon Thommes requested changes 2023-11-24 20:02:19 +01:00

Dismissed

Simon Thommes left a comment

It did feel a bit weird to me initially to add a feature like this that is mainly for debugging in a way that it actually affects and integrates into your node-tree while the data cannot actually be retrieved and used in the node-tree itself. But after testing it for a bit, I do think it's fine.

Would be good to have some nodes around extracting this information into string attributes once those are a thing to make it more useful.

Some notes:

Would be nice if the Object/Collection Info nodes would also write the geometry name to the output. (I could have sworn this was already a thing somehow...)
To make it more useful in the Spreadsheet it would be great if the Name column could also be used for filtering

Both of these things are not very controversial I think, but would already leverage the simple initial version of this to a more useful capacity. Not sure how this looks on the implementation side though. Let me know if you think these things aren't realistic or should be a separate PR.

Name Propagation

I do have some thoughts regarding propagating this information in the node-tree. The way it works right now feels to me like it hasn't been specifically designed, but is rather a consequence of the way things are implemented, so I think we need to put a bit more thought into this.

At first I thought about handling the case for joining geometry. From the user perspective it's not immediately obvious to me why joining would lose the name, even if for example there is just one input, or all names of the inputs are the same.
I can generally understand why some operations might construct an entirely new geometry set and would thus clear the name, but to some degree this seems like more of an implementation detail. For the join node, I think, it is arguable since we have an explicit order of inputs, that is relevant for the way things are joined, already regarding index order, that the first input can be considered a main input that things are joined into. The same is the case also for the Separate Components node. So it wouldn't be unreasonable to me to propagate the name of the first geometry. Again it feels more like the reason for the fact that it does not propagate the name is more so the implementation of the node than a design decision. The Separate Geometry node for example does propagate the name to both ouputs.

In a broader scope when looking at entire node-trees, most operations right now do actually just propagate the name.
For nodes like Set Position that just operate on attribute data it is quite obvious why you would keep the name the same. But at the same time, when viewing both the geometry before and after the operation in the spreadsheet they would not be one geometry set, but still share the same name.
But even the 'conversion' nodes that reconstruct the data in a different geometry component like curve to mesh, points to volume, instance on points propagate the name at the moment. This can potentially result in a lot of confusing duplication of names for different geometry sets, since essentially any geometry that results from a named geometry set will inherit that name.
And since there is a difference in this propagation between nodes, I do feel like there is an additional source for confusion coming from the fact that some operations propagate the name while others don't. I think it's a lot more clear if we put the responsibility of clearing the name entirely on the user, rather than doing it in certain cases.

So the most hardcore approach would be to immediately clear the name with every geometry node, which I don't think is a good idea.
Another approach would be to differentiate the behavior based on specific nodes based on some rule. The way it's done right now though, shows me that this can seem quite arbitrary, since it immediately makes assumptions over the way that the user attaches semantic meaning to that operation, which we should generally avoid. (e.g. after attaching some buttons to a shirt, I might still want to call it a shirt)

My Conclusion

Personally I would propose to keep things broadly as they are, but indeed extend the name propagation to all nodes that have a geometry in and output that have a reaonable association, so including Join and Separate. (I'm not sure how many more examples there are that I'm missing)

Overall I think it is worth considering a way to identify unique geometry sets in the spreadsheet beyond the name to alleviate the resulting confusion. E.g. showing a hash in gray text next to the name that is actually unique. Potentially also identifying unmodified collection/object data would be useful.
I understand that this goes beyond the scope of this PR, but imo it's a crucial aspect to how we approach the propagation of this data to make sure unique geometries are still differentiable.

It did feel a bit weird to me initially to add a feature like this that is mainly for debugging in a way that it actually affects and integrates into your node-tree while the data cannot actually be retrieved and used in the node-tree itself. But after testing it for a bit, I do think it's fine. Would be good to have some nodes around extracting this information into string attributes once those are a thing to make it more useful. Some notes: - Would be nice if the `Object/Collection Info` nodes would also write the geometry name to the output. (I could have sworn this was already a thing somehow...) - To make it more useful in the Spreadsheet it would be great if the Name column could also be used for filtering Both of these things are not very controversial I think, but would already leverage the simple initial version of this to a more useful capacity. Not sure how this looks on the implementation side though. Let me know if you think these things aren't realistic or should be a separate PR. ### Name Propagation I do have some thoughts regarding propagating this information in the node-tree. The way it works right now feels to me like it hasn't been specifically designed, but is rather a consequence of the way things are implemented, so I think we need to put a bit more thought into this. At first I thought about handling the case for joining geometry. From the user perspective it's not immediately obvious to me why joining would lose the name, even if for example there is just one input, or all names of the inputs are the same. I can generally understand why some operations might construct an entirely new geometry set and would thus clear the name, but to some degree this seems like more of an implementation detail. For the join node, I think, it is arguable since we have an explicit order of inputs, that is relevant for the way things are joined, already regarding index order, that the first input can be considered a main input that things are joined into. The same is the case also for the `Separate Components` node. So it wouldn't be unreasonable to me to propagate the name of the first geometry. Again it feels more like the reason for the fact that it does not propagate the name is more so the implementation of the node than a design decision. The `Separate Geometry` node for example does propagate the name to both ouputs. In a broader scope when looking at entire node-trees, most operations right now do actually just propagate the name. For nodes like `Set Position` that just operate on attribute data it is quite obvious why you would keep the name the same. But at the same time, when viewing both the geometry before and after the operation in the spreadsheet they would not be one geometry set, but still share the same name. But even the 'conversion' nodes that reconstruct the data in a different geometry component like curve to mesh, points to volume, instance on points propagate the name at the moment. This can potentially result in a lot of confusing duplication of names for different geometry sets, since essentially any geometry that results from a named geometry set will inherit that name. And since there is a difference in this propagation between nodes, I do feel like there is an additional source for confusion coming from the fact that some operations propagate the name while others don't. I think it's a lot more clear if we put the responsibility of clearing the name entirely on the user, rather than doing it in certain cases. So the most hardcore approach would be to immediately clear the name with every geometry node, which I don't think is a good idea. Another approach would be to differentiate the behavior based on specific nodes based on some rule. The way it's done right now though, shows me that this can seem quite arbitrary, since it immediately makes assumptions over the way that the user attaches semantic meaning to that operation, which we should generally avoid. (e.g. after attaching some buttons to a shirt, I might still want to call it a shirt) ### My Conclusion Personally I would propose to keep things broadly as they are, but indeed extend the name propagation to all nodes that have a geometry in and output that have a reaonable association, so including Join and Separate. (I'm not sure how many more examples there are that I'm missing) Overall I think it is worth considering a way to identify unique geometry sets in the spreadsheet beyond the name to alleviate the resulting confusion. E.g. showing a hash in gray text next to the name that is actually unique. Potentially also identifying unmodified collection/object data would be useful. I understand that this goes beyond the scope of this PR, but imo it's a crucial aspect to how we approach the propagation of this data to make sure unique geometries are still differentiable.

Iliya Katushenock commented

2023-11-24 20:21:53 +01:00

I agree with the @SimonThommes ' issues and this is why i would be against this kind of meta data. On good terms, there should be no meta data at all.
Any changes to implementation and functionality of node will cause problems here.
My approach would be something like: meta data on links between sockets. If it is a Reroute or a group node, then the metadata passes through. In any other case, the metadata is lost. If the node group implementation can get the metadata intarnally (with matched name and type), then everything is working correctly. Otherwise the error message...
But i see that it seems that this is not what the goal of this PR is...

I agree with the @SimonThommes ' issues and this is why i would be against this kind of meta data. On good terms, there should be no meta data at all. Any changes to implementation and functionality of node will cause problems here. My approach would be something like: meta data on links between sockets. If it is a `Reroute` or a group node, then the metadata passes through. In any other case, the metadata is lost. If the node group implementation can get the metadata intarnally (with matched name and type), then everything is working correctly. Otherwise the error message... But i see that it seems that this is not what the goal of this PR is...

Arye Ramaty commented

2023-11-25 21:38:54 +01:00

That's a great concept.
Even without a complete sting attribute implementation, I can already think of several use cases that would benefit from it.
I tested it for a bit, and aside from what Simon has said, I want to raise another UX point.
I understand the implementation reason that the 'set geometry name' is defined on the geometry level, nonetheless, as I see it, the geometry name is only relevant within the context of multiple geometries or the instance domain.
IMO, even though the data itself is part of the geometry set, controlling it from the instance domain may be more intuitive.
For example, after importing a cube object as an instance, you may want to rename the geometry default name (which is working fine in my tests). In the current proposal, the way to achieve that would be to realize the object, set its name, and then convert it back into an instance.
And from another aspect, if someday you would add the option to access it, it would probably be from the instance domain and not from the geometry itself.

That's a great concept. Even without a complete sting attribute implementation, I can already think of several use cases that would benefit from it. I tested it for a bit, and aside from what Simon has said, I want to raise another UX point. I understand the implementation reason that the 'set geometry name' is defined on the geometry level, nonetheless, as I see it, the geometry name is only relevant within the context of multiple geometries or the instance domain. IMO, even though the data itself is part of the geometry set, controlling it from the instance domain may be more intuitive. For example, after importing a cube object as an instance, you may want to rename the geometry default name (which is working fine in my tests). In the current proposal, the way to achieve that would be to realize the object, set its name, and then convert it back into an instance. And from another aspect, if someday you would add the option to access it, it would probably be from the instance domain and not from the geometry itself.

Jacques Lucke added 3 commits 2023-11-29 16:16:16 +01:00

Merge branch 'main' into geometry-names 3338892ca6

set geometry name in object/collection info nodes 5b7ecaaa25

propagate name in remaining nodes c597ce93c8

Jacques Lucke commented

2023-11-29 16:20:21 +01:00

IMO, even though the data itself is part of the geometry set, controlling it from the instance domain may be more intuitive.

A problem is that the name is not really on the instance domain, because that would mean that different instances could have different names even if they reference the same geometry. I know you probably mean to store the name only in the InstanceReference. Technically, that can work, but it doesn't seem more convenient to me right now.

In the current proposal, the way to achieve that would be to realize the object, set its name, and then convert it back into an instance.

I don't quite understand the need to realize the object.

> IMO, even though the data itself is part of the geometry set, controlling it from the instance domain may be more intuitive. A problem is that the name is not really on the instance domain, because that would mean that different instances could have different names even if they reference the same geometry. I know you probably mean to store the name only in the `InstanceReference`. Technically, that can work, but it doesn't seem more convenient to me right now. > In the current proposal, the way to achieve that would be to realize the object, set its name, and then convert it back into an instance. I don't quite understand the need to realize the object.

Jacques Lucke requested review from Simon Thommes 2023-11-29 16:20:35 +01:00

Arye Ramaty commented

2023-11-30 12:44:11 +01:00

Actually, I don't really mind where it is stored, it can be kept on the geometry, but it should be accessed and changed from the instance domain. It's the same way that the 'Fill Curve' node can work on the instance domain, but it actually changes data at the geometry level.

A problem is that the name is not really on the instance domain, because that would mean that different instances could have different names even if they reference the same geometry.

It is like how two geometries can have the same name - in contrast to mesh data.

I know you probably mean to store the name only in the InstanceReference. Technically, that can work, but it doesn't seem more convenient to me right now.

My main point is not 'where should the name be stored' but 'where should the user interact with the geometry name' in an escense I am being deliberately ignorant of the technical side of this.

My ultimate question is: what is the purpose of the node? In what scenario and how would it be used?

As I see it, the purpose of the node is to distinguish and associate geometries in the context of other geometries, aka within the Instance domain.

As for the 'scenarios it may be used in', currently, the design is only about visualization and debugging, but still, even the debugging is only relvant within the context of the instance domain.
I can imagine many utility nodes that can read (selecting) and write (swapping instance reference) that could utilize the 'geometry name', of course, all of them would also be from the POV of instances.

I don't quite understand the need to realize the object.

I often find myself tossing around multiple instances stored on an object (with a geometry node). In my example above, the key part is that you import the object as an instance -> you'll have to realize it for changing its name.

How it is	How I think it should be

Actually, I don't really mind where it is stored, it can be kept on the geometry, but it should be accessed and changed from the instance domain. It's the same way that the 'Fill Curve' node can work on the instance domain, but it actually changes data at the geometry level. > A problem is that the name is not really on the instance domain, because that would mean that different instances could have different names even if they reference the same geometry. > It is like how two geometries can have the same name - in contrast to mesh data. >I know you probably mean to store the name only in the InstanceReference. Technically, that can work, but it doesn't seem more convenient to me right now. > My main point is not 'where should the name be stored' but 'where should the user interact with the geometry name' in an escense I am being deliberately ignorant of the technical side of this. My ultimate question is: what is the purpose of the node? In what scenario and how would it be used? As I see it, the purpose of the node is to distinguish and associate geometries in the context of other geometries, aka within the **Instance domain**. As for the 'scenarios it may be used in', currently, the design is only about visualization and debugging, but still, even the debugging is only relvant within the context of the instance domain. I can imagine many utility nodes that can read (selecting) and write (swapping instance reference) that could utilize the 'geometry name', of course, all of them would also be from the POV of instances. > I don't quite understand the need to realize the object. I often find myself tossing around multiple instances stored on an object (with a geometry node). In my example above, the key part is that you import the object **as an instance** -> you'll have to realize it for changing its name. |How it is|How I think it should be| |-|-| |![image](/attachments/0b3ef35c-bac8-4d8f-b1c0-8c046831baba)|![image](/attachments/aacdfd06-1042-48bf-a562-30342cce0cfe)|

image.png

23 KiB

image.png

26 KiB

Jacques Lucke added 1 commit 2024-03-13 11:22:10 +01:00

Merge branch 'main' into geometry-names 7bde67977a

Jacques Lucke added 2 commits 2024-07-04 11:37:33 +02:00

Merge branch 'main' into geometry-names 73eb60bf37

fix after merge 0291a606a0

Jacques Lucke added 1 commit 2024-07-04 11:39:56 +02:00

Merge branch 'main' into geometry-names

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

fba53cd1e5

Jacques Lucke referenced this pull request

2024-07-04 20:09:09 +02:00

Spreadsheet: support showing data of nested instances (alternative 1) #124186

Jacques Lucke commented

2024-07-05 10:03:40 +02:00

@blender-bot build

Simon Thommes commented

2024-07-05 18:13:40 +02:00

What is the reason that you removed the text field from the input? I think it should be there.

What is the reason that you removed the text field from the input? I think it should be there. ![image](/attachments/e06f328b-6034-484a-8e85-bc5f743a2799)

image.png

20 KiB

Jacques Lucke commented

2024-07-05 18:22:04 +02:00

Hm, good point, not sure why it's done, will check.

Iliya Katushenock commented

2024-07-05 18:32:05 +02:00

This issue is in main...

Jacques Lucke added 1 commit 2024-07-06 14:17:17 +02:00

Merge branch 'main' into geometry-names 467f89665f

Jacques Lucke commented

2024-07-06 14:17:40 +02:00

What is the reason that you removed the text field from the input? I think it should be there.

This was fixed in main.

> What is the reason that you removed the text field from the input? I think it should be there. This was fixed in `main`.

Jacques Lucke closed this pull request

2024-07-06 14:17:43 +02:00

Jacques Lucke reopened this pull request

2024-07-06 14:17:47 +02:00

Iliya Katushenock reviewed 2024-07-06 14:27:00 +02:00

source/blender/blenkernel/BKE_geometry_set.hh

						
				@ -150,0 +152,4 @@

				   * purpose is help debugging instance trees. It may eventually also be used when exporting

				   * instance trees or when creating separate objects from them.

				   */

				  std::string name;

Iliya Katushenock commented

2024-07-06 14:27:00 +02:00

This string can not be shared and will be cause of large memory usage in common case with a lot of levels of nesting instances. In one level, here is just one instance handler so there is no a lot of geometry copy. But in case there is 2 levels, there is N copies of geometry. And since this string is not limited for its length here is can be really long text.

Jacques Lucke commented

2024-07-06 14:33:31 +02:00

It does use some extra memory, but I don't quite get the case that you describe. The GeometrySet is not stored for each instance separately, so it's not copied as often as you make it sound. I don't think this is something we need to be concerned about right now. There are ways to optimize it if we really need to in the future, but those also increase complexity.

It does use some extra memory, but I don't quite get the case that you describe. The `GeometrySet` is not stored for each instance separately, so it's not copied as often as you make it sound. I don't think this is something we need to be concerned about right now. There are ways to optimize it if we really need to in the future, but those also increase complexity.

Iliya Katushenock commented

2024-07-06 14:40:29 +02:00

The case that i describe is:
GeometrySet -> InstanceComponent -> InstanceHandle -> GeometrySet -> InstanceComponent -> InstanceHandle -> GeometrySet.
In this chain, 1 first geometry set, 1 second geometry set that is instantiated. So, last one is also just one.
But let there will be 2 different geometry at second level, with the same instances at 3 level. Now here is 2 copy of the most nested geometry.

The case that i describe is: GeometrySet -> InstanceComponent -> InstanceHandle -> GeometrySet -> InstanceComponent -> InstanceHandle -> GeometrySet. In this chain, 1 first geometry set, 1 second geometry set that is instantiated. So, last one is also just one. But let there will be 2 different geometry at second level, with the same instances at 3 level. Now here is 2 copy of the most nested geometry.

Jacques Lucke commented

2024-07-06 14:43:43 +02:00

Yeah, not a cause of concern for me currently.

~~Jacques Lucke referenced this pull request 2024-07-06 16:43:08 +02:00~~

Geometry Nodes: add conversion nodes for Grease Pencil and Curves #124279

Jacques Lucke added 1 commit 2024-07-09 10:00:42 +02:00

Merge branch 'main' into geometry-names 6f7ca48b80

Jacques Lucke added 1 commit 2024-07-09 10:03:57 +02:00

fix after merge 1aa4f3cad2

Jacques Lucke added 1 commit 2024-07-09 10:08:07 +02:00

fix showing name in spreadsheet a4dddaba15

Simon Thommes commented

2024-07-09 10:57:21 +02:00

What is the reason that you removed the text field from the input? I think it should be there.

Glad you addressed these already! That feels pretty good now imo.

I think it would be good to expose also the top level geometry set name somewhere. This could be shown in the tooltip of the geometry socket for example. I'm not sure where it would fit in the spreadsheet. Maybe in the breadcrumbs (?)

But generally I think this is pretty much good to go

> What is the reason that you removed the text field from the input? I think it should be there. Glad you addressed these already! That feels pretty good now imo. I think it would be good to expose also the top level geometry set name somewhere. This could be shown in the tooltip of the geometry socket for example. I'm not sure where it would fit in the spreadsheet. Maybe in the breadcrumbs (?) But generally I think this is pretty much good to go

Jacques Lucke added 2 commits 2024-07-09 11:54:23 +02:00

Merge branch 'main' into geometry-names 6b33759e4f

show geometry name in socket inspection tooltip 5aafb9ab75

Jacques Lucke commented

2024-07-09 11:57:03 +02:00

I think it would be good to expose also the top level geometry set name somewhere. This could be shown in the tooltip of the geometry socket for example.

I did this now. If there is a non-empty name, it will show in the tooltip.

I'm not sure where it would fit in the spreadsheet. Maybe in the breadcrumbs (?)

It will show there for instances (once #124186 is ready). We could show it there too (maybe only when using a viewer?), but that can also be done separately.

> I think it would be good to expose also the top level geometry set name somewhere. This could be shown in the tooltip of the geometry socket for example. I did this now. If there is a non-empty name, it will show in the tooltip. ![image](/attachments/b9535410-2731-4be5-9e38-f8f542fea9ef) > I'm not sure where it would fit in the spreadsheet. Maybe in the breadcrumbs (?) It will show there for instances (once #124186 is ready). We could show it there too (maybe only when using a viewer?), but that can also be done separately.

image.png

15 KiB

Simon Thommes commented

2024-07-09 12:06:37 +02:00

I did this now. If there is a non-empty name, it will show in the tooltip.

Looks great!

We could show it there too (maybe only when using a viewer?)

I was thinking about that as well, I think that could be fine, since then it would actually have value to inspect. I'd say let's try that. It feels weird to me to not show it in the spreadsheet, so I'd do it as part of this PR tbh

> I did this now. If there is a non-empty name, it will show in the tooltip. Looks great! > We could show it there too (maybe only when using a viewer?) I was thinking about that as well, I think that could be fine, since then it would actually have value to inspect. I'd say let's try that. It feels weird to me to not show it in the spreadsheet, so I'd do it as part of this PR tbh

Jacques Lucke commented

2024-07-09 12:15:30 +02:00

I was thinking about that as well, I think that could be fine, since then it would actually have value to inspect. I'd say let's try that. It feels weird to me to not show it in the spreadsheet, so I'd do it as part of this PR tbh

It's unfortunately not trivial right now because the breadcrumbs are currently drawn in Python and there is no access to the geometry set currently. We should probably move the code from space_spreadsheet.py to C++, but that's a bit more involved. If you insist, I can do this refactor first as a separate patch, but I'd rather do it separately afterwards to have fewer interdependent patches.

> I was thinking about that as well, I think that could be fine, since then it would actually have value to inspect. I'd say let's try that. It feels weird to me to not show it in the spreadsheet, so I'd do it as part of this PR tbh It's unfortunately not trivial right now because the breadcrumbs are currently drawn in Python and there is no access to the geometry set currently. We should probably move the code from `space_spreadsheet.py` to C++, but that's a bit more involved. If you insist, I can do this refactor first as a separate patch, but I'd rather do it separately afterwards to have fewer interdependent patches.

Simon Thommes commented

2024-07-09 12:17:25 +02:00

It's unfortunately not trivial right now because the breadcrumbs are currently drawn in Python and there is no access to the geometry set currently. We should probably move the code from space_spreadsheet.py to C++, but that's a bit more involved. If you insist, I can do this refactor first as a separate patch, but I'd rather do it separately afterwards to have fewer interdependent patches.

Ah okay, sure. It's fine to do separately then. I'll give this PR a final look now

> It's unfortunately not trivial right now because the breadcrumbs are currently drawn in Python and there is no access to the geometry set currently. We should probably move the code from space_spreadsheet.py to C++, but that's a bit more involved. If you insist, I can do this refactor first as a separate patch, but I'd rather do it separately afterwards to have fewer interdependent patches. Ah okay, sure. It's fine to do separately then. I'll give this PR a final look now

Simon Thommes commented

2024-07-09 12:43:59 +02:00

The tooltip is very helpful imo!

It does reveal something that I'm not sure about:

The other way around when instancing layers are removed I definitely think it makes sense to keep the top level name.

Also in this case, I think it's fine, since there is one explicit geometry that the instances are created on

So I think the Geometry to Instance node (which I could swear we wanted to rename to Geometry to Instances ages ago) is more of an exception where a new container geometry nodes set is created that should not have a name. Effectively, right now we end up with 2 different levels of instancing that have the same name. That's unique to this node, I believe and is probably confusing.

The way the Split to Instances node handles this seems fine to me, where only one level ends up with the name

I do feel more and more that these decisions are a little bit arbitrary though...

Also, this here seems straight up wrong, no? Maybe an implicit sharing issue?

The tooltip is very helpful imo! It does reveal something that I'm not sure about: ![image](/attachments/81a44885-f19e-4e05-95f4-c067a47c2f13) The other way around when instancing layers are removed I definitely think it makes sense to keep the top level name. ![image](/attachments/744dda54-e150-45c7-9410-5026bdb6f6e5) Also in this case, I think it's fine, since there is one explicit geometry that the instances are created on ![image](/attachments/c62ff325-2cb5-40c2-8960-cf2b6ad5e4ea) So I think the `Geometry to Instance` node (which I could swear we wanted to rename to `Geometry to Instances` ages ago) is more of an exception where a new container geometry nodes set is created that should not have a name. Effectively, right now we end up with 2 different levels of instancing that have the same name. That's unique to this node, I believe and is probably confusing. The way the `Split to Instances` node handles this seems fine to me, where only one level ends up with the name ![image](/attachments/3d0ad24b-c0b6-4170-8d90-6ef1b30e5006) I do feel more and more that these decisions are a little bit arbitrary though... Also, this here seems straight up wrong, no? Maybe an implicit sharing issue? ![image](/attachments/6120ecae-f595-4f03-84d6-0d4d1f07b9a0)

image.png

66 KiB

image.png

49 KiB

image.png

143 KiB

image.png

123 KiB

image.png

100 KiB

Jacques Lucke added 3 commits 2024-07-09 16:35:05 +02:00

Merge branch 'main' into geometry-names 1496718fb0

propagate name in Geometry to Instance only if there is exactly one input a5cd7b3295

fix geometry set equality check 05dfef576c

Jacques Lucke commented

2024-07-09 16:36:25 +02:00

It does reveal something that I'm not sure about

I changed it so that the Geometry to Instance node only propagates the name when there is exactly one input. This feels reasonable to me.

Also, this here seems straight up wrong, no? Maybe an implicit sharing issue?

Good find, fixed.

> It does reveal something that I'm not sure about I changed it so that the Geometry to Instance node only propagates the name when there is exactly one input. This feels reasonable to me. > Also, this here seems straight up wrong, no? Maybe an implicit sharing issue? Good find, fixed.

Simon Thommes referenced this pull request

2024-07-09 16:37:51 +02:00

Spreadsheet: support showing data of nested instances (alternative 1) #124186

Simon Thommes commented

2024-07-09 16:53:27 +02:00

I changed it so that the Geometry to Instance node only propagates the name when there is exactly one input. This feels reasonable to me.

I thought about that option as well, but I do find it odd that then we still end up with the situation that the propagation ends up with the same name on multiple instancing levels. To be honest, I'd either revert to falling back to the first one or not propagate at all, I think. Changing the behavior this much depending on the number of inputs seems more confusing than helpful imo.

> I changed it so that the Geometry to Instance node only propagates the name when there is exactly one input. This feels reasonable to me. I thought about that option as well, but I do find it odd that then we still end up with the situation that the propagation ends up with the same name on multiple instancing levels. To be honest, I'd either revert to falling back to the first one or not propagate at all, I think. Changing the behavior this much depending on the number of inputs seems more confusing than helpful imo.

Jacques Lucke added 1 commit 2024-07-09 16:56:37 +02:00

don't propagate name in Geometry to Instance node bc24c5294a

Simon Thommes commented

2024-07-09 17:00:10 +02:00

Regarding bc24c5294a don't propagate name in Geometry to Instance node

Just want to add that I think this behavior is consistent with the fact that we don't propagate (attribute) data from the instanced geometry up to the instance domain (except for specific cases, like the Split to Instances node). This is just a more specialized aspect of that.

Regarding bc24c5294a `don't propagate name in Geometry to Instance node` Just want to add that I think this behavior is consistent with the fact that we don't propagate (attribute) data from the instanced geometry up to the instance domain (except for specific cases, like the `Split to Instances` node). This is just a more specialized aspect of that.

Simon Thommes approved these changes 2024-07-09 17:02:05 +02:00

Simon Thommes left a comment

This is good to be merged to me now. I'm not entirely certain we're not missing any corner cases, where the propagation behavior could be tweaked, but that can still be ironed out, if we run into something.

Jacques Lucke merged commit dc8e9678e1 into main

2024-07-09 17:04:12 +02:00

Jacques Lucke referenced this issue from a commit

2024-07-09 17:04:13 +02:00

Geometry Nodes: allow naming geometry sets

Jacques Lucke deleted branch geometry-names

2024-07-09 17:04:15 +02:00

Jacques Lucke referenced this pull request

2024-07-26 13:12:53 +02:00

Spreadsheet: support navigating instance trees #125293

Jacques Lucke referenced this issue from a commit

2024-07-29 20:42:26 +02:00