widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve

2 comments to think about, otherwise LGTM.

Diff comments:

> === modified file 'src/editor/ui_menus/editor_tool_set_terrain_options_menu.cc'
> --- src/editor/ui_menus/editor_tool_set_terrain_options_menu.cc	2014-12-04 09:00:20 +0000
> +++ src/editor/ui_menus/editor_tool_set_terrain_options_menu.cc	2014-12-07 19:05:20 +0000
> @@ -145,7 +145,7 @@
>  		}
>  
>  		// Make sure we delete this later on.
> -		offscreen_images->emplace_back(new_in_memory_image("dummy_hash", texture));
> +		offscreen_images->emplace_back(new_in_memory_image(texture));
>  		break;
>  	}
>  	/** TRANSLATORS: %1% = terrain name, %2% = list of terrain types  */
> 
> === modified file 'src/graphic/animation.cc'
> --- src/graphic/animation.cc	2014-12-07 19:05:19 +0000
> +++ src/graphic/animation.cc	2014-12-07 19:05:20 +0000
> @@ -127,6 +127,7 @@
>  	uint32_t frametime() const override;
>  	const Point& hotspot() const override;
>  	const Image& representative_image_from_disk() const override;
> +	const std::string& representative_image_from_disk_filename() const override;
>  	virtual void blit(uint32_t time, const Point&, const Rect& srcrc, const RGBColor* clr, Surface*)
>  	   const override;
>  	void trigger_soundfx(uint32_t framenumber, uint32_t stereo_position) const override;
> @@ -302,6 +303,10 @@
>  	return *frames_[0];
>  }
>  
> +const std::string& NonPackedAnimation::representative_image_from_disk_filename() const {
> +	return image_files_[0];
> +}
> +
>  void NonPackedAnimation::trigger_soundfx(uint32_t time, uint32_t stereo_position) const {
>  	if (sound_effect_.empty()) {
>  		return;
> 
> === modified file 'src/graphic/animation.h'
> --- src/graphic/animation.h	2014-12-07 19:05:19 +0000
> +++ src/graphic/animation.h	2014-12-07 19:05:20 +0000
> @@ -72,6 +72,7 @@
>  	// a clutch needed to make sure that messages can always be displayed, even
>  	// no image processing has taken place before.
>  	virtual const Image& representative_image_from_disk() const = 0;
> +	virtual const std::string& representative_image_from_disk_filename() const = 0;
>  
>  	/// Blit the animation frame that should be displayed at the given time index
>  	/// so that the given point is at the top left of the frame. Srcrc defines
> 
> === modified file 'src/graphic/font_handler.cc'
> --- src/graphic/font_handler.cc	2014-11-24 07:10:03 +0000
> +++ src/graphic/font_handler.cc	2014-12-07 19:05:20 +0000
> @@ -22,6 +22,7 @@
>  #include "graphic/font_handler.h"
>  
>  #include <list>
> +#include <memory>
>  
>  #include <SDL_ttf.h>
>  #include <boost/algorithm/string.hpp>
> @@ -73,7 +74,7 @@
>  	/*@}*/
>  
>  	/*@{*/
> -	const Image* image;
> +	std::unique_ptr<const Image> image;
>  	uint32_t width;
>  	uint32_t height;
>  	/*@}*/
> @@ -93,7 +94,6 @@
>  
>  	~Data() {
>  		while (!linecache.empty()) {
> -			delete linecache.back().image;
>  			linecache.pop_back();
>  		}
>  	}
> @@ -161,7 +161,6 @@
>  	render_line(*it);
>  
>  	while (linecache.size() > MaxLineCacheSize) {
> -		delete linecache.back().image;
>  		linecache.pop_back();
>  	}
>  
> @@ -197,7 +196,7 @@
>  		return;
>  	}
>  
> -	lce.image = new_in_memory_image("dummy_hash", new Texture(text_surface));
> +	lce.image = new_in_memory_image(new Texture(text_surface));
>  	lce.width = lce.image->width();
>  	lce.height = lce.image->height();
>  }
> @@ -221,7 +220,7 @@
>  	UI::correct_for_align(align, lce.width + 2 * LINE_MARGIN, lce.height, &dstpoint);
>  
>  	if (lce.image)
> -		dst.blit(Point(dstpoint.x + LINE_MARGIN, dstpoint.y), lce.image);
> +		dst.blit(Point(dstpoint.x + LINE_MARGIN, dstpoint.y), lce.image.get());
>  
>  	if (caret <= copytext.size())
>  		draw_caret(dst, style, dstpoint, copytext, caret);
> @@ -239,7 +238,7 @@
>  	const LineCacheEntry & lce = d->get_line(style, text);
>  
>  	if (lce.image)
> -		dst.blit(dstpoint, lce.image);
> +		dst.blit(dstpoint, lce.image.get());
>  
>  	return lce.width;
>  }
> 
> === modified file 'src/graphic/font_handler1.cc'
> --- src/graphic/font_handler1.cc	2014-11-24 07:12:35 +0000
> +++ src/graphic/font_handler1.cc	2014-12-07 19:05:20 +0000
> @@ -57,7 +57,6 @@
>  	// Implements Image.
>  	uint16_t width() const override {return texture()->width();}
>  	uint16_t height() const override {return texture()->height();}
> -	const string& hash() const override {return hash_;}
>  	Texture* texture() const override {
>  		Texture* surf = texture_cache_->get(hash_);
>  		if (surf)
> @@ -100,7 +99,7 @@
>  		std::unique_ptr<RTImage> image(new RTImage(hash, texture_cache_, renderer_.get(), text, w));
>  		image->texture(); // force the rich text to get rendered in case there is an exception thrown.
>  
> -		return image_cache_->insert(image.release());
> +		return image_cache_->insert(hash, std::move(image));
>  	}
>  
>  private:
> 
> === modified file 'src/graphic/gl/road_program.cc'
> --- src/graphic/gl/road_program.cc	2014-11-24 07:10:03 +0000
> +++ src/graphic/gl/road_program.cc	2014-12-07 19:05:20 +0000
> @@ -87,8 +87,8 @@
>  	u_normal_road_texture_ = glGetUniformLocation(gl_program_.object(), "u_normal_road_texture");
>  	u_busy_road_texture_ = glGetUniformLocation(gl_program_.object(), "u_busy_road_texture");
>  
> -	normal_road_texture_.reset(load_image("world/pics/roadt_normal.png"));
> -	busy_road_texture_.reset(load_image("world/pics/roadt_busy.png"));
> +	normal_road_texture_ = load_image("world/pics/roadt_normal.png");
> +	busy_road_texture_ = load_image("world/pics/roadt_busy.png");
>  }
>  
>  RoadProgram::~RoadProgram() {
> 
> === modified file 'src/graphic/graphic.cc'
> --- src/graphic/graphic.cc	2014-12-07 19:05:19 +0000
> +++ src/graphic/graphic.cc	2014-12-07 19:05:20 +0000
> @@ -67,7 +67,7 @@
>       m_window_mode_height(window_mode_h),
>       m_update(true),
>       texture_cache_(create_texture_cache(TRANSIENT_TEXTURE_CACHE_SIZE)),
> -     image_cache_(new ImageCache(texture_cache_.get())),
> +     image_cache_(new ImageCache()),
>       animation_manager_(new AnimationManager())
>  {
>  	// Request an OpenGL 2 context with double buffering.
> 
> === modified file 'src/graphic/image.h'
> --- src/graphic/image.h	2014-11-24 07:10:03 +0000
> +++ src/graphic/image.h	2014-12-07 19:05:20 +0000
> @@ -39,11 +39,9 @@
>  
>  	virtual uint16_t width() const = 0;
>  	virtual uint16_t height() const = 0;
> -
> -	// Internal functions needed for caching.
>  	virtual Texture* texture() const = 0;
> -	virtual const std::string& hash() const = 0;
>  
> +private:
>  	DISALLOW_COPY_AND_ASSIGN(Image);
>  };
>  
> 
> === modified file 'src/graphic/image_cache.cc'
> --- src/graphic/image_cache.cc	2014-11-24 07:10:03 +0000
> +++ src/graphic/image_cache.cc	2014-12-07 19:05:20 +0000
> @@ -20,78 +20,37 @@
>  #include "graphic/image_cache.h"
>  
>  #include <cassert>
> +#include <memory>
>  #include <string>
>  
>  #include "base/log.h"
>  #include "graphic/image.h"
>  #include "graphic/image_io.h"
> +#include "graphic/in_memory_image.h"
>  #include "graphic/texture.h"
> -#include "graphic/texture_cache.h"
> -
> -namespace  {
> -
> -// Image Implementation that loads images from disc when they should be drawn.
> -// Uses TextureCache. These images are meant to be cached in ImageCache.
> -class FromDiskImage : public Image {
> -public:
> -	FromDiskImage(const std::string& filename, TextureCache* texture_cache) :
> -		filename_(filename),
> -		texture_cache_(texture_cache) {
> -			Texture* texture = reload_image_();
> -			w_ = texture->width();
> -			h_ = texture->height();
> -		}
> -	virtual ~FromDiskImage() {}
> -
> -	// Implements Image.
> -	uint16_t width() const override {return w_; }
> -	uint16_t height() const override {return h_;}
> -	const std::string& hash() const override {return filename_;}
> -	Texture* texture() const override {
> -		Texture* texture = texture_cache_->get(filename_);
> -		if (texture)
> -			return texture;
> -		return reload_image_();
> -	}
> -
> -private:
> -	Texture* reload_image_() const {
> -		Texture* texture = texture_cache_->insert(filename_, load_image(filename_), false);
> -		return texture;
> -	}
> -	uint16_t w_, h_;
> -	const std::string filename_;
> -
> -	TextureCache* const texture_cache_;  // Not owned.
> -};
> -
> -}  // namespace
> -
> -ImageCache::ImageCache(TextureCache* const texture_cache) : texture_cache_(texture_cache) {
> +
> +ImageCache::ImageCache() {
>  }
>  
>  ImageCache::~ImageCache() {
> -	for (ImageMap::value_type& p : images_) {
> -		delete p.second;
> -	}
> -	images_.clear();
>  }
>  
>  bool ImageCache::has(const std::string& hash) const {
>  	return images_.count(hash);
>  }
>  
> -const Image* ImageCache::insert(const Image* image) {
> -	assert(!has(image->hash()));
> -	images_.insert(make_pair(image->hash(), image));
> -	return image;
> +const Image* ImageCache::insert(const std::string& hash, std::unique_ptr<const Image> image) {
> +	assert(!has(hash));

Should we check for g_fs->file_exists here?

> +	const Image* return_value = image.get();
> +	images_.insert(make_pair(hash, std::move(image)));

Why not just return image.get() and do away with the extra variable? Unless image.get() has a side effect that we need for the insert.

> +	return return_value;
>  }
>  
>  const Image* ImageCache::get(const std::string& hash) {
>  	ImageMap::const_iterator it = images_.find(hash);
>  	if (it == images_.end()) {
> -		images_.insert(make_pair(hash, new FromDiskImage(hash, texture_cache_)));
> +		images_.insert(make_pair(hash, new_in_memory_image(load_image(hash).release())));
>  		return get(hash);
>  	}
> -	return it->second;
> +	return it->second.get();
>  }
> 
> === modified file 'src/graphic/image_cache.h'
> --- src/graphic/image_cache.h	2014-11-24 07:10:03 +0000
> +++ src/graphic/image_cache.h	2014-12-07 19:05:20 +0000
> @@ -20,16 +20,15 @@
>  #ifndef WL_GRAPHIC_IMAGE_CACHE_H
>  #define WL_GRAPHIC_IMAGE_CACHE_H
>  
> +#include <map>
> +#include <memory>
>  #include <string>
> -#include <map>
>  
>  #include <boost/utility.hpp>
>  
>  #include "base/macros.h"
>  #include "graphic/image.h"
>  
> -class TextureCache;
> -
>  // For historic reasons, most part of the Widelands code base expect that an
>  // Image stays valid for the whole duration of the program run. This class is
>  // the one that keeps ownership of all Images to ensure that this is true. Also
> @@ -39,14 +38,12 @@
>  // releasing their ownership.
>  class ImageCache {
>  public:
> -	// Does not take ownership.
> -	ImageCache(TextureCache* texture_cache);
> +	ImageCache();
>  	~ImageCache();
>  
> -	// Insert the given Image into the cache. The hash is defined by Image's hash()
> -	// function. Ownership of the Image is taken. Will return a pointer to the freshly inserted
> -	// image for convenience.
> -	const Image* insert(const Image*);
> +	// Insert the given Image into the cache.
> +	// Will return a pointer to the freshly inserted image for convenience.
> +	const Image* insert(const std::string& hash, std::unique_ptr<const Image> image);
>  
>  	// Returns the image associated with the given hash. If no image by this
>  	// hash is known, it will try to load one from disk with the filename =
> @@ -57,10 +54,9 @@
>  	bool has(const std::string& hash) const;
>  
>  private:
> -	using ImageMap = std::map<std::string, const Image*>;
> +	using ImageMap = std::map<std::string, std::unique_ptr<const Image>>;
>  
>  	ImageMap images_;  /// hash of cached filename/image pairs
> -	TextureCache* const texture_cache_;  // Not owned.
>  
>  	DISALLOW_COPY_AND_ASSIGN(ImageCache);
>  };
> 
> === modified file 'src/graphic/image_io.cc'
> --- src/graphic/image_io.cc	2014-12-03 07:39:19 +0000
> +++ src/graphic/image_io.cc	2014-12-07 19:05:20 +0000
> @@ -56,8 +56,8 @@
>  
>  }  // namespace
>  
> -Texture* load_image(const std::string& fname, FileSystem* fs) {
> -	return new Texture(load_image_as_sdl_surface(fname, fs));
> +std::unique_ptr<Texture> load_image(const std::string& fname, FileSystem* fs) {
> +	return std::unique_ptr<Texture>(new Texture(load_image_as_sdl_surface(fname, fs)));
>  }
>  
>  SDL_Surface* load_image_as_sdl_surface(const std::string& fname, FileSystem* fs) {
> 
> === modified file 'src/graphic/image_io.h'
> --- src/graphic/image_io.h	2014-11-30 09:17:50 +0000
> +++ src/graphic/image_io.h	2014-12-07 19:05:20 +0000
> @@ -20,6 +20,7 @@
>  #ifndef WL_GRAPHIC_IMAGE_IO_H
>  #define WL_GRAPHIC_IMAGE_IO_H
>  
> +#include <memory>
>  #include <string>
>  
>  #include "base/wexception.h"
> @@ -45,7 +46,7 @@
>  };
>  
>  /// Loads the image 'fn' from 'fs'.
> -Texture* load_image(const std::string& fn, FileSystem* fs = nullptr);
> +std::unique_ptr<Texture> load_image(const std::string& fn, FileSystem* fs = nullptr);
>  
>  /// Loads the image 'fn' from 'fs' into an SDL_Surface. Caller must SDL_FreeSurface() the returned value.
>  SDL_Surface* load_image_as_sdl_surface(const std::string& fn, FileSystem* fs = nullptr);
> 
> === modified file 'src/graphic/in_memory_image.cc'
> --- src/graphic/in_memory_image.cc	2014-12-07 10:31:12 +0000
> +++ src/graphic/in_memory_image.cc	2014-12-07 19:05:20 +0000
> @@ -38,24 +38,25 @@
>  // or prepare for core dumps.
>  class InMemoryImage : public Image {
>  public:
> +<<<<<<< TREE
>  	InMemoryImage(const string& ghash, Texture* init_texture) :
>  		hash_(ghash), texture_(init_texture) {}
> +=======
> +	InMemoryImage(Texture* texture) :
> +		texture_(texture) {}
> +>>>>>>> MERGE-SOURCE
>  	virtual ~InMemoryImage() {
>  	}
>  
>  	// Implements Image.
>  	uint16_t width() const override {return texture_->width();}
>  	uint16_t height() const override {return texture_->height();}
> -	// Note: hash will mostly be dummy values for this implementation. It should
> -	// not wind up in ImageCache, otherwise the ownership question is not clear.
> -	const string& hash() const override {return hash_;}
>  	Texture* texture() const override {return texture_.get();}
>  
>  private:
> -	const string hash_;
>  	std::unique_ptr<Texture> texture_;
>  };
>  
> -const Image* new_in_memory_image(const string& hash, Texture* texture) {
> -	return new InMemoryImage(hash, texture);
> +std::unique_ptr<const Image> new_in_memory_image(Texture* texture) {
> +	return std::unique_ptr<const Image>(new InMemoryImage(texture));
>  }
> 
> === modified file 'src/graphic/in_memory_image.h'
> --- src/graphic/in_memory_image.h	2014-11-24 07:10:03 +0000
> +++ src/graphic/in_memory_image.h	2014-12-07 19:05:20 +0000
> @@ -20,11 +20,11 @@
>  #ifndef WL_GRAPHIC_IN_MEMORY_IMAGE_H
>  #define WL_GRAPHIC_IN_MEMORY_IMAGE_H
>  
> -#include <string>
> +#include <memory>
>  
>  class Texture;
>  class Image;
>  
> -const Image* new_in_memory_image(const std::string& hash, Texture* texture);
> +std::unique_ptr<const Image> new_in_memory_image(Texture* texture);
>  
>  #endif  // end of include guard: WL_GRAPHIC_IN_MEMORY_IMAGE_H
> 
> === modified file 'src/graphic/minimap_renderer.cc'
> --- src/graphic/minimap_renderer.cc	2014-11-27 21:29:21 +0000
> +++ src/graphic/minimap_renderer.cc	2014-12-07 19:05:20 +0000
> @@ -308,7 +308,7 @@
>  
>  	// Render minimap
>  	std::unique_ptr<Texture> texture(draw_minimap(egbase, player, viewpoint, layers));
> -	std::unique_ptr<const Image> image(new_in_memory_image("minimap", texture.release()));
> +	std::unique_ptr<const Image> image(new_in_memory_image(texture.release()));
>  	g_gr->save_png(image.get(), streamwrite);
>  	image.reset();
>  }
> 
> === modified file 'src/graphic/surface.h'
> --- src/graphic/surface.h	2014-12-07 19:05:19 +0000
> +++ src/graphic/surface.h	2014-12-07 19:05:20 +0000
> @@ -33,7 +33,7 @@
>   * Interface to a basic surfaces that can be used as destination for blitting and drawing.
>   * It also allows low level pixel access.
>   */
> -class Surface  {
> +class Surface {
>  public:
>  	Surface() = default;
>  	virtual ~Surface() {}
> 
> === modified file 'src/graphic/text/test/render.cc'
> --- src/graphic/text/test/render.cc	2014-11-24 07:10:03 +0000
> +++ src/graphic/text/test/render.cc	2014-12-07 19:05:20 +0000
> @@ -36,7 +36,7 @@
>  	g_fs->add_file_system(&FileSystem::create(RICHTEXT_DATA_DIR));
>  
>  	texture_cache_.reset(create_texture_cache(500 << 20));  // 500 MB
> -	image_cache_.reset(new ImageCache(texture_cache_.get()));
> +	image_cache_.reset(new ImageCache());
>  	renderer_.reset(new RT::Renderer(image_cache_.get(), texture_cache_.get()));
>  }
>  
> 
> === modified file 'src/logic/building.cc'
> --- src/logic/building.cc	2014-11-30 18:49:38 +0000
> +++ src/logic/building.cc	2014-12-07 19:05:20 +0000
> @@ -889,7 +889,7 @@
>  	// animations of buildings so that the messages can still be displayed, even
>  	// after reload.
>  	const std::string& img = g_gr->animations().get_animation
> -		(get_ui_anim()).representative_image_from_disk().hash();
> +		(get_ui_anim()).representative_image_from_disk_filename();
>  	std::string rt_description;
>  	rt_description.reserve
>  		(strlen("<rt image=") + img.size() + 1 +
> 
> === modified file 'src/map_io/map_extradata_packet.cc'
> --- src/map_io/map_extradata_packet.cc	2014-12-01 06:19:03 +0000
> +++ src/map_io/map_extradata_packet.cc	2014-12-07 19:05:20 +0000
> @@ -59,7 +59,8 @@
>  					const std::string hash = std::string("map:") + FileSystem::fs_filename(pname->c_str());
>  					const Image* image = nullptr;
>  					if (!g_gr->images().has(hash)) {
> -						image = g_gr->images().insert(new_in_memory_image(hash, load_image(*pname, &fs)));
> +						image = g_gr->images().insert(
> +						   hash, new_in_memory_image(load_image(*pname, &fs).release()));
>  					} else {
>  						image = g_gr->images().get(hash);
>  					}
> 
> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc	2014-12-03 20:13:44 +0000
> +++ src/scripting/lua_map.cc	2014-12-07 19:05:20 +0000
> @@ -1097,8 +1097,7 @@
>  */
>  int LuaMapObjectDescription::get_representative_image(lua_State * L) {
>  	const std::string& filepath = g_gr->animations().get_animation
> -		(get()->get_animation("idle")).representative_image_from_disk().hash();
> -
> +		(get()->get_animation("idle")).representative_image_from_disk_filename();
>  	lua_pushstring(L, filepath);
>  	return 1;
>  }
> 
> === modified file 'src/ui_fsmenu/loadgame.cc'
> --- src/ui_fsmenu/loadgame.cc	2014-12-04 09:18:03 +0000
> +++ src/ui_fsmenu/loadgame.cc	2014-12-07 19:05:20 +0000
> @@ -321,8 +321,7 @@
>  									minimap_path,
>  									std::unique_ptr<FileSystem>(g_fs->make_sub_file_system(gamedata.filename)).get()));
>  
> -					m_minimap_image.reset(new_in_memory_image(std::string(gamedata.filename + minimap_path),
> -																			texture.release()));
> +					m_minimap_image = new_in_memory_image(texture.release());
>  
>  					// Scale it
>  					double scale = double(m_minimap_w) / m_minimap_image->width();
> 
> === modified file 'src/wui/minimap.cc'
> --- src/wui/minimap.cc	2014-11-30 18:49:38 +0000
> +++ src/wui/minimap.cc	2014-12-07 19:05:20 +0000
> @@ -71,7 +71,7 @@
>  	                   Point((m_viewx - get_w() / 2), (m_viewy - get_h() / 2)),
>  	                *m_flags | MiniMapLayer::ViewWindow));
>  	// Give ownership of the texture to the new image
> -	std::unique_ptr<const Image> im(new_in_memory_image("minimap", texture.release()));
> +	std::unique_ptr<const Image> im(new_in_memory_image(texture.release()));
>  	dst.blit(Point(), im.get());
>  	im.reset();
>  }
> 


-- 
https://code.launchpad.net/~widelands-dev/widelands/remove_in_memory_image/+merge/243911
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/remove_player_color.