widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

If you click on "Show diff comments" for a post, the diff will switch to the appropriate revision so you can see them.

Answers to your comments inline - I added all important comments to the code as well. I will also replace the 0's with 0.f for the Rectf calls.

I think the caching model is generally broken and also the cause of the cache. The textures are kept in a cache that can overflow, so the RenderedText has references to objects that have been destroyed by the TextureCache. I think that I need to do the following 2 things:

- Turn the TextureCache into a templated object por mimic the code, so we will have an upper size limit for both the small textures within a text and the while RenderedText, and maximum reuse of computed textures at the same time,

- Replace unique_ptr with shared_ptr. This way, when an texture is removed from the cache, it won't be destroyed if a RenderedText is still using it. Furthermore, some UI elements prerender their texts and keep them for their lifetime, so those objects need to share ownership as well. I think the only reason that this particular problem hasn't blown up in our faces yet is that the texture cache never overflows in Build 19.

Diff comments:

> 
> === modified file 'src/logic/map_objects/map_object.cc'
> --- src/logic/map_objects/map_object.cc	2017-05-13 18:48:26 +0000
> +++ src/logic/map_objects/map_object.cc	2017-05-20 18:11:58 +0000
> @@ -463,30 +464,25 @@
>  	}
>  
>  	// Rendering text is expensive, so let's just do it for only a few sizes.
> -	scale = std::round(scale);
> -	if (scale == 0.f) {
> +	scale = std::round(2 * (scale > 1.f ? std::sqrt(scale) : std::pow(scale, 2))) / 2;

Each zoom factor will be a separate rendered text, since the text to render changes (font size option).

Formula is a bit fancy to prevent overlap.

> +	if (scale < 1.f) {
>  		return;
>  	}
>  	const int font_size = scale * UI_FONT_SIZE_SMALL;
>  
>  	// We always render this so we can have a stable position for the statistics string.
> -	const Image* rendered_census_info =
> -	   UI::g_fh1->render(as_condensed(census, UI::Align::kCenter, font_size), 120);
> -
> -	const Vector2i base_pos = field_on_dst.cast<int>() - Vector2i(0, 48) * scale;
> -	Vector2i census_pos(base_pos);
> -	UI::correct_for_align(UI::Align::kCenter, rendered_census_info->width(), &census_pos);
> +	const UI::RenderedText* rendered_census =
> +	   UI::g_fh1->render(as_condensed(census, UI::Align::kCenter, font_size), 120 * scale);
> +	Vector2i position = field_on_dst.cast<int>() - Vector2i(0, 48) * scale;
>  	if (draw_text & TextToDraw::kCensus) {
> -		dst->blit(census_pos, rendered_census_info, BlendMode::UseAlpha);
> +		rendered_census->draw(*dst, position, UI::Align::kCenter);
>  	}
>  
>  	if (draw_text & TextToDraw::kStatistics && !statictics.empty()) {
> -		Vector2i statistics_pos =
> -		   base_pos + Vector2i(0, rendered_census_info->height() / 2 + 10 * scale);
> -		const Image* rendered_statictics =
> +		const UI::RenderedText* rendered_statistics =
>  		   UI::g_fh1->render(as_condensed(statictics, UI::Align::kCenter, font_size));
> -		UI::correct_for_align(UI::Align::kCenter, rendered_statictics->width(), &statistics_pos);
> -		dst->blit(statistics_pos, rendered_statictics, BlendMode::UseAlpha);
> +		position.y += rendered_census->height() + text_height(font_size) / 4;
> +		rendered_statistics->draw(*dst, position, UI::Align::kCenter);
>  	}
>  }
>  
> 
> === modified file 'src/wui/chatoverlay.cc'
> --- src/wui/chatoverlay.cc	2017-04-22 12:19:21 +0000
> +++ src/wui/chatoverlay.cc	2017-05-20 18:11:58 +0000
> @@ -167,13 +167,13 @@
>  	if (!m->havemessages_)
>  		return;
>  
> -	const Image* im = nullptr;
> +	const UI::RenderedText* im = nullptr;
>  	try {
>  		im = UI::g_fh1->render(m->all_text_, get_w());

Messages time out, so this will never get big.

>  	} catch (RT::WidthTooSmall&) {
>  		// Oops, maybe one long word? We render again, not limiting the width, but
>  		// render everything in one single line.
> -		im = UI::g_fh1->render(m->all_text_, 0);
> +		im = UI::g_fh1->render(m->all_text_);
>  	}
>  	assert(im != nullptr);
>  
> 
> === modified file 'src/wui/waresdisplay.cc'
> --- src/wui/waresdisplay.cc	2017-05-13 13:14:29 +0000
> +++ src/wui/waresdisplay.cc	2017-05-20 18:11:58 +0000
> @@ -331,11 +331,10 @@
>  	dst.fill_rect(Recti(p + Vector2i(0, WARE_MENU_PIC_HEIGHT), w, WARE_MENU_INFO_SIZE),
>  	              info_color_for_ware(id));
>  
> -	const Image* text = UI::g_fh1->render(as_waresinfo(info_for_ware(id)));
> -	if (text)  // might be zero when there is no info text.
> -		dst.blit(p + Vector2i(w - text->width() - 1,
> -		                      WARE_MENU_PIC_HEIGHT + WARE_MENU_INFO_SIZE + 1 - text->height()),
> -		         text);
> +	const UI::RenderedText* rendered_text = UI::g_fh1->render(as_waresinfo(info_for_ware(id)));

We get only empty RenderedRects. Since this is only rendered once and then grabbed from the cache, I don't see much of an efficiency issue. You can see it in action in the wares statistics menu.

Another other option would be to write the result for info_for_ware(id) into a string variable that we check for empty(). So, it's the old memory vs. time issue I guess - I don't know which version would be better.

> +	rendered_text->draw(
> +	   dst, Vector2i(p.x + w - rendered_text->width() - 1,
> +	                 p.y + WARE_MENU_PIC_HEIGHT + WARE_MENU_INFO_SIZE + 1 - rendered_text->height()));
>  }
>  
>  // Wares highlighting/selecting


-- 
https://code.launchpad.net/~widelands-dev/widelands/fh1-multitexture/+merge/323903
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fh1-multitexture.