widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

I think that render_cache_ need a bit more attention:
 * explian what this actually maps
 * what is the cachek poliy (e.g. when will object be removed?)
 * why do you use an (additional) std::unique_ptr if the make already makes this unique?
 * if its not small it deserverves some presizing.

Some comments inline, but I need more time to read this ....

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.

... if the font handler is used ... ?

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

I found no explanation of this w paameter, looks like a kind of hash_salt?
As it seem 0 most times, can we leave it out from the hash calculation?

>  		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)))));

not sure if we need that extra std::unique_ptr here.

> +		}
> +		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());

why not just 
   Rectf source_rect(0f, 0f, image->width(), image->height());
avoding somw conversions and temporary stuff?

> +	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);

Mhh, how do you tile a singel color ;-)
-> ... will use given background_color.
and name it background_color

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