widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/graphic_resetting into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)

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

There is at least one critical bug lurking in this code - but I wasn't able to produce a reliable crash scenario, not did I see the bug. Reaching out for a bunch more eyes :)
-- 
https://code.launchpad.net/~widelands-dev/widelands/graphic_resetting/+merge/159985
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/graphic_resetting into lp:widelands.

=== modified file 'src/graphic/font_handler.cc'
--- src/graphic/font_handler.cc	2013-03-01 16:50:11 +0000
+++ src/graphic/font_handler.cc	2013-04-21 18:52:24 +0000
@@ -87,9 +87,13 @@
 
 
 Font_Handler::~Font_Handler() {
+	flush();
 	Font::shutdown();
 }
 
+void Font_Handler::flush() {
+	d.reset(new Data);
+}
 
 /*
  * Returns the height of the font, in pixels.

=== modified file 'src/graphic/font_handler.h'
--- src/graphic/font_handler.h	2013-02-10 16:41:12 +0000
+++ src/graphic/font_handler.h	2013-04-21 18:52:24 +0000
@@ -68,6 +68,9 @@
 	void do_align
 		(Align, int32_t & dstx, int32_t & dsty, int32_t w, int32_t h);
 
+	// Delete the whole cache.
+	void flush();
+
 private:
 	struct Data;
 	boost::scoped_ptr<Data> d;

=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2013-03-09 14:41:03 +0000
+++ src/graphic/graphic.cc	2013-04-21 18:52:24 +0000
@@ -44,6 +44,7 @@
 
 #include "animation.h"
 #include "animation_gfx.h"
+#include "font_handler.h"
 #include "image.h"
 #include "image_loader_impl.h"
 #include "image_transformations.h"
@@ -62,14 +63,9 @@
 /**
  * Initialize the SDL video mode.
 */
-Graphic::Graphic
-	(int32_t w, int32_t h,
-	 int32_t bpp,
-	 bool    fullscreen,
-	 bool    opengl)
+Graphic::Graphic()
 	:
 	m_fallback_settings_in_effect (false),
-	m_rendertarget     (0),
 	m_nr_update_rects  (0),
 	m_update_fullscreen(true),
 	image_loader_(new ImageLoaderImpl()),
@@ -93,8 +89,12 @@
 	assert(not opengl);
 #endif
 
+}
+
+void Graphic::initialize(int32_t w, int32_t h, int32_t bpp, bool fullscreen, bool opengl) {
+	cleanup();
+
 	// Set video mode using SDL. First collect the flags
-
 	int32_t flags = 0;
 	g_opengl = false;
 	SDL_Surface * sdlsurface = 0;
@@ -106,7 +106,6 @@
 		flags |= SDL_OPENGL;
 	}
 #endif
-
 	if (fullscreen) {
 		flags |= SDL_FULLSCREEN;
 		log("Graphics: Trying FULLSCREEN\n");
@@ -115,7 +114,6 @@
 	log("Graphics: Try to set Videomode %ux%u %uBit\n", w, h, bpp);
 	// Here we actually set the video mode
 	sdlsurface = SDL_SetVideoMode(w, h, bpp, flags);
-
 #ifdef USE_OPENGL
 	// If we tried opengl and it was not successful try without opengl
 	if (!sdlsurface and opengl)
@@ -125,7 +123,6 @@
 		sdlsurface = SDL_SetVideoMode(w, h, bpp, flags);
 	}
 #endif
-
 	if (!sdlsurface)
 	{
 		log
@@ -288,7 +285,6 @@
 		glEnable(GL_TEXTURE_2D);
 
 		GLSurfaceTexture::Initialize();
-
 	}
 
 	if (g_opengl)
@@ -298,11 +294,11 @@
 	else
 #endif
 	{
-		screen_.reset(new SDLSurface(sdlsurface));
+		screen_.reset(new SDLSurface(sdlsurface, false));
 	}
 
 	m_sdl_screen = sdlsurface;
-	m_rendertarget = new RenderTarget(screen_.get());
+	m_rendertarget.reset(new RenderTarget(screen_.get()));
 }
 
 bool Graphic::check_fallback_settings_in_effect()
@@ -310,16 +306,15 @@
 	return m_fallback_settings_in_effect;
 }
 
-/**
- * Free the surface
-*/
-Graphic::~Graphic()
-{
+void Graphic::cleanup() {
 	BOOST_FOREACH(Texture* texture, m_maptextures)
 		delete texture;
-	delete m_rendertarget;
 
 	flush_animations();
+	surface_cache_->flush();
+	// TODO: this should really not be needed, but currently is :(
+	if (UI::g_fh)
+		UI::g_fh->flush();
 
 #if USE_OPENGL
 	if (g_opengl)
@@ -327,6 +322,11 @@
 #endif
 }
 
+Graphic::~Graphic()
+{
+	cleanup();
+}
+
 /**
  * Return the screen x resolution
 */
@@ -350,7 +350,7 @@
 {
 	m_rendertarget->reset();
 
-	return m_rendertarget;
+	return m_rendertarget.get();
 }
 
 /**

=== modified file 'src/graphic/graphic.h'
--- src/graphic/graphic.h	2013-03-09 08:07:22 +0000
+++ src/graphic/graphic.h	2013-04-21 18:52:24 +0000
@@ -86,10 +86,12 @@
  */
 class Graphic {
 public:
-	Graphic
-		(int32_t w, int32_t h, int32_t bpp,
+	Graphic();
+	~Graphic();
+
+	// Initialize or reinitialize the graphics system. Throws on error.
+	void initialize(int32_t w, int32_t h, int32_t bpp,
 		 bool fullscreen, bool opengl);
-	~Graphic();
 
 	int32_t get_xres() const;
 	int32_t get_yres() const;
@@ -133,6 +135,7 @@
 	bool check_fallback_settings_in_effect();
 
 private:
+	void cleanup();
 	void save_png_(Surface & surf, StreamWrite*) const;
 
 	bool m_fallback_settings_in_effect;
@@ -153,7 +156,7 @@
 	/// manipulation the screen context.
 	SDL_Surface * m_sdl_screen;
 	/// A RenderTarget for screen_. This is initialized during init()
-	RenderTarget * m_rendertarget;
+	boost::scoped_ptr<RenderTarget> m_rendertarget;
 	/// keeps track which screen regions needs to be redrawn during the next
 	/// update(). Only used for SDL rendering.
 	SDL_Rect m_update_rects[MAX_RECTS];

=== modified file 'src/graphic/image_cache.h'
--- src/graphic/image_cache.h	2013-02-10 16:08:37 +0000
+++ src/graphic/image_cache.h	2013-04-21 18:52:24 +0000
@@ -25,6 +25,7 @@
 #include <boost/utility.hpp>
 
 #include "image.h"
+
 class IImageLoader;
 class SurfaceCache;
 

=== modified file 'src/graphic/render/sdl_surface.cc'
--- src/graphic/render/sdl_surface.cc	2013-03-01 23:12:08 +0000
+++ src/graphic/render/sdl_surface.cc	2013-04-21 18:52:24 +0000
@@ -26,7 +26,8 @@
 SDLSurface::~SDLSurface() {
 	assert(m_surface);
 
-	SDL_FreeSurface(m_surface);
+	if (m_free_surface_on_delete)
+		SDL_FreeSurface(m_surface);
 }
 
 const SDL_PixelFormat & SDLSurface::format() const {

=== modified file 'src/graphic/render/sdl_surface.h'
--- src/graphic/render/sdl_surface.h	2013-02-10 16:41:12 +0000
+++ src/graphic/render/sdl_surface.h	2013-04-21 18:52:24 +0000
@@ -33,10 +33,13 @@
 */
 class SDLSurface : public Surface {
 public:
-	SDLSurface(SDL_Surface* surface) :
+	// The surface set by SetVideoMode must not be freed according to the SDL
+	// docs, so we need an extra param to be sure we do net.
+	SDLSurface(SDL_Surface* surface, bool free_surface_on_delete = true) :
 		m_surface(surface),
 		m_offsx(0), m_offsy(0),
-		m_w(surface->w), m_h(surface->h)
+		m_w(surface->w), m_h(surface->h),
+		m_free_surface_on_delete(free_surface_on_delete)
 	{}
 	virtual ~SDLSurface();
 
@@ -72,6 +75,7 @@
 	int32_t m_offsx;
 	int32_t m_offsy;
 	uint16_t m_w, m_h;
+	bool m_free_surface_on_delete;
 };
 
 

=== modified file 'src/graphic/surface_cache.cc'
--- src/graphic/surface_cache.cc	2013-03-01 18:36:40 +0000
+++ src/graphic/surface_cache.cc	2013-04-21 18:52:24 +0000
@@ -40,6 +40,7 @@
 	virtual ~SurfaceCacheImpl();
 
 	// Implements SurfaceCache.
+	virtual void flush();
 	virtual Surface* get(const std::string& hash);
 	virtual Surface* insert(const std::string& hash, Surface*);
 
@@ -64,9 +65,16 @@
 };
 
 SurfaceCacheImpl::~SurfaceCacheImpl() {
+	flush();
+}
+
+void SurfaceCacheImpl::flush() {
 	for (Container::iterator it = map_.begin(); it != map_.end(); ++it) {
 		delete it->second;
 	}
+	map_.clear();
+	hist_.clear();
+	used_memory_ = 0;
 }
 
 Surface* SurfaceCacheImpl::get(const std::string& hash) {

=== modified file 'src/graphic/surface_cache.h'
--- src/graphic/surface_cache.h	2013-02-10 14:37:59 +0000
+++ src/graphic/surface_cache.h	2013-04-21 18:52:24 +0000
@@ -36,6 +36,9 @@
 	SurfaceCache() {};
 	virtual ~SurfaceCache() {};
 
+	/// Deletes all surfaces in the cache leaving it as if it were just created.
+	virtual void flush() = 0;
+
 	/// Returns an entry if it is cached, NULL otherwise.
 	virtual Surface* get(const std::string& hash) = 0;
 

=== modified file 'src/helper.cc'
--- src/helper.cc	2013-02-10 19:36:24 +0000
+++ src/helper.cc	2013-04-21 18:52:24 +0000
@@ -17,9 +17,16 @@
  *
  */
 
+#include <cstdarg>
+#include <string>
+
+#include <boost/random.hpp>
+#include <boost/scoped_array.hpp>
+
 #include "helper.h"
 
-#include <cstdarg>
+using namespace std;
+
 
 /// Split a string by separators.
 /// \note This ignores empty elements, so do not use this for example to split
@@ -71,3 +78,15 @@
 		((k.sym >= SDLK_WORLD_0) && (k.sym <= SDLK_WORLD_95)) ||
 		((k.sym >= SDLK_KP0)     && (k.sym <= SDLK_KP_EQUALS));
 }
+
+static boost::random::mt19937 random_generator;
+string random_string(const string& chars, int nlen) {
+	boost::random::uniform_int_distribution<> index_dist(0, chars.size() - 1);
+	boost::scoped_array<char> buffer(new char[nlen - 1]);
+	for (int i = 0; i < nlen; ++i) {
+		buffer[i] = chars[index_dist(random_generator)];
+	}
+	buffer[nlen] = '\0';
+	return string(buffer.get());
+}
+

=== modified file 'src/helper.h'
--- src/helper.h	2013-02-10 19:36:24 +0000
+++ src/helper.h	2013-04-21 18:52:24 +0000
@@ -158,4 +158,7 @@
 
 bool is_printable(SDL_keysym k);
 
+/// Generate a random string of given size out of the given alphabet.
+std::string random_string(const std::string& chars, int nlen);
+
 #endif

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2013-03-02 20:35:18 +0000
+++ src/ui_basic/panel.cc	2013-04-21 18:52:24 +0000
@@ -17,15 +17,14 @@
  *
  */
 
-#include <boost/concept_check.hpp>
-
 #include "constants.h"
 #include "graphic/font_handler.h"
 #include "graphic/font_handler1.h"
 #include "graphic/graphic.h"
-#include "graphic/in_memory_image.h"
 #include "graphic/rendertarget.h"
 #include "graphic/surface.h"
+#include "graphic/surface_cache.h"
+#include "helper.h"
 #include "log.h"
 #include "profile/profile.h"
 #include "sound/sound_handler.h"
@@ -34,8 +33,37 @@
 
 #include "panel.h"
 
+using namespace std;
+
 namespace UI {
 
+namespace  {
+class CacheImage : public Image {
+public:
+	CacheImage(uint16_t w, uint16_t h) :
+		width_(w), height_(h),
+		hash_("cache_image_" + random_string("0123456789ABCDEFGH", 32)) {}
+	virtual ~CacheImage() {}
+
+	// Implements Image.
+	virtual uint16_t width() const {return width_;}
+	virtual uint16_t height() const {return height_;}
+	virtual const string& hash() const {return hash_;}
+	virtual Surface* surface() const {
+		Surface* rv = g_gr->surfaces().get(hash_);
+		if (rv)
+			return rv;
+
+		rv = g_gr->surfaces().insert(hash_, Surface::create(width_, height_));
+		return rv;
+	}
+
+private:
+	const int16_t width_, height_;
+	const string hash_;
+};
+
+}  // namespace
 Panel * Panel::_modal       = 0;
 Panel * Panel::_g_mousegrab = 0;
 Panel * Panel::_g_mousein   = 0;
@@ -835,12 +863,8 @@
 		uint32_t innerw = _w - (_lborder + _rborder);
 		uint32_t innerh = _h - (_tborder + _bborder);
 
-		if
-			(!_cache ||
-			 _cache.get()->width() != innerw ||
-			 _cache.get()->height() != innerh)
-		{
-			_cache.reset(new_in_memory_image("dummy_hash", Surface::create(innerw, innerh)));
+	if (!_cache || _cache->width() != innerw || _cache->height() != innerh) {
+			_cache.reset(new CacheImage(innerw, innerh));
 			_needdraw = true;
 		}
 

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2013-04-20 20:20:34 +0000
+++ src/wlapplication.cc	2013-04-21 18:52:24 +0000
@@ -262,9 +262,6 @@
 m_mouse_locked         (0),
 m_mouse_compensate_warp(0, 0),
 m_should_die           (false),
-m_gfx_w(0), m_gfx_h(0), m_gfx_bpp(0),
-m_gfx_fullscreen       (false),
-m_gfx_opengl           (true),
 m_default_datadirs     (true),
 #ifdef WIN32
 m_homedir(FileSystem::GetHomedir() + "\\.widelands"),
@@ -469,7 +466,6 @@
 			throw;
 		}
 	} else {
-
 		g_sound_handler.start_music("intro");
 
 		{
@@ -791,42 +787,25 @@
 }
 
 /**
- * Initialize the graphics subsystem (or shutdown, if system == GFXSYS_NONE)
+ * Initialize the graphics subsystem (or shutdown, if w and h are 0)
  * with the given resolution.
  * Throws an exception on failure.
- *
- * \note Because of the way pictures are handled now, this function must not be
- * called while UI elements are active.
- *
- * \todo Ensure that calling this with active UI elements does barf
- * \todo Document parameters
  */
-
 void WLApplication::init_graphics
 	(const int32_t w, const int32_t h, const int32_t bpp,
 	 const bool fullscreen, const bool opengl)
 {
-	if
-		(w == m_gfx_w && h == m_gfx_h && bpp == m_gfx_bpp &&
-		 fullscreen == m_gfx_fullscreen &&
-		 opengl == m_gfx_opengl)
+	if (!w && !h) { // shutdown.
+		delete g_gr;
+		g_gr = 0;
 		return;
-
-	delete g_gr;
-	g_gr = 0;
-
-	m_gfx_w = w;
-	m_gfx_h = h;
-	m_gfx_bpp = bpp;
-	m_gfx_fullscreen = fullscreen;
-	m_gfx_opengl = opengl;
-
-
-	// If we are not to be shut down
-	if (w && h) {
-		g_gr = new Graphic
-			(w, h, bpp, fullscreen, opengl);
-	}
+	}
+	assert(w > 0 && h > 0);
+
+	if (!g_gr) {
+		g_gr = new Graphic();
+	}
+	g_gr->initialize(w, h, bpp, fullscreen, opengl);
 }
 
 void WLApplication::refresh_graphics()
@@ -837,7 +816,7 @@
 	init_graphics
 		(s.get_int("xres", XRES),
 		 s.get_int("yres", YRES),
-		 s.get_int("depth", 16),
+		 s.get_int("depth", 32),
 		 s.get_bool("fullscreen", false),
 #if USE_OPENGL
 		 s.get_bool("opengl", true));
@@ -866,16 +845,12 @@
 	set_input_grab(s.get_bool("inputgrab", false));
 	set_mouse_swap(s.get_bool("swapmouse", false));
 
-	m_gfx_fullscreen = s.get_bool("fullscreen", false);
-
-#if USE_OPENGL
-	m_gfx_opengl = s.get_bool("opengl", true);
-#endif
-
 	// KLUDGE!
 	// Without this the following config options get dropped by check_used().
 	// Profile needs support for a Syntax definition to solve this in a
 	// sensible way
+	s.get_bool("fullscreen");
+	s.get_bool("opengl");
 	s.get_int("xres");
 	s.get_int("yres");
 	s.get_int("border_snap_distance");
@@ -1072,12 +1047,7 @@
 	SDL_EnableUNICODE(1); //needed by helper.h:is_printable()
 	SDL_EnableKeyRepeat(SDL_DEFAULT_REPEAT_DELAY, SDL_DEFAULT_REPEAT_INTERVAL);
 
-	uint32_t xres = s.get_int("xres", XRES);
-	uint32_t yres = s.get_int("yres", YRES);
-
-	init_graphics
-		(xres, yres, s.get_int("depth", 16),
-		 m_gfx_fullscreen, m_gfx_opengl);
+	refresh_graphics();
 
 	// Start the audio subsystem
 	// must know the locale before calling this!

=== modified file 'src/wlapplication.h'
--- src/wlapplication.h	2013-02-10 20:07:27 +0000
+++ src/wlapplication.h	2013-04-21 18:52:24 +0000
@@ -283,21 +283,6 @@
 	///true if an external entity wants us to quit
 	bool   m_should_die;
 
-	///The Widelands window's width in pixels
-	int32_t    m_gfx_w;
-
-	///The Widelands window's height in pixels
-	int32_t    m_gfx_h;
-
-	///The Widelands window's bits per pixel
-	int32_t    m_gfx_bpp;
-
-	///If true Widelands is (should be, we never know ;-) running
-	///in a fullscreen window
-	bool   m_gfx_fullscreen;
-
-	bool   m_gfx_opengl;
-
 	//do we want to search the default places for widelands installs
 	bool   m_default_datadirs;
 	std::string m_homedir;