widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/remove_in_memory_image into lp:widelands with lp:~widelands-dev/widelands/remove_player_color as a prerequisite.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/remove_in_memory_image/+merge/243911

InMemoryImage and FromDiskImage are now the same thing, only richtext images are
special in any kind of way.


-- 
This is only a tiny cleanup, but I think it is useful and should go in. I need to experiment with the future direction I want to go, so I push this now for review. 
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/remove_in_memory_image into lp:widelands.

=== 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 16:02:24 +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 16:02:23 +0000
+++ src/graphic/animation.cc	2014-12-07 16:02:24 +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 16:02:23 +0000
+++ src/graphic/animation.h	2014-12-07 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:23 +0000
+++ src/graphic/graphic.cc	2014-12-07 16:02:24 +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 16:02:24 +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 16:02:24 +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));
+	const Image* return_value = image.get();
+	images_.insert(make_pair(hash, std::move(image)));
+	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 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:23 +0000
+++ src/graphic/surface.h	2014-12-07 16:02:24 +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/logic/building.cc'
--- src/logic/building.cc	2014-11-30 18:49:38 +0000
+++ src/logic/building.cc	2014-12-07 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:24 +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 16:02:24 +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();
 }