I18n: add label declaration to node sockets so they can be shortened #113070

Merged

Bastien Montagne merged 2 commits from pioverfour/blender:dp_shorten_panel_labels into blender-v4.0-release

2023-10-12 17:51:47 +02:00

Damien Picard commented

2023-09-29 23:45:06 +02:00

Member

In !112591, nodes got the ability to group sockets into panels. The
labels for these sockets are automatically shortened if they begin
with the same text as their parent labels. For instance, "Transmission
Weight" will be shortened to just "Weight" because it is under the
"Transmission" panel.

While this is a good heuristic for English, it breaks down in
languages which do not have the same word order.

This commit adds a .short_label() callback to socket declarations so
that a shortened label can be explicitly declared.

It also adds two regexps to the translation script so that these new
fields can be extracted to the .po translation files. One extracts the
label with a translation context, the other without. Only the one
without context is currently in use.

The current automatic shortening logic is kept and will be used only
if a shortened label is not manually provided.

Fixes #112970: Node socket labels under panels are not shortened when
translated.

In !112591, nodes got the ability to group sockets into panels. The labels for these sockets are automatically shortened if they begin with the same text as their parent labels. For instance, "Transmission Weight" will be shortened to just "Weight" because it is under the "Transmission" panel. While this is a good heuristic for English, it breaks down in languages which do not have the same word order. This commit adds a `.short_label()` callback to socket declarations so that a shortened label can be explicitly declared. It also adds two regexps to the translation script so that these new fields can be extracted to the .po translation files. One extracts the label with a translation context, the other without. Only the one without context is currently in use. The current automatic shortening logic is kept and will be used only if a shortened label is not manually provided. Fixes #112970: Node socket labels under panels are not shortened when translated.

129 KiB

155 KiB

Damien Picard added the

Nodes & Physics

labels 2023-09-29 23:45:06 +02:00

Damien Picard force-pushed dp_shorten_panel_labels from 32e1cb4a42 to b472ef2ff8

2023-09-30 00:06:28 +02:00

Compare

Damien Picard force-pushed dp_shorten_panel_labels from b472ef2ff8 to 12891a68d1

2023-09-30 00:42:42 +02:00

Compare

Damien Picard requested review from Bastien Montagne 2023-10-03 17:26:13 +02:00

Bastien Montagne reviewed 2023-10-03 18:10:12 +02:00

Bastien Montagne left a comment

Owner

Change generally seems to make sense to me, although I am not happy at all with having yet another different UI string for this. And I would not use just label to name it, at least something like short_label? With proper comment about what it is exactly and why it is needed.

This also needs the Node team checking anyway.

Change generally seems to make sense to me, although I am not happy at all with having yet _another_ different UI string for this. And I would not use just `label` to name it, at least something like `short_label`? With proper comment about what it is exactly and why it is needed. This also needs the Node team checking anyway.

Bastien Montagne requested review from Lukas Tönne 2023-10-03 18:10:28 +02:00

Damien Picard commented

2023-10-03 18:17:19 +02:00

Author

Member

although I am not happy at all with having yet another different UI string for this.

I can understand that!

And I would not use just label to name it, at least something like short_label?

I considered this at first but figured that since there are so many different ways to label nodes already, might as well reuse one. Also there was some reluctance before about adding naming field to structs.
But I agree it would be cleaner, I’ll ~~change back to that~~ wait for more feedback.

> although I am not happy at all with having yet _another_ different UI string for this. I can understand that! > And I would not use just `label` to name it, at least something like `short_label`? I considered this at first but figured that since there are so many different ways to label nodes already, might as well reuse one. Also there was some reluctance before about adding naming field to structs. But I agree it would be cleaner, I’ll ~~change back to that~~ wait for more feedback.

Lukas Tönne commented

2023-10-03 18:30:41 +02:00

Member

Is a new label really necessary for this? We already have socket names (which can be duplicate) vs. socket identifiers (which are unique). It's perfectly fine to use shorter socket names and then add a longer unique identifier based on the panel. I've done this in a branch recently for a new node i'm testing: code. There are multiple similar panels and the sockets inside have the same names, but the panel name is used as a prefix for unique identifiers.

If labels are needed for some reason i'd have to agree with @mont29: This probably requires yet another string variable or it will conflict with nodes that use the bNodeSocket::label. An example would be the Separate Color node which names its sockets based on the conversion mode (code).

Design-wise i'd ask @brecht to compare this to the automatic shortening. It might be a lot of monkey work to decide which names/labels require shortening manually, and then all the translations for those.

Is a new label really necessary for this? We already have socket names (which can be duplicate) vs. socket identifiers (which are unique). It's perfectly fine to use shorter socket names and then add a longer unique identifier based on the panel. I've done this in a branch recently for a new node i'm testing: [code](https://projects.blender.org/LukasTonne/blender/src/commit/0ed1cf8247fe308d2d698234ce402205777c389d/source/blender/nodes/geometry/node_geometry_grid_debug_points.hh#L40-L67). There are multiple similar panels and the sockets inside have the same names, but the panel name is used as a prefix for unique identifiers. **If** labels are needed for some reason i'd have to agree with @mont29: This probably requires yet another string variable or it will conflict with nodes that use the `bNodeSocket::label`. An example would be the _Separate Color_ node which names its sockets based on the conversion mode ([code](https://projects.blender.org/blender/blender/src/commit/a6376ec77ae387d56abb3ec26971b8af93e31e87/source/blender/nodes/intern/node_util.cc#L242-L273)). Design-wise i'd ask @brecht to compare this to the automatic shortening. It might be a lot of monkey work to decide which names/labels require shortening manually, and then all the translations for those.

Damien Picard commented

2023-10-03 18:36:12 +02:00

Author

Member

Is a new label really necessary for this? […] It's perfectly fine to use shorter socket names and then add a longer unique identifier based on the panel.

@brecht mentioned this in !112591:

We want the properties and sockets to be identifiable outside of the
panel context, for example in the animation editor, API docs, or
when exposing a single socket as a group input. So making the actual
name shorter is not an option.

> Is a new label really necessary for this? […] It's perfectly fine to use shorter socket names and then add a longer unique identifier based on the panel. @brecht mentioned this in !112591: > We want the properties and sockets to be identifiable outside of the panel context, for example in the animation editor, API docs, or when exposing a single socket as a group input. So making the actual name shorter is not an option.

Brecht Van Lommel commented

2023-10-03 19:39:35 +02:00

Owner

I'm not a native French speaker, but I'm not sure I agree with the way the socket names were translated even before panels were added.

For example:

"Specular Tint" is a shorthand for the tint of the specular layer. But "teinte spéculaire" interprets "specular" as an adjective, but this is not "a tint that is specular".
"Subsurface Radius" appears to be translated as "radius of the subsurface". But it is the average scattering distance for photons within the subsurface medium. It is not the size of the volume below the surface.

What I'm getting at is that even in English, the meaning of this wording is really "Group: Item". And I'm wondering why we can't translate it as such in other languages as well? Without panels this also helps group together related properties.

If it does read really poorly, I would perhaps suggest to add a colon in the translation and have our shortening code recognize that.

I'm not a native French speaker, but I'm not sure I agree with the way the socket names were translated even before panels were added. For example: * "Specular Tint" is a shorthand for the tint of the specular layer. But "teinte spéculaire" interprets "specular" as an adjective, but this is not "a tint that is specular". * "Subsurface Radius" appears to be translated as "radius of the subsurface". But it is the average scattering distance for photons within the subsurface medium. It is not the size of the volume below the surface. What I'm getting at is that even in English, the meaning of this wording is really "Group: Item". And I'm wondering why we can't translate it as such in other languages as well? Without panels this also helps group together related properties. If it does read really poorly, I would perhaps suggest to add a colon in the translation and have our shortening code recognize that.

Damien Picard commented

2023-10-03 20:31:00 +02:00

Author

Member

"Specular Tint" is a shorthand for the tint of the specular layer. But "teinte spéculaire" interprets "specular" as an adjective, but this is not "a tint that is specular".

You’re technically right but to me it’s also a shorthand for “Teinte [de la] spéculaire”, “Tint [of the] specular”. TBH I’d rather use the full “Teinte de la spéculaire” but the translation of this area is in progress and hasn’t been reviewed in regard to the latest Principled changes.

"Subsurface Radius" appears to be translated as "radius of the subsurface". But it is the average scattering distance for photons within the subsurface medium. It is not the size of the volume below the surface.

Thank you, I’ll try to come up with a better translation for this one!

What I'm getting at is that even in English, the meaning of this wording is really "Group: Item". And I'm wondering why we can't translate it as such in other languages as well? Without panels this also helps group together related properties.

Firstly I really believe that outside the context of the node, reversing the word order for some labels would sound very strange in French. “Vernis Rugosité” (for Coat Roughness) is unclear but it kind of sounds like it means “A coat of roughness”.
Secondly, French is still fairly similar to English so it could maybe work, but many other languages are not. Consider the example for simplified Chinese provided by @ChengduLittleA, where if I understand correctly the shortened string appears in the middle of the full string.
Some other languages may have a grammar that is even further away from English, this is not something we can anticipate.

If it does read really poorly, I would perhaps suggest to add a colon in the translation and have our shortening code recognize that.

That’s a great idea, but the issue I see with it is that this must be communicated to the translators, who won’t look at the code to know what it does. They won’t know they need to change the syntax simply based on the message they’re presented with.
We could manually add a comment in the PO files to let them know, but this kind of knowledge is easily lost. Developers in turn won’t know they need to do that when a new field is added.

> * "Specular Tint" is a shorthand for the tint of the specular layer. But "teinte spéculaire" interprets "specular" as an adjective, but this is not "a tint that is specular". You’re technically right but to me it’s also a shorthand for “_Teinte [de la] spéculaire_”, “Tint [of the] specular”. TBH I’d rather use the full “_Teinte de la spéculaire_” but the translation of this area is in progress and hasn’t been reviewed in regard to the latest Principled changes. > * "Subsurface Radius" appears to be translated as "radius of the subsurface". But it is the average scattering distance for photons within the subsurface medium. It is not the size of the volume below the surface. Thank you, I’ll try to come up with a better translation for this one! > What I'm getting at is that even in English, the meaning of this wording is really "Group: Item". And I'm wondering why we can't translate it as such in other languages as well? Without panels this also helps group together related properties. Firstly I really believe that outside the context of the node, reversing the word order for some labels would sound very strange in French. “_Vernis Rugosité_” (for Coat Roughness) is unclear but it kind of sounds like it means “A coat of roughness”. Secondly, French is still fairly similar to English so it could maybe work, but many other languages are not. Consider [the example for simplified Chinese](https://projects.blender.org/blender/blender/issues/112970#issuecomment-1033062) provided by @ChengduLittleA, where if I understand correctly the shortened string appears in the middle of the full string. Some other languages may have a grammar that is even further away from English, this is not something we can anticipate. > If it does read really poorly, I would perhaps suggest to add a colon in the translation and have our shortening code recognize that. That’s a great idea, but the issue I see with it is that this must be communicated to the translators, who won’t look at the code to know what it does. They won’t know they need to change the syntax simply based on the message they’re presented with. We could manually add a comment in the PO files to let them know, but this kind of knowledge is easily lost. Developers in turn won’t know they need to do that when a new field is added.

Brecht Van Lommel requested changes 2023-10-04 17:24:21 +02:00

Brecht Van Lommel left a comment

Owner

I guess an additional short label is the only reliable solution then. It's not really different than the strings coming from UI layout files.

But it should be called short_label.

This then also raises the question of what to do with user created node groups. Simplest would be to keep the automatic shortening for them.

I guess an additional short label is the only reliable solution then. It's not really different than the strings coming from UI layout files. But it should be called `short_label`. This then also raises the question of what to do with user created node groups. Simplest would be to keep the automatic shortening for them.

source/blender/blenkernel/intern/node.cc

						
				@ -4024,3 +4024,3 @@

				const char *nodeSocketLabel(const bNodeSocket *sock)

				{

				  return (sock->label[0] != '\0') ? sock->label : sock->name;

				  /* Get the declaration label if possible. This is used when grouping sockets under panels, to

Brecht Van Lommel commented

2023-10-04 16:41:08 +02:00

Owner

Putting the code here will shorten the label in the tooltip as well, which I don't think is right.

Putting the code here will shorten the label in the tooltip as well, which I don't think is right.

Damien Picard commented

2023-10-04 19:12:01 +02:00

Author

Member

I think I fixed it but I couldn’t figure out in which cases this happens.

I think I fixed it but I couldn’t figure out in which cases this happens.

brecht marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -408,4 +408,1 @@

				  const char *socket_translation_context = node_socket_get_translation_context(*socket);

				  const char *translated_socket_label = CTX_IFACE_(socket_translation_context, socket_label);

				  /* Shorten socket label if it begins with the panel label. */

Brecht Van Lommel commented

2023-10-04 16:41:57 +02:00

Owner

Note that removing this code removes the shortening functionality for user created group nodes.

Note that removing this code removes the shortening functionality for user created group nodes.

pioverfour marked this conversation as resolved

Damien Picard added 1 commit 2023-10-04 19:11:22 +02:00

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

c000bb7105 Address review

- Rename `label` to `short_label`.
- Restore automatic shortening.
- Do not translate from `bke::nodeSocketLabel`.

- Add translation context to "Tint" sockets.

Brecht Van Lommel approved these changes 2023-10-09 18:58:30 +02:00

Brecht Van Lommel left a comment

Owner

This looks good to me now.

This looks good to me now.

Brecht Van Lommel commented

2023-10-12 13:33:05 +02:00

Owner

@mont29 @LukasTonne any feedback on this? It would be good to get this into 4.0 as quickly as possible so translations can be added still.

@mont29 @LukasTonne any feedback on this? It would be good to get this into 4.0 as quickly as possible so translations can be added still.

Lukas Tönne approved these changes 2023-10-12 13:48:01 +02:00

Lukas Tönne left a comment

Member

This looks good to me now.

This looks good to me now.

Bastien Montagne approved these changes 2023-10-12 17:07:04 +02:00

Bastien Montagne left a comment

Owner

LGTM as well now.

LGTM as well now.

Bastien Montagne commented

2023-10-12 17:07:17 +02:00

Owner

@blender-bot build

@blender-bot build

Bastien Montagne merged commit 2d703e9200 into blender-v4.0-release

2023-10-12 17:51:47 +02:00

Bastien Montagne referenced this issue from a commit

2023-10-12 17:51:47 +02:00

I18n: add label declaration to node sockets so they can be shortened

Bastien Montagne deleted branch dp_shorten_panel_labels

2023-10-12 17:51:52 +02:00

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#113070

No description provided.