Merge lp:~widelands-dev/widelands/bug-1741779-map-description-edit-cutoff into lp:widelands

[GunChleoc]

Removed texture size restrictions from editboxes. We no longer need them thanks to the RenderedText class.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3514. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/378610485.
Appveyor build 3319. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1741779_map_description_edit_cutoff-3319.

[Notabilis (notabilis27)]

I am not quite sure what to make out of this branch. If I guess right, that means that text in editboxes should be able to become as long as I want. However, when typing long texts I get (both here and in trunk):

Texture (2051, 18) too big! Maximum size is 2048.
terminate called after throwing an instance of 'RT::TextureTooBig'
  what(): Texture (2051, 18) too big! Maximum size is 2048.

Tested with the host-ip box in the LAN lobby, the edit field when saving a game and the map description when using the editor by entering one long string (i.e., keeping key pressed for 10 seconds). When entering multiple "words" in the editor it does not crash in either branch but starts lagging after some time.

Some remarks about the code:

- In Editbox the maximum length can also be set by set_max_length(). I guess the condition there has to be adapted, too?

  m_->maxLength = std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, static_cast<int>(n));

(Also a small documentation bug: the function is called set_max_length() but the in the documentation of set_text() it is called setMaxLength().)

- It seems like MultilineEditbox::Data::maxbytes can be removed completely since it is only used as read-only upper bound and is now fixed at "infinite". I think the conditions where it is used will never trigger now.

[GunChleoc (gunchleoc)]

YOu're right, somebody leaning endlessly on a key will eventually crash it, because there are no word breaks for splitting the text into separate textures.

I have brought back the limit and fixed it for the Multilineeditbox - the bug there was that we forgot to square the texture size.

I haven't tested my last commit yet, it's still compiling.

[Notabilis (notabilis27)]

Hm, seems to behave as with the commit before. Only tested the map description edit box, but long inputs still crash and I seem to be able to input ... well, at least very long texts.

On a side note: Tab key has a strange order in that dialog.

[GunChleoc (gunchleoc)]

I have tied the calculation to actual rendered font height now - this fixes the Editbox for me.

The problem in Multilineeditbox was the text_width function used by the WordWrap class - I have added a safeguard there too.

I have also opened a new bug for the tabbing order:

https://bugs.launchpad.net/widelands/+bug/1772561

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3533. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/381985250.
Appveyor build 3337. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1741779_map_description_edit_cutoff-3337.

[Notabilis (notabilis27)]

Thanks for the bug report.

Unfortunately, I am still unable to notice any changes. :(
Maybe I am understanding the purpose of this branch wrong or are testing something unrelated.
Can you describe a testcase that works different in this branch than in trunk?

Setting maxLength and maxBytes to 3 in the source does restrict the number of characters I am able to enter, so it seems to me as if the calculations show different results for you than for me. Sounds strange, though, since I am assuming that the font and its height is everywhere the same. Does it help if I try to figure out restrictions that do not lead to crashes for me?

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3543. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/382558005.
Appveyor build 3347. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1741779_map_description_edit_cutoff-3347.

[GunChleoc (gunchleoc)]

Open the "Last bastion" map in the editor, open "Map Options" and scroll down to the bottom of the description. Some text is missing here in trunk.

Font and its height is not the same everywhere - we get bigger textures for Arabic.

By all means, change the restrictions so it won't crash for you. I would expect the results to be identical for the default fontset, so this is strange indeed.

I would like to rework all this stuff again once we get rid of the old renderer - at the moment, the WordWrap class is used by both renderers, so I don' want to touch it. Too many complications.

[Notabilis (notabilis27)]

Okay, thanks. I think I finally understand what is happening on my system.
Regarding the primary bug, I am unfortunately unable to reproduce it on my system. But the code is looking okay and if it fixes the bug for you, that is enough testing for me.

Are you by chance testing in release mode? Or has your system only limited texture memory? My RT::TextureTooBig exception is not triggered in release mode since another restriction is used there. My exception is triggered due to graphic/text/rt_render.cc:266 where different values for debug and release are used. When using the release version your restrictions work fine. The problem in debug mode is that the exception uses the smaller (hardcoded) value while your checks are using the real (hardware) value which is seemingly bigger on my system. Maybe we should move the restriction from rt_render.cc:266 into graphic/graphic.cc:91 ? (If you do: render_text.cc:183 can then be cleaned up, too.) Except for the text rendering no-one seems to care about the maximum texture size so moving this should be safe.

I added some questions regarding int-casting in the diff, but they haven't been introduced by your changes and someone probably had a reasons for the casts.

[kaputtnik (franku)]

The main bug is solved, IMHO.

review: Approve (testing)

[GunChleoc (gunchleoc)]

I introduced that condition in check_size to make sure that the texture size would be OK for everybody. That was especially important before we had the RenderedText class. I have now shifted the difference to Graphic, which will hopefully fix the bug in debug builds.

[Notabilis (notabilis27)]

Thanks, it is no longer crashing for me now.
One nit: In rendered_text.cc:183 the DEBUG-check is done twice, once there and once inside the called max_texture_size_for_font_rendering().

review: Approve

[GunChleoc (gunchleoc)]

That line needs to be fixed

[GunChleoc (gunchleoc)]

Done :)

@bunnybot merge

=== modified file 'src/graphic/CMakeLists.txt'
--- src/graphic/CMakeLists.txt	2018-05-11 04:42:13 +0000
+++ src/graphic/CMakeLists.txt	2018-05-31 16:50:31 +0000
@@ -300,6 +300,7 @@
     text_layout.cc
     text_layout.h
   DEPENDS
+    graphic # TODO(Gunchleoc): For text_width safety only - rewrite that function
     graphic_align
     graphic_color
     graphic_surface

=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2018-04-27 06:11:05 +0000
+++ src/graphic/graphic.cc	2018-05-31 16:50:31 +0000
@@ -171,6 +171,15 @@
 	return render_target_.get();
 }
 
+int Graphic::max_texture_size_for_font_rendering() const {
+// Test with minimum supported size in debug builds.
+#ifndef NDEBUG
+		return kMinimumSizeForTextures;
+#else
+		return max_texture_size_;
+#endif
+}
+
 bool Graphic::fullscreen() {
 	uint32_t flags = SDL_GetWindowFlags(sdl_window_);
 	return (flags & SDL_WINDOW_FULLSCREEN) || (flags & SDL_WINDOW_FULLSCREEN_DESKTOP);

=== modified file 'src/graphic/graphic.h'
--- src/graphic/graphic.h	2018-04-27 06:11:05 +0000
+++ src/graphic/graphic.h	2018-05-31 16:50:31 +0000
@@ -81,9 +81,7 @@
 		return sdl_window_;
 	}
 
-	int max_texture_size() const {
-		return max_texture_size_;
-	}
+	int max_texture_size_for_font_rendering() const;
 
 	ImageCache& images() const {
 		return *image_cache_.get();

=== modified file 'src/graphic/text/rendered_text.cc'
--- src/graphic/text/rendered_text.cc	2018-04-07 16:59:00 +0000
+++ src/graphic/text/rendered_text.cc	2018-05-31 16:50:31 +0000
@@ -180,11 +180,7 @@
 
 	// Draw Solid background Color
 	if (rect.has_background_color()) {
-#ifndef NDEBUG
-		const int maximum_size = kMinimumSizeForTextures;
-#else
-		const int maximum_size = g_gr->max_texture_size();
-#endif
+		const int maximum_size = g_gr->max_texture_size_for_font_rendering();
 		const int tile_width = std::min(maximum_size, rect.width());
 		const int tile_height = std::min(maximum_size, rect.height());
 		for (int tile_x = blit_point.x; tile_x + tile_width <= blit_point.x + rect.width();

=== modified file 'src/graphic/text/rt_render.cc'
--- src/graphic/text/rt_render.cc	2018-04-07 16:59:00 +0000
+++ src/graphic/text/rt_render.cc	2018-05-31 16:50:31 +0000
@@ -264,12 +264,7 @@
 	/// Throws a TextureTooBig exception if the given dimensions would be bigger than the graphics
 	/// can handle
 	void check_size(int check_w, int check_h) {
-// Test for minimum supported size in debug builds.
-#ifndef NDEBUG
-		const int maximum_size = kMinimumSizeForTextures;
-#else
-		const int maximum_size = g_gr->max_texture_size();
-#endif
+		const int maximum_size = g_gr->max_texture_size_for_font_rendering();
 		if (check_w > maximum_size || check_h > maximum_size) {
 			const std::string error_message =
 			   (boost::format("Texture (%d, %d) too big! Maximum size is %d.") % check_w % check_h %

=== modified file 'src/graphic/text_layout.cc'
--- src/graphic/text_layout.cc	2018-05-05 17:10:37 +0000
+++ src/graphic/text_layout.cc	2018-05-31 16:50:31 +0000
@@ -26,6 +26,7 @@
 #include <boost/format.hpp>
 
 #include "graphic/font_handler1.h"
+#include "graphic/graphic.h"
 #include "graphic/image.h"
 #include "graphic/text/bidi.h"
 #include "graphic/text/font_set.h"
@@ -49,7 +50,7 @@
 }
 
 int text_width(const std::string& text, int ptsize) {
-	return UI::g_fh1->render(as_editorfont(text, ptsize - UI::g_fh1->fontset()->size_offset()))
+	return UI::g_fh1->render(as_editorfont(text.substr(0, g_gr->max_texture_size_for_font_rendering() / text_height() - 1), ptsize - UI::g_fh1->fontset()->size_offset()))
 	   ->width();
 }
 

=== modified file 'src/ui_basic/editbox.cc'
--- src/ui_basic/editbox.cc	2018-04-27 06:11:05 +0000
+++ src/ui_basic/editbox.cc	2018-05-31 16:50:31 +0000
@@ -97,8 +97,7 @@
 	m_->caret = 0;
 	m_->scrolloffset = 0;
 	// yes, use *signed* max as maximum length; just a small safe-guard.
-	m_->maxLength =
-	   std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, std::numeric_limits<int32_t>::max());
+	set_max_length(std::numeric_limits<int32_t>::max());
 
 	set_handle_mouse(true);
 	set_can_focus(true);
@@ -124,7 +123,7 @@
  * Set the current text in the edit box.
  *
  * The text is truncated if it is longer than the maximum length set by
- * \ref setMaxLength().
+ * \ref set_max_length().
  */
 void EditBox::set_text(const std::string& t) {
 	if (t == m_->text)
@@ -146,7 +145,7 @@
  * its end is cut off to fit into the maximum length.
  */
 void EditBox::set_max_length(uint32_t const n) {
-	m_->maxLength = std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, static_cast<int>(n));
+	m_->maxLength = std::min(g_gr->max_texture_size_for_font_rendering() / text_height(), static_cast<int>(n));
 
 	if (m_->text.size() > m_->maxLength) {
 		m_->text.erase(m_->text.begin() + m_->maxLength, m_->text.end());

=== modified file 'src/ui_basic/multilineeditbox.cc'
--- src/ui_basic/multilineeditbox.cc	2018-05-13 07:15:39 +0000
+++ src/ui_basic/multilineeditbox.cc	2018-05-31 16:50:31 +0000
@@ -97,7 +97,7 @@
      background_style(style),
      cursor_pos(0),
      lineheight(text_height()),
-     maxbytes(std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, 0xffff)),
+     maxbytes(std::min(g_gr->max_texture_size_for_font_rendering() * g_gr->max_texture_size_for_font_rendering() / (text_height() * text_height()), std::numeric_limits<int32_t>::max())),
      ww_valid(false),
      owner(o) {
 	scrollbar.moved.connect(boost::bind(&MultilineEditbox::scrollpos_changed, &o, _1));
@@ -133,8 +133,7 @@
 		d_->erase_bytes(d_->prev_char(d_->text.size()), d_->text.size());
 	}
 
-	d_->set_cursor_pos(d_->text.size());
-
+	d_->set_cursor_pos(0);
 	d_->update();
 	d_->scroll_cursor_into_view();