Button text does not respect Space setting of DynamicFont tres

[ErwinBr]

Godot version:
3.2.1 Stable

OS/device including version:
Mac OS Catalina 10.15.2

Issue description:
It seems that text on Buttons do not respect the Space setting of my DynamicFont. Screenshot shows my button (top) and label (bottom). The label is correct.

The space is set to -20 on the TRES file. I'm 100% sure I'm using the same TRES for Button and Label text, because I only have one. Other properties, such as Size and Outline Size are working fine.

Steps to reproduce:

1. Create a Button and a Label.
2. Create a DynamicFont and change the Spacing to -20
3. Assign the same DynamicFont file to Button and Label
4. Observe the difference

[follower]

---

TL;DR:

Somewhat ironically, the reason Label displays the text correctly appears to be because Label doesn't draw spaces. :D

And so coincidentally Label avoids the issue that DynamicFont's draw_char() implementation does not account for the extra space character spacing.

But Button is not so lucky...

---

Character width from draw_char() vs get_char_size()

Because Label doesn't draw spaces it never uses the width value returned for the space character from draw_char(), instead it uses the width value returned from get_char_size().

Related Code

Label stores the space width value in space_w here:

godot/scene/gui/label.cpp

Line 114 in 058a0af

real_t space_w = font->get_char_size(' ').width;

And later uses it in a number of places, the key one being here where it modifies the x offset of the next rendered word:

godot/scene/gui/label.cpp

Lines 234 to 241 in 058a0af

	if (from->space_count) {
	/* spacing */
	x_ofs += space_w * from->space_count;
	if (can_fill && align == ALIGN_FILL && spaces) {
	x_ofs += int((size.width - (taken + space_w * spaces)) / spaces);
	}
	}

The word splitting/space character removal appears around here:

godot/scene/gui/label.cpp

Lines 419 to 458 in 058a0af

	bool separatable = (current >= 0x2E08 && current <= 0xFAFF) || (current >= 0xFE30 && current <= 0xFE4F);
	//current>=33 && (current < 65||current >90) && (current<97||current>122) && (current<48||current>57);
	bool insert_newline = false;
	real_t char_width = 0;
	if (current < 33) {
	if (current_word_size > 0) {
	WordCache *wc = memnew(WordCache);
	if (word_cache) {
	last->next = wc;
	} else {
	word_cache = wc;
	}
	last = wc;
	wc->pixel_width = current_word_size;
	wc->char_pos = word_pos;
	wc->word_len = i - word_pos;
	wc->space_count = space_count;
	current_word_size = 0;
	space_count = 0;
	}
	if (current == '\n') {
	insert_newline = true;
	} else if (current != ' ') {
	total_char_cache++;
	}
	if (i < xl_text.length() && xl_text[i] == ' ') {
	if (line_width > 0 || last == nullptr || last->char_pos != WordCache::CHAR_WRAPLINE) {
	space_count++;
	line_width += space_width;
	} else {
	space_count = 0;
	}
	}
	} else {

And while DynamicFont's get_char_size() implementation does account for the extra space character spacing, its draw_char() implementation does not account for the extra space character spacing.

Related Code

DynamicFont's get_char_size() accounts for extra space character spacing here:

godot/scene/resources/dynamic_font.cpp

Lines 858 to 859 in d7b85fb

	if (p_char == ' ')
	ret.width += spacing_space + spacing_char;

But its draw_char() only adds the extra character spacing to the width returned from FreeType, at the end of this line:

godot/scene/resources/dynamic_font.cpp

Line 886 in d7b85fb

return font_at_size->draw_char(p_canvas_item, p_pos, p_char, p_next, color, fallbacks, advance_only, p_outline) + spacing_char;

This underlying issue appears to have existed since the original commit that added the spacing functionality: af6ef01#diff-fce1ec7b33979481c1af96f2d6be6686R787

And Button calls get_string_size() (which calls get_char_size()) and the font's draw() a.k.a Font::draw() but also...

godot/scene/gui/button.cpp

Lines 200 to 228 in 6a0473b

	Point2 text_ofs = (size - style->get_minimum_size() - icon_ofs - font->get_string_size(xl_text) - Point2(_internal_margin[MARGIN_RIGHT] - _internal_margin[MARGIN_LEFT], 0)) / 2.0;
	switch (align) {
	case ALIGN_LEFT: {
	if (_internal_margin[MARGIN_LEFT] > 0) {
	text_ofs.x = style->get_margin(MARGIN_LEFT) + icon_ofs.x + _internal_margin[MARGIN_LEFT] + get_theme_constant("hseparation");
	} else {
	text_ofs.x = style->get_margin(MARGIN_LEFT) + icon_ofs.x;
	}
	text_ofs.y += style->get_offset().y;
	} break;
	case ALIGN_CENTER: {
	if (text_ofs.x < 0)
	text_ofs.x = 0;
	text_ofs += icon_ofs;
	text_ofs += style->get_offset();
	} break;
	case ALIGN_RIGHT: {
	if (_internal_margin[MARGIN_RIGHT] > 0) {
	text_ofs.x = size.x - style->get_margin(MARGIN_RIGHT) - font->get_string_size(xl_text).x - _internal_margin[MARGIN_RIGHT] - get_theme_constant("hseparation");
	} else {
	text_ofs.x = size.x - style->get_margin(MARGIN_RIGHT) - font->get_string_size(xl_text).x;
	}
	text_ofs.y += style->get_offset().y;
	} break;
	}
	text_ofs.y += font->get_ascent();
	font->draw(ci, text_ofs.floor(), xl_text, color, clip_text ? text_clip : -1);

...note that Font::draw() calls both get_char_size() (to check for clipping) and accumulates the result of the draw_char() call:

godot/scene/resources/font.cpp

Lines 69 to 74 in d7b85fb

	int width = get_char_size(p_text[i]).width;
	if (p_clip_w >= 0 && (ofs.x + width) > p_clip_w)
	break; //clip
	ofs.x += draw_char(p_canvas_item, p_pos + ofs, p_text[i], p_text[i + 1], with_outline ? p_outline_modulate : p_modulate, with_outline);

So, in addition to fixing the issue in DynamicFont::draw_char(), it might be helpful to at least add an assert that the two methods are equal in Font::draw() to hopefully avoid similar situations in future...

Other affected code

This means that any code that uses DynamicFont's draw_char() or draw() method to draw space characters or strings containing space characters will render the text incorrectly.

This also affects features like text alignment or clipping if the code uses width value(s) from get_string_size()/get_char_size() with width value(s) returned from draw_char() in calculations.

From some non-comprehensive tests:

• LineEdit -- Bizarrely, didn't seem affected, until I modified the alignment to be centered--whereupon it displayed the non-placeholder text correctly in the running app but incorrectly in the editor! However when I entered placeholder text it centered correctly in editor & app. And...then when I removed the placeholder the original text displayed correctly in the editor too--so who knows what that means...

  (After further investigation I suspect that LineEdit probably doesn't like negative character widths...which is probably fair enough. :) )

• RichTextLabel -- seems to handle the extra space spacing correctly in both bbcode & non-bbcode modes.

• ItemList -- doesn't handle the extra space spacing correctly. It keeps the original spacing and also clips the last characters of my test text.

• CanvasItem.draw_string() -- doesn't handle the extra space spacing correctly.

Here are the related searches:

• get_char_size search

• draw_char search -- if the code stores the return value it is probably affected.

Other handling of extra spacing

As mentioned above, DynamicFont's draw_char() implementation does account for extra character spacing, in addition to the width value returned from FreeType.

As a comparison, BitmapFont's draw_char() implementation actually returns the result of get_char_size()--but BitmapFont doesn't support extra character/space spacing anyway (so this implementation detail is probably kerning related).

godot/scene/resources/font.cpp

Line 572 in d7b85fb

return get_char_size(p_char, p_next).width;

(And, as I discovered, DynamicFont doesn't currently support kerning: #35730)

Solutions

This suggests that some potential solutions:

• Modify DynamicFont's draw_char() implementation to return the result of get_char_size() also although that might be considered "inefficient".

• Modify DynamicFont's draw_char() implementation to add to the returned width if the character is a space.

  But this duplicates the handling code and runs the risk that a similar situation might occur in future (or already occurs) if get_char_size() calculates anything differently to draw_char() (e.g. kerning? hinting? outlines?)

Code to reproduce

Prerequisites:

• A DynamicFont resource. (e.g. located at res://fonts/barlow-regular-64.tres)
• The following script attached to a Control in a scene.

extends Control

var the_font: DynamicFont = preload("res://fonts/barlow-regular-64.tres")

func _ready():
    self.the_font.set_spacing(DynamicFont.SPACING_SPACE, -20)

func _draw():
    assert(the_font.get_string_size(" ").x == self.draw_char(the_font, Vector2(0,0), " ", ""))

Result:

• The assertion in _draw() will fail.

Red Herrings Encountered

From initial research, it appeared the issue may have been because Label & Button use two different ways of drawing their text:

• Label uses draw_char() method of FontDrawer:

  godot/scene/gui/label.cpp

  Line 278 in 6a0473b

  x_ofs += drawer.draw_char(ci, Point2(x_ofs, y_ofs), c, n, font_color);

• Button uses draw() method of Font directly:

  godot/scene/gui/button.cpp

  Line 228 in 6a0473b

  font->draw(ci, text_ofs.floor(), xl_text, color, clip_text ? text_clip : -1);

But this difference was not the cause of the issue with extra space character spacing.

Other code base observations

• There are a number of names given to spacing related properties/methods/constants/enums which complicates finding all the references in the code base (e.g. for character/space spacing):

  • Properties

    • extra_spacing_char, extra_spacing_space

  • Setters/getters/methods

    • set_spacing()/get_spacing()

  • Enums

    • SpacingType with SPACING_CHAR, SPACING_SPACE

  • C++ code

    • spacing_char, spacing_space

    • space_w, space_width

    • ' ' :D

[WIP: Intend to add some other info & code references but since "draft" issue comments aren't supported am posting this first.]

[ErwinBr]

@akien-mga I'm not sure if this issue is supposed to be fixed in 3.2.4 (is it even the intention? I keep reading "doc clarity ..." not sure what it means?), but in any case I tested the RC1 and it does not seem to be different.

[ErwinBr]

I guess this issue will not be solved in 3.2.4?

[Calinou]

I guess this issue will not be solved in 3.2.4?

Nobody has opened a pull request to fix the issue yet, so probably not. Also, since 3.2.4 is already in RC stage, we can't risk merging anything that can cause regressions down the line.

The only pull request that was merged is for documentation, not code.

[ErwinBr]

The only pull request that was merged is for documentation, not code.

Clear now, thanks. Maybe @bruvzg can have a look at this, since he is working on fonts for 4.0.

[Calinou]

This was likely fixed in 4.0 thanks to #46043.

[akien-mga]

Fixed by #46043 (master) and #46055 (3.2).