widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Thanks for taking on this review :)

 * explian what this actually maps

It keeps rendered text so that we won't have to render it again - rendering is expensive, especially for complex stuff like the help windows. And some text images are reused a lot, e.g. the census and statistics strings.

 * what is the cachek poliy (e.g. when will object be removed?)

Just like with the main image cache and the animation cache, objects are never removed. I guess we could have a flush function if the memory consumption should get too big and that could be called at strategic points, e.g. when switching between fullscreen menu and game.

 * why do you use an (additional) std::unique_ptr if the make already makes this unique?

I don't know what you mean. Are you misreading make_pair as make_unique here?

 * if its not small it deserverves some presizing.

You mean as is already done in texture_cache_(new TextureCache(kTextureCacheSize))?

I have also added comments to your in-line comments.

Diff comments:

> 
> === modified file 'src/graphic/font_handler1.cc'
> --- src/graphic/font_handler1.cc	2017-03-25 15:32:49 +0000
> +++ src/graphic/font_handler1.cc	2017-05-19 14:23:43 +0000
> @@ -50,57 +50,8 @@
>  // 30 MB was enough to cache texts for many frames (> 1000), while it is
>  // quickly overflowing in the map selection menu.
>  // This might need reevaluation is the new font handler is used for more stuff.

Changed it to "if this font handler..." since we also still have the old font renderer kicking about.

> -const uint32_t RICHTEXT_TEXTURE_CACHE = 30 << 20;  // shifting converts to MB
> -
> -// An Image implementation that recreates a rich text texture when needed on
> -// the fly. It is meant to be saved into the ImageCache.
> -class RTImage : public Image {
> -public:
> -	RTImage(const string& ghash,
> -	        TextureCache* texture_cache,
> -	        std::function<RT::Renderer*()> get_renderer,
> -	        const string& text,
> -	        int gwidth)
> -	   : hash_(ghash),
> -	     text_(text),
> -	     width_(gwidth),
> -	     get_renderer_(get_renderer),
> -	     texture_cache_(texture_cache) {
> -	}
> -	virtual ~RTImage() {
> -	}
> -
> -	// Implements Image.
> -	int width() const override {
> -		return texture()->width();
> -	}
> -	int height() const override {
> -		return texture()->height();
> -	}
> -
> -	const BlitData& blit_data() const override {
> -		return texture()->blit_data();
> -	}
> -
> -private:
> -	Texture* texture() const {
> -		Texture* surf = texture_cache_->get(hash_);
> -		if (surf != nullptr) {
> -			return surf;
> -		}
> -		return texture_cache_->insert(
> -		   hash_, std::unique_ptr<Texture>(get_renderer_()->render(text_, width_)));
> -	}
> -
> -	const string hash_;
> -	const string text_;
> -	int width_;
> -	std::function<RT::Renderer*()> get_renderer_;
> -
> -	// Nothing owned.
> -	TextureCache* const texture_cache_;
> -};
> -}
> +constexpr uint32_t kTextureCacheSize = 30 << 20;  // shifting converts to MB
> +}  // namespace
>  
>  namespace UI {
>  
> @@ -110,26 +61,23 @@
>  class FontHandler1 : public IFontHandler1 {
>  public:
>  	FontHandler1(ImageCache* image_cache, const std::string& locale)
> -	   : texture_cache_(new TextureCache(RICHTEXT_TEXTURE_CACHE)),
> +	   : texture_cache_(new TextureCache(kTextureCacheSize)),
>  	     fontsets_(),
>  	     fontset_(fontsets_.get_fontset(locale)),
>  	     rt_renderer_(new RT::Renderer(image_cache, texture_cache_.get(), fontsets_)),
>  	     image_cache_(image_cache) {
>  	}
>  	virtual ~FontHandler1() {
> +		render_cache_.clear();
>  	}
>  
> -	const Image* render(const string& text, uint16_t w = 0) override {
> +	const RenderedText* render(const string& text, uint16_t w = 0) override {

w is the maximum width for the returned text, e.g. the available width of of the text area that wants the text. Rendered text is only identical if both the text and the width is identical. 0 means "no width restriction."

>  		const string hash = boost::lexical_cast<string>(w) + text;
> -
> -		if (image_cache_->has(hash))
> -			return image_cache_->get(hash);
> -
> -		std::unique_ptr<RTImage> image(
> -		   new RTImage(hash, texture_cache_.get(), [this] { return rt_renderer_.get(); }, text, w));
> -		image->width();  // force the rich text to get rendered in case there is an exception thrown.
> -
> -		return image_cache_->insert(hash, std::move(image));
> +		if (render_cache_.count(hash) != 1) {
> +			render_cache_.insert(std::make_pair(hash, std::unique_ptr<const RenderedText>(std::move(rt_renderer_->render(text, w)))));

We do - we need to convert a pointer into a unique_ptr, because the renderer gives us a raw pointer, but the cache stores everything as unique_ptr - the cache owns the object then until the cache is destroyed.

> +		}
> +		assert(render_cache_.count(hash) == 1);
> +		return render_cache_.find(hash)->second.get();
>  	}
>  
>  	UI::FontSet const* fontset() const override {
> 
> === modified file 'src/graphic/rendertarget.cc'
> --- src/graphic/rendertarget.cc	2017-05-13 18:48:26 +0000
> +++ src/graphic/rendertarget.cc	2017-05-19 14:23:43 +0000
> @@ -142,9 +143,15 @@
>   *
>   * This blit function copies the pixels to the destination surface.
>   */
> -void RenderTarget::blit(const Vector2i& dst, const Image* image, BlendMode blend_mode) {
> -	Rectf source_rect(Vector2i::zero(), image->width(), image->height());
> -	Rectf destination_rect(dst.x, dst.y, source_rect.w, source_rect.h);
> +void RenderTarget::blit(const Vector2i& dst,
> +                        const Image* image,
> +                        BlendMode blend_mode,
> +                        UI::Align align) {
> +	Vector2i destination_point(dst);
> +	UI::correct_for_align(align, image->width(), &destination_point);
> +
> +	Rectf source_rect(Vector2i(0, 0), image->width(), image->height());

Done :)

> +	Rectf destination_rect(destination_point.x, destination_point.y, source_rect.w, source_rect.h);
>  
>  	if (to_surface_geometry(&destination_rect, &source_rect)) {
>  		// I seem to remember seeing 1. a lot in blitting calls.
> 
> === added file 'src/graphic/text/rendered_text.h'
> --- src/graphic/text/rendered_text.h	1970-01-01 00:00:00 +0000
> +++ src/graphic/text/rendered_text.h	2017-05-19 14:23:43 +0000
> @@ -0,0 +1,140 @@
> +/*
> + * Copyright (C) 2017 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#ifndef WL_GRAPHIC_TEXT_RENDERED_TEXT_H
> +#define WL_GRAPHIC_TEXT_RENDERED_TEXT_H
> +
> +#include <memory>
> +#include <vector>
> +
> +#include "base/rect.h"
> +#include "base/vector.h"
> +#include "graphic/image.h"
> +#include "graphic/rendertarget.h"
> +#include "graphic/texture.h"
> +
> +namespace UI {
> +
> +/// A rectangle that contains blitting information for rendered text.
> +class RenderedRect {
> +public:
> +	/// Whether the RenderedRect's image should be blitted once or tiled
> +	enum class DrawMode { kBlit, kTile };
> +
> +private:
> +	RenderedRect(const Recti& init_rect,
> +	             const Image* init_image,
> +	             bool visited,
> +	             const RGBColor& color,
> +	             bool is_background_color_set,
> +	             DrawMode init_mode);
> +
> +public:
> +	/// RenderedRect will contain a background image that should be tiled
> +	RenderedRect(const Recti& init_rect, const Image* init_image);
> +
> +	/// RenderedRect will contain a background color that should be tiled
> +	RenderedRect(const Recti& init_rect, const RGBColor& color);

Tiling means little rectangles next to each other, not on top of each other. Let's say I have a multilinetextarea with a huge amount of text to scroll through that I want to have a special background color for, and that text gets 12000 pixels high, but my graphics card can only handle 2048 pixels. Crash. So, we break it down into smaller rectangles.

You can see the tiling code in the draw function.

> +
> +	/// RenderedRect will contain a normal image
> +	RenderedRect(const Image* init_image);
> +	~RenderedRect() {
> +	}
> +
> +	/// An image to be blitted. Can be nullptr.
> +	const Image* image() const;
> +
> +	/// The x position of the rectangle
> +	int x() const;
> +	/// The y position of the rectangle
> +	int y() const;
> +	/// The width of the rectangle
> +	int width() const;
> +	/// The height of the rectangle
> +	int height() const;
> +
> +	/// Change x and y position of the rectangle.
> +	void set_origin(const Vector2i& new_origin);
> +
> +	/// Set that this rectangle was already visited by the font renderer. Needed by the font renderer
> +	/// for correct positioning.
> +	void set_visited();
> +	/// Whether this rectangle was already visited by the font renderer
> +	bool was_visited() const;
> +
> +	/// Whether this rectangle contains a background color
> +	bool has_background_color() const;
> +	/// This rectangle's background color
> +	const RGBColor& background_color() const;
> +
> +	/// Whether the RenderedRect's image should be blitted once or tiled
> +	DrawMode mode() const;
> +
> +private:
> +	Recti rect_;
> +	const Image* image_;  // Not owned
> +	bool visited_;
> +	const RGBColor background_color_;
> +	const bool is_background_color_set_;
> +	const DrawMode mode_;
> +};
> +
> +struct RenderedText {
> +	/// RenderedRects that can be drawn on screen
> +	std::vector<std::unique_ptr<RenderedRect>> rects;
> +
> +	/// The width occupied  by all rects in pixels.
> +	int width() const;
> +	/// The height occupied  by all rects in pixels.
> +	int height() const;
> +
> +	enum class CropMode {
> +		kRenderTarget,  // The RenderTarget will handle all cropping
> +		kHorizontal     // The draw() method will handle horizontal cropping
> +	};
> +
> +	/// Draw the rects. 'position', 'region' and 'align' are used to control the overall drawing
> +	/// position and cropping.
> +	/// For 'cropmode', use kRenderTarget if you wish the text to fill the whole RenderTarget, e.g.
> +	/// for scrolling panels. Use kHorizontal for horizontal cropping in smaller elements, e.g. table
> +	/// cells.
> +	void draw(RenderTarget& dst,
> +	          const Vector2i& position,
> +	          const Recti& region,
> +	          UI::Align align = UI::Align::kLeft,
> +	          CropMode cropmode = CropMode::kRenderTarget) const;
> +
> +	/// Draw the rects without cropping. 'position' and 'align' are used to control the overall
> +	/// drawing position
> +	void draw(RenderTarget& dst, const Vector2i& position, UI::Align align = UI::Align::kLeft) const;
> +
> +private:
> +	/// Helper function for CropMode::kHorizontal
> +	void blit_cropped(RenderTarget& dst,
> +	                  int offset_x,
> +	                  const Vector2i& position,
> +	                  const Vector2i& blit_point,
> +	                  const RenderedRect& rect,
> +	                  const Recti& region,
> +	                  Align align) const;
> +};
> +
> +}  // namespace UI
> +
> +#endif  // end of include guard: WL_GRAPHIC_TEXT_RENDERED_TEXT_H


-- 
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.