Added Label3D auto localization #87599

DeltaW0x · 2024-01-26T00:14:28Z

Added support for automatic key-matched string localization directly from the inspector, like present in Label (2D)

akien-mga · 2024-01-26T00:22:07Z

Thanks for the contribution!

Did you test whether the changes work? I might be missing something, but the code doesn't look like it will actually do anything to enable translations. It's just the interface for it copied over from control.h, but there's no code in label_3d.cpp that will actually make use of the added properties to trigger a translation from what I can see.

DeltaW0x · 2024-01-26T00:23:46Z

The code was tested and it works as it should, as far as I know.I have built it and both the matching and the ability to change language at runtime work, the label updates . Sorry but this is my first pull request ever. The code from control.h to handle the language was copied too inside label_3d.cpp

akien-mga · 2024-01-26T00:27:31Z

Are you sure you committed all your local changes? In label_3d.cpp, the values for auto_translate can set and read, but it's never used by any internal API (again, unless I'm missing something, which may be possible).

I would expect that it would need to be handled in NOTIFICATION_TRANSLATION_CHANGED in _notification to actually decide whether to translate or not, using the atr() helper you also backported (but which is also never used in the current PR).

DeltaW0x · 2024-01-26T00:42:02Z

I'm sorry, maybe I'm misunderstanding something, the code that handles the translation was copied directly from control.h, which is how the Label node can handle the automatic key-matching directly from the Label inspector text field, without tr(). It is exaclty the same, just move from control to Label3D. There is a nicer way to do this without copying a bunch of code probably, but this seems to work fine for now, better than having nothing or to have to translate the strings from script using tr().
The translation changed notification is pushed like is pushed in control.h, minus the data struct

Label3D::set_auto_translate(bool p_enable) {
	ERR_MAIN_THREAD_GUARD;
	if (p_enable == auto_translate) {
		return;
	}

	auto_translate = p_enable;

	notification(MainLoop::NOTIFICATION_TRANSLATION_CHANGED);
}

akien-mga · 2024-01-26T00:53:25Z

Well the code in label.cpp works because of this part:

void Label::_notification(int p_what) {
	switch (p_what) {
		case NOTIFICATION_TRANSLATION_CHANGED: {
			String new_text = atr(text);
			if (new_text == xl_text) {
				return; // Nothing new.
			}
			xl_text = new_text;
			if (visible_ratio < 1) {
				visible_chars = get_total_character_count() * visible_ratio;
			}
			dirty = true;

			queue_redraw();
			update_configuration_warnings();
		} break;

So I'm really puzzled it would work in label_3d.cpp without similar code. But I guess I should test the PR to see :D

akien-mga · 2024-01-26T00:54:44Z

Ah I see label_3d.cpp does have this code already:

		case NOTIFICATION_TRANSLATION_CHANGED: {
			String new_text = tr(text);
			if (new_text == xl_text) {
				return; // Nothing new.
			}
			xl_text = new_text;
			dirty_text = true;
			_queue_update();
		} break;

So indeed what was missing was just the code to trigger this notification.

Sorry for the noise :)

DeltaW0x · 2024-01-26T00:57:16Z

Oh please don't worry it's ok, I hadn't seen it either, it just worked so i thought about pushing the repo '^^, I'm just inexperienced so I thought I had missed something

akien-mga

So, now I understand better why it seems to work, and my initial confusion.

The key thing which makes auto translation work is the atr() method, which handles checking whether auto translation is enabled or not. You added the implementation in label_3d.h, but you're not making use of it in label_3d.cpp.

You need to change this for it to work like in label.cpp:

diff --git a/scene/3d/label_3d.cpp b/scene/3d/label_3d.cpp
index 3fb472335e..ff8bd32866 100644
--- a/scene/3d/label_3d.cpp
+++ b/scene/3d/label_3d.cpp
@@ -208,7 +208,7 @@ void Label3D::_notification(int p_what) {
 			viewport->disconnect("size_changed", callable_mp(this, &Label3D::_font_changed));
 		} break;
 		case NOTIFICATION_TRANSLATION_CHANGED: {
-			String new_text = tr(text);
+			String new_text = atr(text);
 			if (new_text == xl_text) {
 				return; // Nothing new.
 			}
@@ -635,8 +635,11 @@ void Label3D::_shape() {
 }
 
 void Label3D::set_text(const String &p_string) {
+	if (text == p_string) {
+		return;
+	}
 	text = p_string;
-	xl_text = tr(p_string);
+	xl_text = atr(p_string);
 	dirty_text = true;
 	_queue_update();
 }

If you test the current PR, turning off auto translation shouldn't work, it will keep translating.

scene/3d/label_3d.cpp

scene/3d/label_3d.h

DeltaW0x · 2024-01-26T01:36:12Z

Fixed the issues you pointed out and removed the unnecessary methods. Now the automatic translation indeed stops when it's disabled

akien-mga · 2024-01-26T01:37:33Z

Looks good! Could you squash the commits? See PR workflow for instructions.

Then from my perspective the PR looks ready. I'll still aim to actually test it tomorrow but the code makes sense to me.

CC @YeldhamDev who's our auto translation expert now ;)

akien-mga · 2024-01-26T01:39:19Z

Ah one more thing actually, you need to add documentation for the new property:

diff --git a/doc/classes/Label3D.xml b/doc/classes/Label3D.xml
index 977fb00402..c4c9c58543 100644
--- a/doc/classes/Label3D.xml
+++ b/doc/classes/Label3D.xml
@@ -48,6 +48,10 @@
 		<member name="alpha_scissor_threshold" type="float" setter="set_alpha_scissor_threshold" getter="get_alpha_scissor_threshold" default="0.5">
 			Threshold at which the alpha scissor will discard values.
 		</member>
+                <member name="auto_translate" type="bool" setter="set_auto_translate" getter="is_auto_translating" default="true">
+                        Toggles if any text should automatically change to its translated version depending on the current locale.
+                        Also decides if the node's strings should be parsed for POT generation.
+                </member>
 		<member name="autowrap_mode" type="int" setter="set_autowrap_mode" getter="get_autowrap_mode" enum="TextServer.AutowrapMode" default="0">
 			If set to something other than [constant TextServer.AUTOWRAP_OFF], the text gets wrapped inside the node's bounding rectangle. If you resize the node, it will change its height automatically to show all the text. To see how each mode behaves, see [enum TextServer.AutowrapMode].
 		</member>

Not sure if the part about POT generation is accurate (I copied the description from Control), Yeldham can maybe clarify.

akien-mga · 2024-01-26T01:47:37Z

scene/3d/label_3d.h

@@ -133,6 +133,9 @@ class Label3D : public GeometryInstance3D {
 	TextServer::StructuredTextParser st_parser = TextServer::STRUCTURED_TEXT_DEFAULT;
 	Array st_args;

+	bool auto_translate = true;
+	bool localize_numeral_system = true;


I missed this one, should be removed too.

You mean only localize_numeral_system right? isn't auto_translate is necessary?

Yes localize_numeral_system is now unused and should be removed. The other one is needed.

YeldhamDev · 2024-01-26T01:57:26Z

This PR kinda came in a bad time right now, since I'm re-doing how auto translation works. To make matters worse, having yet another type of node requiring it makes the system even more convoluted, since both Control and Window already share a lot of duplicate code.

DeltaW0x · 2024-01-26T02:01:49Z

Of course, my perfect timing as always. Anyways, i've squashed the commits. It should be good for merging but at this point it's kinda useless i think, if there's a new system in development

akien-mga

I'll approve from my side I think the implementation is straightforward and correct. Great work!

But before merging, let's see first how @YeldhamDev's refactor in #87530 goes, and how to include Label3D one way or another.

doc/classes/Label3D.xml

bruvzg · 2024-01-26T05:58:09Z

The same change can be done to the TextMesh as well.

DeltaW0x · 2024-01-26T16:56:03Z

If there's more than one node that needs autotranslation maybe it's just better to wait for the general refractor, I don't think it's a good idea to have the same exact code just copied and pasted in multiple spaces

InfernalWAVE · 2024-02-03T02:03:10Z

This is great! Any idea what version we could expect to see this implemented?

Mickeon · 2024-02-03T12:34:19Z

The implementation depends on #87530 (TL:DR Every Node having auto_translate_mode). Once that's merged this PR should be rebased as it would basically come "for free", without code duplication.

Mickeon · 2024-02-17T15:49:05Z

#87530 has been merged. This is the go. Time to rebase this to use the new auto_translate_mode.

Mickeon · 2024-02-18T15:03:33Z

You did not have to close this, but I have to assume you will open another PR from scratch, or have lost interest implementing this yourself?

DeltaW0x · 2024-02-18T15:05:29Z

Wait, I thought there's no need for this with the new localization system?

akien-mga · 2024-02-18T15:20:18Z

I believe the changes to Label3D::set_text() would still be needed to actually switch to using atr() instead of tr() (and likewise for TextMesh).

akien-mga · 2024-03-01T14:34:33Z

I believe the changes to Label3D::set_text() would still be needed to actually switch to using atr() instead of tr() (and likewise for TextMesh).

A task for you @YeldhamDev ;)

YeldhamDev · 2024-03-01T17:06:30Z

Done: #89056

DeltaW0x requested a review from a team as a code owner January 26, 2024 00:14

akien-mga added enhancement topic:gui topic:3d and removed topic:gui labels Jan 26, 2024

akien-mga added this to the 4.x milestone Jan 26, 2024

akien-mga reviewed Jan 26, 2024

View reviewed changes

scene/3d/label_3d.cpp Outdated Show resolved Hide resolved

scene/3d/label_3d.cpp Outdated Show resolved Hide resolved

scene/3d/label_3d.h Outdated Show resolved Hide resolved

DeltaW0x force-pushed the master branch from 1ea5ce2 to 0d497bb Compare January 26, 2024 01:44

akien-mga reviewed Jan 26, 2024

View reviewed changes

DeltaW0x requested a review from a team as a code owner January 26, 2024 01:48

DeltaW0x force-pushed the master branch from 2502ebc to 24efa5d Compare January 26, 2024 01:49

DeltaW0x force-pushed the master branch from 28d6ddb to 283fff8 Compare January 26, 2024 01:58

akien-mga approved these changes Jan 26, 2024

View reviewed changes

bruvzg approved these changes Jan 26, 2024

View reviewed changes

doc/classes/Label3D.xml Outdated Show resolved Hide resolved

DeltaW0x force-pushed the master branch from 2a5deb0 to 791c729 Compare January 26, 2024 19:24

Added Label3D auto localization

c3a72a0

DeltaW0x force-pushed the master branch from 232b25a to c3a72a0 Compare January 26, 2024 19:27

DeltaW0x closed this Feb 18, 2024

Mickeon added the archived label Feb 18, 2024

Mickeon removed this from the 4.x milestone Feb 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Label3D auto localization #87599

Added Label3D auto localization #87599

DeltaW0x commented Jan 26, 2024 •

edited

Loading

akien-mga commented Jan 26, 2024

DeltaW0x commented Jan 26, 2024 •

edited

Loading

akien-mga commented Jan 26, 2024 •

edited

Loading

DeltaW0x commented Jan 26, 2024 •

edited

Loading

akien-mga commented Jan 26, 2024

akien-mga commented Jan 26, 2024 •

edited

Loading

DeltaW0x commented Jan 26, 2024

akien-mga left a comment •

edited

Loading

DeltaW0x commented Jan 26, 2024 •

edited

Loading

akien-mga commented Jan 26, 2024

akien-mga commented Jan 26, 2024

akien-mga Jan 26, 2024

DeltaW0x Jan 26, 2024 •

edited

Loading

akien-mga Jan 26, 2024

YeldhamDev commented Jan 26, 2024

DeltaW0x commented Jan 26, 2024

akien-mga left a comment •

edited

Loading

bruvzg commented Jan 26, 2024

DeltaW0x commented Jan 26, 2024

InfernalWAVE commented Feb 3, 2024

Mickeon commented Feb 3, 2024

Mickeon commented Feb 17, 2024

Mickeon commented Feb 18, 2024

DeltaW0x commented Feb 18, 2024

akien-mga commented Feb 18, 2024

akien-mga commented Mar 1, 2024

YeldhamDev commented Mar 1, 2024

Added Label3D auto localization #87599

Added Label3D auto localization #87599

Conversation

DeltaW0x commented Jan 26, 2024 • edited Loading

akien-mga commented Jan 26, 2024

DeltaW0x commented Jan 26, 2024 • edited Loading

akien-mga commented Jan 26, 2024 • edited Loading

DeltaW0x commented Jan 26, 2024 • edited Loading

akien-mga commented Jan 26, 2024

akien-mga commented Jan 26, 2024 • edited Loading

DeltaW0x commented Jan 26, 2024

akien-mga left a comment • edited Loading

Choose a reason for hiding this comment

DeltaW0x commented Jan 26, 2024 • edited Loading

akien-mga commented Jan 26, 2024

akien-mga commented Jan 26, 2024

akien-mga Jan 26, 2024

Choose a reason for hiding this comment

DeltaW0x Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

akien-mga Jan 26, 2024

Choose a reason for hiding this comment

YeldhamDev commented Jan 26, 2024

DeltaW0x commented Jan 26, 2024

akien-mga left a comment • edited Loading

Choose a reason for hiding this comment

bruvzg commented Jan 26, 2024

DeltaW0x commented Jan 26, 2024

InfernalWAVE commented Feb 3, 2024

Mickeon commented Feb 3, 2024

Mickeon commented Feb 17, 2024

Mickeon commented Feb 18, 2024

DeltaW0x commented Feb 18, 2024

akien-mga commented Feb 18, 2024

akien-mga commented Mar 1, 2024

YeldhamDev commented Mar 1, 2024

DeltaW0x commented Jan 26, 2024 •

edited

Loading

DeltaW0x commented Jan 26, 2024 •

edited

Loading

akien-mga commented Jan 26, 2024 •

edited

Loading

DeltaW0x commented Jan 26, 2024 •

edited

Loading

akien-mga commented Jan 26, 2024 •

edited

Loading

akien-mga left a comment •

edited

Loading

DeltaW0x commented Jan 26, 2024 •

edited

Loading

DeltaW0x Jan 26, 2024 •

edited

Loading

akien-mga left a comment •

edited

Loading