issue_360 Fix dialog_caption size #361

balloonpopper · 2024-11-29T13:17:58Z

Fixes #360

mapedorr

I'm asking for a couple of changes. I still have to devote some time to test the behavior of the component.

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_text.gd

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_caption/dialog_caption.gd

…og_caption/dialog_caption.gd Co-authored-by: Carenalga <[email protected]>

…tion ends up wrong

…serted between lines

balloonpopper · 2024-12-05T11:39:34Z

The changes to the _calculate_size function were because the existing code would split the text from this command onto 2 lines for a 100px text box, when the text was only about 60px wide when tested with (from memory) dialog_overhead.

await C.get_OfficerBlonde().say("It's free.")

mapedorr

I request some changes before merging this to develop.

mapedorr · 2024-12-28T00:39:09Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_caption/dialog_caption.gd

+	rich_text_label.size.x = _size.x
 	rich_text_label.size.y = _size.y
+	rich_text_label.position.x = (get_viewport_rect().size.x/2) - (_size.x /2)


I think the ideal approach in this case is not to modify the width (size.x) and X position of the RichTextLabel. Considering how this Control is configured in Godot, what we should do is ensure that the value defined in the wrap_width property is assigned to the size.x property of the RichTextLabel in the _ready() function. To control the Y position of the component, Godot's anchors can be used.

mapedorr · 2024-12-28T00:41:10Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_caption/dialog_caption.gd

+func _correct_line_breaks(msg: String) -> String:
+	rich_text_label.text = msg
+	var number_of_lines_of_text := rich_text_label.get_line_count()
+	if number_of_lines_of_text > 1:
+		var current_line_number := 0
+		for current_character in range(0, rich_text_label.text.length()):
+			if rich_text_label.get_character_line(current_character) > current_line_number:
+				current_line_number += 1
+				if rich_text_label.text[current_character-1] == " ":
+					rich_text_label.text[current_character-1] = "\n"
+				elif rich_text_label.text[current_character-1] != "\n":
+					rich_text_label.text = rich_text_label.text.left(current_character) +\
+					"\n" + rich_text_label.text.right(-current_character)
+
+				if current_line_number == number_of_lines_of_text - 1:
+					break
+	return rich_text_label.text


I like what this code does, but it shouldn't be duplicated in the 3 DialogText components. You should move this to the dialog_text.gd script so any DialogText component can make use of it.

mapedorr · 2024-12-28T00:41:49Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_overhead/dialog_overhead.gd

+## Appends text for the dialog caption
+## Ensures that where a printing a word would see it wrap to the next line that the newline
+## is forced into the text. Without this the tween in dialog_text.gd would print part of the word
+## until it runs out of space, then erase the part word and rewrite it on the next line which looks
+## messy.
+func _correct_line_breaks(msg: String) -> String:
+	rich_text_label.text = msg
+	var number_of_lines_of_text := rich_text_label.get_line_count()
+	if number_of_lines_of_text > 1:
+		var current_line_number := 0
+		for current_character in range(0, rich_text_label.text.length()):
+			if rich_text_label.get_character_line(current_character) > current_line_number:
+				current_line_number += 1
+				if rich_text_label.text[current_character-1] == " ":
+					rich_text_label.text[current_character-1] = "\n"
+				elif rich_text_label.text[current_character-1] != "\n":
+					rich_text_label.text = rich_text_label.text.left(current_character) + "\n" +\
+					rich_text_label.text.right(-current_character)
+
+				if current_line_number == number_of_lines_of_text - 1:
+					break
+	return rich_text_label.text
+
+


Remove this after moving this function to the dialog_text.gd script.

mapedorr · 2024-12-28T00:45:00Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_overhead/dialog_overhead.gd

+	msg = _correct_line_breaks(msg)
+


Assign the [color][/color] to the msg in the same way you did for the dialog_caption.gd:17 and the dialog_portrait.gd:47.

mapedorr · 2024-12-28T00:45:22Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_portrait/dialog_portrait.gd

+## Appends text for the dialog caption
+## Ensures that where a printing a word would see it wrap to the next line that the newline
+## is forced into the text. Without this the tween in dialog_text.gd would print part of the word
+## until it runs out of space, then erase the part word and rewrite it on the next line which looks
+## messy.
+func _correct_line_breaks(msg: String) -> String:
+	rich_text_label.text = msg
+	var number_of_lines_of_text := rich_text_label.get_line_count()
+	if number_of_lines_of_text > 1:
+		var current_line_number := 0
+		for current_character in range(0, rich_text_label.text.length()):
+			if rich_text_label.get_character_line(current_character) > current_line_number:
+				current_line_number += 1
+				if rich_text_label.text[current_character-1] == " ":
+					rich_text_label.text[current_character-1] = "\n"
+				elif rich_text_label.text[current_character-1] != "\n":
+					rich_text_label.text = rich_text_label.text.left(current_character) + "\n" +\
+					rich_text_label.text.right(-current_character)
+
+				if current_line_number == number_of_lines_of_text - 1:
+					break
+	return rich_text_label.text
+


Remove this after moving this function to the dialog_text.gd script.

mapedorr · 2024-12-28T00:47:29Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_text.gd

 	# Call the virtual method that modifies the size of the RichTextLabel in case the dialog style
 	# requires it.
 	await _modify_size(msg, props.position)

+	rich_text_label.push_color(props.color)


We removed this in #357 because it was causing issues when centering text. Please remove it again since each DialogText component should assign the color itself using BBCode ([color][/color]).

mapedorr · 2024-12-28T00:48:31Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_text.gd

+	if PopochiuConfig.should_talk_gibberish():
+		_append_text(D.create_gibberish(msg), props)
+	else:
+		_append_text(msg, props)


This should be just this since the gibberish text replacement was already done in line 75.

Suggested change

if PopochiuConfig.should_talk_gibberish():

_append_text(D.create_gibberish(msg), props)

else:

_append_text(msg, props)

_append_text(msg, props)

mapedorr · 2024-12-28T00:50:00Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_text.gd

 	lbl.text = rt.get_parsed_text()
+	lbl.size = lbl.get_minimum_size()


I think this is not necessary since the custom_minimum_size of the component at this point will be Vector2.ZERO.

mapedorr · 2024-12-28T00:53:26Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_text.gd

 ## Creates a RichTextLabel to calculate the resulting size of this node once the whole text is shown.
+## Uses a RichTextLabel to provide the "get_parsed_text" function, and a label within it to work out
+## the minimum size. Calculating the size from just the RichTextLabel does not work.


If you want to replace the description of the function, please remove the previous one. Also, I suggest a small change to make it clear what the function really does.

Suggested change

## Creates a RichTextLabel to calculate the resulting size of this node once the whole text is shown.

## Uses a RichTextLabel to provide the "get_parsed_text" function, and a label within it to work out

## the minimum size. Calculating the size from just the RichTextLabel does not work.

## Uses a [RichTextLabel] and a [Label] to work out the minimum size since calculating it from just

## the [RichTextLabel] does not work.

mapedorr · 2024-12-28T01:37:36Z

I think the _correct_line_breaks() function you created to modify the text so the line breaks are displayed properly works fine, and it actually improves the behavior of the tween, but I suggest making that calculation before trying to calculate the size of each DialogText component.

About the Overhead dialog

The idea of this component is to take into account the dialog_pos property of each PopochiuCharacter to make the bottom of the component to render above that line. In many cases, your implementation makes the text to render below that line, which can cause reading issues on long texts.

👇 Your implementation 👇

dialog_overhead_wrong_behavior.mp4

👇 The current (an expected) behavior 👇

dialog_overhead_expected_behavior.mp4

About the Portrait dialog

Due to how this scene is structured, the size of the RichTextLabel adjusts to the available space within the PanelContainer component where it is contained. Perhaps what should happen with this component is that the value of the wrap_width property defines the size that the PanelContainer should have. In any case, modifying the text using your _correct_line_breaks() function can help the make the tween look better.

About the Caption dialog

As I mentioned in one of the comments, I think the ideal approach for this component is that the value of the wrap_width property defines the X size of the RichTextLabel component on its _ready() function. The width of this component will remain fixed regardless of the amount of content to render. I think modifying only the height based on the content is fine (as it is in the current implementation (in develop)), but we could use the _correct_line_breaks() function to ensure the final size adjusts better based on the final text content.

To summarize

I think the right approach to make use of the code you made for calculating the line breaks should be used before calling _modify_size() (line 79 in your code) in the DialogText class. That way, the calculation of the size of the RichTextLabel component will use the text with the line breaks fixes.

mapedorr · 2024-12-28T01:38:25Z

addons/popochiu/engine/objects/gui/components/dialog_text/dialog_caption/dialog_caption.gd

+## is forced into the text. Without this the tween in dialog_text.gd would print part of the word
+## until it runs out of space, then erase the part word and rewrite it on the next line which looks
+## messy.
+func _correct_line_breaks(msg: String) -> String:


I think _fix_line_breaks is a better name for this function. "Correct" sounds weird.

issue_360 Fix dialog_caption size

3bc3b6a

mapedorr requested changes Nov 29, 2024

View reviewed changes

balloonpopper and others added 6 commits November 30, 2024 10:28

Update addons/popochiu/engine/objects/gui/components/dialog_text/dial…

44ecc0f

…og_caption/dialog_caption.gd Co-authored-by: Carenalga <[email protected]>

Update addons/popochiu/engine/objects/gui/components/dialog_text/dial…

62a0acd

…og_caption/dialog_caption.gd Co-authored-by: Carenalga <[email protected]>

issue_360 Fix bug where short text ends up on 2 lines and text y loca…

e3f6756

…tion ends up wrong

issue_360 overhead dialog now positioned correctly and line breaks in…

85366a1

…serted between lines

issue_360 Added line splitting function to portrait dialog mode

25f6e53

issue_360 Fix long lines

bf1d6f5

balloonpopper force-pushed the issue_360 branch from 8170932 to bf1d6f5 Compare December 5, 2024 11:36

mapedorr requested changes Dec 28, 2024

View reviewed changes

mapedorr reviewed Dec 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue_360 Fix dialog_caption size #361

issue_360 Fix dialog_caption size #361

balloonpopper commented Nov 29, 2024

mapedorr left a comment

balloonpopper commented Dec 5, 2024

mapedorr left a comment

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr Dec 28, 2024

mapedorr commented Dec 28, 2024

mapedorr Dec 28, 2024

		lbl.text = rt.get_parsed_text()
		lbl.size = lbl.get_minimum_size()

issue_360 Fix dialog_caption size #361

Are you sure you want to change the base?

issue_360 Fix dialog_caption size #361

Conversation

balloonpopper commented Nov 29, 2024

mapedorr left a comment

Choose a reason for hiding this comment

balloonpopper commented Dec 5, 2024

mapedorr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapedorr commented Dec 28, 2024

About the Overhead dialog

About the Portrait dialog

About the Caption dialog

To summarize

Choose a reason for hiding this comment