Nodes: support looking up node sockets by identifier and name #113984

Jacques Lucke · 2023-10-20T22:08:13+02:00

Jacques Lucke commented

2023-10-20 22:08:13 +02:00

The main goal of this patch is to turn node.inputs[...] and node.outputs[...] into the main way to lookup sockets on a node. Currently, it's used for that sometimes, but often it is not, because it does not work in all cases. More specifically, it does not work when a node has multiple sockets with the same name but different identifiers, which is relatively common.

This patch proposes to make the string lookup more convenient, more useful and less prone to breaking after changes in Blender.

This is achieved by changing how the lookup works:

First, it tries to find an available socket with an identifier that matches the given key. This is checked first, it makes sure that every socket can be accessed by its identifier. The identifier matching is currently disabled in some nodes where we want to change identifiers soonish.
If that didn't work, it tries to find an available socket with a matching name. This is often convenient because it generally matches what can be seen in the UI. Furthermore, it's also necessary to avoid breakage with old usages of this lookup function.
If both options above didn't work, it checks whether the given key matches any socket name/identifier that the node used to have in the past, but has been renamed for whatever reason. This mainly exists to avoid breaking scripts by certain kinds of changes in the future (e.g. #113553). Note that this patch does not include any of this behavior yet.

Note, previously, string lookup would also find unavailable sockets. This is disabled now, because the way we handle unavailable sockets internally is likely to change. Therefor, any use of unavailable sockets might break (unavailable != hidden). For the time being, it's still possible to find unavailable sockets by iterating over all sockets.

This change has a small chance to break existing scripts because the behavior changes for some inputs in some nodes. However, for the cases where the function is used in practice, I don't really expect any breakage.

This patch also allows us to give a better answer to reports like #113106.

The main goal of this patch is to turn `node.inputs[...]` and `node.outputs[...]` into the main way to lookup sockets on a node. Currently, it's used for that sometimes, but often it is not, because it does not work in all cases. More specifically, it does not work when a node has multiple sockets with the same name but different identifiers, which is relatively common. This patch proposes to make the string lookup more convenient, more useful and less prone to breaking after changes in Blender. This is achieved by changing how the lookup works: * First, it tries to find an available socket with an identifier that matches the given key. This is checked first, it makes sure that every socket can be accessed by its identifier. The identifier matching is currently disabled in some nodes where we want to change identifiers soonish. * If that didn't work, it tries to find an available socket with a matching name. This is often convenient because it generally matches what can be seen in the UI. Furthermore, it's also necessary to avoid breakage with old usages of this lookup function. * If both options above didn't work, it checks whether the given key matches any socket name/identifier that the node used to have in the past, but has been renamed for whatever reason. This mainly exists to avoid breaking scripts by certain kinds of changes in the future (e.g. #113553). Note that this patch does not include any of this behavior yet. Note, previously, string lookup would also find unavailable sockets. This is disabled now, because the way we handle unavailable sockets internally is likely to change. Therefor, any use of unavailable sockets might break (unavailable != hidden). For the time being, it's still possible to find unavailable sockets by iterating over all sockets. This change has a small chance to break existing scripts because the behavior changes for some inputs in some nodes. However, for the cases where the function is used in practice, I don't really expect any breakage. This patch also allows us to give a better answer to reports like #113106.

Jacques Lucke force-pushed node-sockets-string-lookup from d81efd7530 to 1975cb0c84

2023-10-20 22:36:50 +02:00

Compare

Jacques Lucke changed title from ~~Nodes: support looking up node sockets by name and identifier~~ to Nodes: support looking up node sockets by name and identifier

2023-10-20 22:37:02 +02:00

Jacques Lucke changed target branch from main to blender-v4.0-release

2023-10-20 22:37:04 +02:00

Jacques Lucke commented

2023-10-20 22:39:02 +02:00

@blender-bot build

Jacques Lucke requested review from Brecht Van Lommel 2023-10-20 22:40:26 +02:00

Jacques Lucke requested review from Lukas Tönne 2023-10-20 22:40:26 +02:00

Jacques Lucke requested review from Hans Goudey 2023-10-20 22:40:26 +02:00

Jacques Lucke changed title from ~~Nodes: support looking up node sockets by name and identifier~~ to Nodes: support looking up node sockets by identifier and name

2023-10-20 22:40:37 +02:00

Hans Goudey approved these changes 2023-10-20 22:47:23 +02:00

Hans Goudey left a comment

This approach makes sense to me, it's a nice compromise, and the code is pretty obvious.
I do wonder about keeping the baggage of older names around after changes. Though this design just gives us that ability I guess. We'd still have the option of only keeping those names for the next major version or so.

This approach makes sense to me, it's a nice compromise, and the code is pretty obvious. I do wonder about keeping the baggage of older names around after changes. Though this design just gives us that ability I guess. We'd still have the option of only keeping those names for the next major version or so.

Jacques Lucke commented

2023-10-20 22:51:45 +02:00

I do wonder about keeping the baggage of older names around after changes. Though this design just gives us that ability I guess. We'd still have the option of only keeping those names for the next major version or so.

Thing is, what's baggage to us, is extra work for others. If we can keep it local, I don't mind keeping it for longer as long as it doesn't conflict with anything. But yeah, we do at least have more options of the string-lookup is not strictly a name lookup anymore.

> I do wonder about keeping the baggage of older names around after changes. Though this design just gives us that ability I guess. We'd still have the option of only keeping those names for the next major version or so. Thing is, what's baggage to us, is extra work for others. If we can keep it local, I don't mind keeping it for longer as long as it doesn't conflict with anything. But yeah, we do at least have more options of the string-lookup is not strictly a name lookup anymore.

Hans Goudey commented

2023-10-20 22:53:58 +02:00

Also "baggage" to the extent that having multiple ways to do things complicates the use of an API. But at least it clearly is more "correct" to use the latest name, so that concern isn't a big deal at all, just thought it was worth mentioning.

Lukas Tönne commented

2023-10-23 10:03:04 +02:00

This change has a small chance to break existing scripts because the behavior changes for some inputs in some nodes

Are there specific cases you were thinking of here?

If it's really necessary the API could get additional lookup functions for identifiers and for names respectively, but i don't see a use case for that.

> This change has a small chance to break existing scripts because the behavior changes for some inputs in some nodes Are there specific cases you were thinking of here? If it's _really_ necessary the API could get additional lookup functions for identifiers and for names respectively, but i don't see a use case for that.

Lukas Tönne approved these changes 2023-10-23 10:03:31 +02:00

Brecht Van Lommel commented

2023-10-23 14:40:41 +02:00

In my opinion the better API would be for inputs and outputs with dynamic types to appear and disappear in the list, rather than existing for all types with a unique identifier. Recommending users to use the identifier in cases like #113106 then would be going in the wrong direction.

I would be more inclined to make the string lookup prefer available inputs over unavailable ones.

In my opinion the better API would be for inputs and outputs with dynamic types to appear and disappear in the list, rather than existing for all types with a unique identifier. Recommending users to use the identifier in cases like #113106 then would be going in the wrong direction. I would be more inclined to make the string lookup prefer available inputs over unavailable ones.

Jacques Lucke commented

2023-10-23 14:50:39 +02:00

In my opinion the better API would be for inputs and outputs with dynamic types to appear and disappear in the list, rather than existing for all types with a unique identifier.

Definitely, this is what #113553 is doing (also mentioned in the description of this patch). One goal of this patch is to allow Python scripts to use node.inputs[...] with the identifier in 4.0 in a way that still works in 4.1 when we want to unify the sockets.

> In my opinion the better API would be for inputs and outputs with dynamic types to appear and disappear in the list, rather than existing for all types with a unique identifier. Definitely, this is what #113553 is doing (also mentioned in the description of this patch). One goal of this patch is to allow Python scripts to use `node.inputs[...]` with the identifier in 4.0 in a way that still works in 4.1 when we want to unify the sockets.

Jacques Lucke commented

2023-10-23 14:55:11 +02:00

The recommendation in #113106 goes against that indeed. Unfortunately, that's the main solution in 4.0 currently. This patch tries to improve what we recommend to people. It also changes the recommendation I'd give in #113106.

Brecht Van Lommel commented

2023-10-23 14:58:52 +02:00

I'm not sure I understand. We want to recommend add-ons to use the identifier with the type name now in 4.0, and then immediately make it unnecessary in 4.1 and recommend the 3.6 way again?

Wouldn't preferring available sockets in the lookup be simpler then?

I'm not sure I understand. We want to recommend add-ons to use the identifier with the type name now in 4.0, and then immediately make it unnecessary in 4.1 and recommend the 3.6 way again? Wouldn't preferring available sockets in the lookup be simpler then?

Jacques Lucke commented

2023-10-23 15:07:04 +02:00

Ignoring unavailable sockets in this lookup seems like more of breaking change to me. Could also do that though. Generally, it still seems better if we can change the string lookup to use the identifier instead of name (using the name is causing problems with duplicate names as well, even without dynamic sockets), and doing that change in 4.0 seems better than in 4.1.

I could add code so that unavailable sockets are ignored by this patch already. I'm mainly not sure if unavailable sockets should be ignored only in the name or also in the identifier comparison.

Ignoring unavailable sockets in this lookup seems like more of breaking change to me. Could also do that though. Generally, it still seems better if we can change the string lookup to use the identifier instead of name (using the name is causing problems with duplicate names as well, even without dynamic sockets), and doing that change in 4.0 seems better than in 4.1. I could add code so that unavailable sockets are ignored by this patch already. I'm mainly not sure if unavailable sockets should be ignored only in the name or also in the identifier comparison.

Brecht Van Lommel commented

2023-10-23 15:31:49 +02:00

Ignoring unavailable sockets in this lookup seems like more of breaking change to me. Could also do that though.

But isn't the plan to do effectively the same break in 4.1 anyway, by switching to dynamic declarations? As in, it would always return only the "available" socket, since the "unavailable" ones don't even exist anymore?

Generally, it still seems better if we can change the string lookup to use the identifier instead of name (using the name is causing problems with duplicate names as well, even without dynamic sockets), and doing that change in 4.0 seems better than in 4.1.

I could add code so that unavailable sockets are ignored by this patch already. I'm mainly not sure if unavailable sockets should be ignored only in the name or also in the identifier comparison.

Will socket.identifier in 4.1 still include the type? Or will it be equal to the name most of the time?

I agree it would make sense to support identifiers to distinguish between identically name sockets. But that is a pre-existing problem. I'm just not sure what the benefit would be of doing it right now in 4.0, when I think the identifier will change in 4.1?

> Ignoring unavailable sockets in this lookup seems like more of breaking change to me. Could also do that though. But isn't the plan to do effectively the same break in 4.1 anyway, by switching to dynamic declarations? As in, it would always return only the "available" socket, since the "unavailable" ones don't even exist anymore? > Generally, it still seems better if we can change the string lookup to use the identifier instead of name (using the name is causing problems with duplicate names as well, even without dynamic sockets), and doing that change in 4.0 seems better than in 4.1. > > I could add code so that unavailable sockets are ignored by this patch already. I'm mainly not sure if unavailable sockets should be ignored only in the name or also in the identifier comparison. Will `socket.identifier` in 4.1 still include the type? Or will it be equal to the name most of the time? I agree it would make sense to support identifiers to distinguish between identically name sockets. But that is a pre-existing problem. I'm just not sure what the benefit would be of doing it right now in 4.0, when I think the identifier will change in 4.1?

Jacques Lucke commented

2023-10-23 15:36:52 +02:00

As in, it would always return only the "available" socket, since the "unavailable" ones don't even exist anymore?

The idea was to remove the socket, but to make node.inputs["Attribute_xxx"] still return the right socket (mentioned as third lookup step in the patch description), even if it does not have the exact name or identifier.

Will socket.identifier in 4.1 still include the type? Or will it be equal to the name most of the time?

Right, ideally it will be equal to the name in most cases where we only have dynamic socket types. With dynamic socket amounts, this part becomes a bit more complex and the identifier is more important again.
To answer your first question, no, socket.identifier will hopefully not include the type in 4.1.

I agree it would make sense to support identifiers to distinguish between identically name sockets. But that is a pre-existing problem. I'm just not sure what the benefit would be of doing it right now in 4.0, when I think the identifier will change in 4.1?

The reason for doing this in 4.0 is mainly just to break things earlier (in 4.0) instead of later (in 4.1). None of the changes in the patch are strictly necessary for 4.0, since this is indeed a problem that exists for many releases already.

> As in, it would always return only the "available" socket, since the "unavailable" ones don't even exist anymore? The idea was to remove the socket, but to make `node.inputs["Attribute_xxx"]` still return the right socket (mentioned as third lookup step in the patch description), even if it does not have the exact name or identifier. > Will socket.identifier in 4.1 still include the type? Or will it be equal to the name most of the time? Right, ideally it will be equal to the name in most cases where we only have dynamic socket types. With dynamic socket amounts, this part becomes a bit more complex and the identifier is more important again. To answer your first question, no, `socket.identifier` will hopefully not include the type in 4.1. > I agree it would make sense to support identifiers to distinguish between identically name sockets. But that is a pre-existing problem. I'm just not sure what the benefit would be of doing it right now in 4.0, when I think the identifier will change in 4.1? The reason for doing this in 4.0 is mainly just to break things earlier (in 4.0) instead of later (in 4.1). None of the changes in the patch are strictly necessary for 4.0, since this is indeed a problem that exists for many releases already.

Brecht Van Lommel commented

2023-10-23 15:56:31 +02:00

The idea was to remove the socket, but to make node.inputs["Attribute_xxx"] still return the right socket (mentioned as third lookup step in the patch description), even if it does not have the exact name or identifier.

If you would use node.inputs["A_Float"].default_value = 1.0 in 4.1, but the A socket has type vector, what would happen?

I'm not sure how this would be backwards compatible, unless there is still a float socket somewhere. But then that float socket can't be found by iterating over node.inputs, which would also be strange and breaking in some ways.

The reason for doing this in 4.0 is mainly just to break things earlier (in 4.0) instead of later (in 4.1). None of the changes in the patch are strictly necessary for 4.0, since this is indeed a problem that exists for many releases already.

For me it's reasonable to do a breaking change in 4.0. But if you just add the identifiers, you will still have to do the breaking changes for the names in 4.1 as well. And if you do the available sockets change in 4.0, there is not much reason to have the identifiers lookup as long as they contain the type.

> The idea was to remove the socket, but to make `node.inputs["Attribute_xxx"]` still return the right socket (mentioned as third lookup step in the patch description), even if it does not have the exact name or identifier. If you would use `node.inputs["A_Float"].default_value = 1.0` in 4.1, but the `A` socket has type vector, what would happen? I'm not sure how this would be backwards compatible, unless there is still a float socket somewhere. But then that float socket can't be found by iterating over `node.inputs`, which would also be strange and breaking in some ways. > The reason for doing this in 4.0 is mainly just to break things earlier (in 4.0) instead of later (in 4.1). None of the changes in the patch are strictly necessary for 4.0, since this is indeed a problem that exists for many releases already. For me it's reasonable to do a breaking change in 4.0. But if you just add the identifiers, you will still have to do the breaking changes for the names in 4.1 as well. And if you do the available sockets change in 4.0, there is not much reason to have the identifiers lookup as long as they contain the type.

Jacques Lucke commented

2023-10-23 16:06:06 +02:00

If you would use node.inputs["A_Float"].default_value = 1.0 in 4.1, but the A socket has type vector, what would happen?

That would not work unless you set the type of the node first. I'm not aware of a solution to this that does not involve some kind of breakage in the API.

Your idea with skipping unavailable sockets in node.inputs[...] and node.outputs[...] seems like a good enough solution indeed. I think we can go for it, but would still be good to allow looking up (available) sockets by their identifier in general. We could add a special case so that the identifier lookup does not work with the nodes with dynamic sockets type that we intend to change soonish.

> If you would use `node.inputs["A_Float"].default_value = 1.0` in 4.1, but the A socket has type vector, what would happen? That would not work unless you set the type of the node first. I'm not aware of a solution to this that does not involve some kind of breakage in the API. Your idea with skipping unavailable sockets in `node.inputs[...]` and `node.outputs[...]` seems like a good enough solution indeed. I think we can go for it, but would still be good to allow looking up (available) sockets by their identifier in general. We could add a special case so that the identifier lookup does not work with the nodes with dynamic sockets type that we intend to change soonish.

Brecht Van Lommel commented

2023-10-23 16:21:12 +02:00

Your idea with skipping unavailable sockets in node.inputs[...] and node.outputs[...] seems like a good enough solution indeed. I think we can go for it, but would still be good to allow looking up (available) sockets by their identifier in general. We could add a special case so that the identifier lookup does not work with the nodes with dynamic sockets type that we intend to change soonish.

I think making identifier lookups work for those sockets that we don't intend to change makes sense. I would just avoid it for ones like node.inputs["A_Float"] because that code can break in 4.1.

> Your idea with skipping unavailable sockets in `node.inputs[...]` and `node.outputs[...]` seems like a good enough solution indeed. I think we can go for it, but would still be good to allow looking up (available) sockets by their identifier in general. We could add a special case so that the identifier lookup does not work with the nodes with dynamic sockets type that we intend to change soonish. I think making identifier lookups work for those sockets that we don't intend to change makes sense. I would just avoid it for ones like `node.inputs["A_Float"]` because that code can break in 4.1.

Jacques Lucke added 2 commits 2023-10-23 22:15:41 +02:00

Merge branch 'blender-v4.0-release' into node-sockets-string-lookup 4b8f94d89e

skip unavailable sockets and disallow identifier lookup in some nodes b807c058a0

Jacques Lucke commented

2023-10-23 22:21:35 +02:00

I updated the socket lookup function so that it ignores unavailable sockets and does not check identifiers in a few nodes for which we want to change identifiers soonish.

Brecht Van Lommel approved these changes 2023-10-24 12:36:24 +02:00

Jacques Lucke merged commit e4ad58114b into blender-v4.0-release

2023-10-24 14:04:28 +02:00

Jacques Lucke referenced this issue from a commit

2023-10-24 14:04:28 +02:00

Nodes: support looking up node sockets by identifier and name

Jacques Lucke deleted branch node-sockets-string-lookup

2023-10-24 14:04:30 +02:00

Jacques Lucke referenced this issue from a commit

2023-11-17 16:23:46 +01:00

Geometry Nodes: use dynamic declaration for some nodes

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Nodes: support looking up node sockets by identifier and name #113984